diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 6b8e9c18..8f439426 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -18,13 +18,13 @@ "name": "work-system", "source": "./plugins/work-system", "description": "Generic task and worktree workflow system for Claude Code. Keep tasks as GitHub issues or local markdown files, run them in isolated git worktrees with a choice of worker agent (Claude, codex, grok, kimi, or a PATH-detected cc-harness agent), and track progress across the define/kickoff/continue/status/close lifecycle.", - "version": "1.28.2" + "version": "1.28.4" }, { "name": "pr-flow", "source": "./plugins/pr-flow", "description": "PR review feedback loop for Claude Code. Create PRs with readiness checks, commit + push + trigger @claude review, inspect status, and work through review issues interactively.", - "version": "1.8.1" + "version": "1.8.2" }, { "name": "swarm", diff --git a/.claude/knowledge/features/worker-autonomy-mandate.md b/.claude/knowledge/features/worker-autonomy-mandate.md index e4fd925b..9c027bd3 100644 --- a/.claude/knowledge/features/worker-autonomy-mandate.md +++ b/.claude/knowledge/features/worker-autonomy-mandate.md @@ -239,20 +239,35 @@ false, and `mandate.sh lane ` is why: that resolver exists so a Manager session running `/cycle` from the main repo can act on a *worker's* worktree. The worker's own review loop and the Manager's can therefore book a round at the same instant, both read the same count, and a budget of two funds an unbounded number -of reviews. The lock is eight lines of `mkdir`. Worth re-reading a standing -rejection whenever the surrounding design moves under it — the argument was sound -when first made and quietly stopped being true. - -Its first version had a TOCTOU of its own (CI flake, rc=4, 1.28.1): after a -failed `mkdir` it tested `[ ! -d "$lock" ]` to tell "cannot create" from -contention — but the holder can release the lock in between, so contention -read as a read-only tree. Decide from `mkdir`'s own error (`LC_ALL=C`, "File -exists" = contention) instead of from the lock's state a moment later. -Reproduce such races by running many copies of the concurrent test at once -(16/72 runs failed); pin the fix with a PATH `mkdir` shim that fails once with -"File exists" and no lock left behind. The pid is written aside and renamed -in, so `lock_is_stale` never reads a partial pid; a young lock without one is -never stale, which covers the `mkdir`-to-pid gap. +of reviews. Worth re-reading a standing rejection whenever the surrounding +design moves under it — the argument was sound when first made and quietly +stopped being true. + +The lock is a `flock(2)` on `.MANDATE.lock` (1.28.4), not the `mkdir` lock it +started as. A `mkdir` lock outlives `kill -9`, so it needs a "the owner is dead, +break it" rule, and that rule has no safe form: a breaker reads the dead owner's +pid, the lock is meanwhile released and re-taken by a live booker, and the +breaker deletes the live lock (ABA; a directory cannot be deleted +conditionally). Two earlier fixes (a TOCTOU after a failed `mkdir`, the pid +written aside and renamed in) each closed one window and left this one. The +kernel drops a flock when the last descriptor closes, so nothing ever judges a +lock abandoned. Mechanics worth keeping: bash holds the descriptor +(`exec 9<>file`) and a short `python3 -I` flocks the inherited fd 9 — flock +belongs to the open file description, so the lock outlives the python process +and lasts as long as the script (macOS has no `flock(1)`; `lockf`/`fcntl` +record locks would die with python). Every child inherits fd 9, so a daemon +spawned under the lock would hold it forever: `mandate.sh` wraps `git` to close +fd 9 (a PATH git shim that probes `( true >&9 )` pins it — not `: >&9`, whose +failed redirect on a special builtin exits dash; only `fixed`/`acknowledged` +run git under the lock, so `round` cannot test it). Never delete the lock file: +unlinking a flock file lets two processes lock two different inodes. Because it +stays, `close-request.sh` counts `.MANDATE.lock` as a lane file (else every +mandate lane loses the zero-loss auto-close) and every writer re-ensures the +`.MANDATE.*` exclude. Sessions on 1.28.2 and older cannot share a lane with +1.28.4: their mkdir refuses the lock file and advises `rm -f`, which would +unlink a held lock — reload them (`/sessions`) first. +Reproduce such races by running many concurrent bookings in a loop (16 at once, +1 of 40 runs lost a booking before, 0 of 160 after). ## Indentation is structure too diff --git a/CHANGELOG.md b/CHANGELOG.md index c7738514..72952374 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,9 @@ entries are grouped per plugin, newest first. ## work-system +### 1.28.4 — 2026-10-27 +- `mandate.sh` locks the record writers (`round`, `reviewed`, `fixed`, `acknowledged`) with `flock(2)` on `.MANDATE.lock` instead of a `mkdir` lock directory. Breaking an "abandoned" `mkdir` lock raced: a breaker read the dead owner's pid, the lock was released and taken by a live booker meanwhile, and the breaker deleted the live lock, so two writers ran and a concurrent `round` booking was lost (CI flake in `test_mandate.py`; 1 of 40 runs of 16 concurrent bookings on 1.28.2, 0 of 160 now). The kernel frees a flock when its owner dies, so the staleness rule (pid check, 5-minute age bound) is gone. The lock file stays beside the record (covered by the `.MANDATE.*` exclude); git calls close the lock descriptor so no daemon can inherit it. Taking the lock needs python3 (macOS has no `flock(1)`); without it a write is refused with exit 4, never done unlocked. A leftover lock directory from 1.28.2 or older is refused with the `rm -rf` to run. Reload every session working a lane (`/sessions`) before it books again: a 1.28.2 session refuses the new lock file and advises an `rm -f` that would unlink a held lock. The lock file counts as a lane file for the Manager's zero-loss close, every writer re-ensures the `.MANDATE.*` exclude, and a filesystem that refuses flock is named as such instead of reported as contention. + ### 1.28.2 — 2026-10-27 - `test_close_request.py` and `test_landing.py` build their shadow `PATH` skipping dangling source entries and checking presence with `os.path.lexists`, so a dangling link that `/usr/bin` and `/bin` both list (usrmerge) no longer fails the second `symlink_to` with `FileExistsError`, and a later source's real binary of that name still gets linked (Structure Checks red on main since #91). Tests only; no script behavior changes. @@ -333,6 +336,9 @@ entries are grouped per plugin, newest first. ## pr-flow +### 1.8.2 — 2026-10-28 +- `REVIEW-ROUTING.md` describes `mandate.sh round` exit 4 as work-system 1.28.4 reports it (another session writing the record, no python3 for the lock, a read-only tree); the advice to clear an abandoned lock is gone, since a flock is never abandoned. Docs only. + ### 1.8.1 — 2026-10-22 - Trim the `/open` skill description to the word budget; the readiness-check list lives in the skill body. diff --git a/plugins/pr-flow/.claude-plugin/plugin.json b/plugins/pr-flow/.claude-plugin/plugin.json index 360397ec..68ad1502 100644 --- a/plugins/pr-flow/.claude-plugin/plugin.json +++ b/plugins/pr-flow/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "pr-flow", "description": "PR review feedback loop for Claude Code. Create PRs with readiness checks, commit + push + trigger @claude review, inspect status, and work through review issues interactively.", - "version": "1.8.1", + "version": "1.8.2", "author": { "name": "gering" }, diff --git a/plugins/pr-flow/docs/REVIEW-ROUTING.md b/plugins/pr-flow/docs/REVIEW-ROUTING.md index a7e43890..b2befdc9 100644 --- a/plugins/pr-flow/docs/REVIEW-ROUTING.md +++ b/plugins/pr-flow/docs/REVIEW-ROUTING.md @@ -194,9 +194,9 @@ Branch on **`round_authorized`**, not on the exhaustion flag: ones" — finish, then stop. - `round_authorized=no` → the budget is spent and **nothing was charged**. Stop and ask; do not review. -- exit `4` → the round could not be persisted (read-only tree, or a lock another - session holds). Say so and do not review on an in-session count. The message - names the lock and the command to clear an abandoned one. +- exit `4` → the round could not be persisted (read-only tree, another session + writing the record right now, no python3 for the lock). Say so and do not + review on an in-session count; the message names the cause. Book **once per review**, and only if your own stage does not already book — §3 says which consumer books where. diff --git a/plugins/work-system/.claude-plugin/plugin.json b/plugins/work-system/.claude-plugin/plugin.json index 11af24ee..b09cb373 100644 --- a/plugins/work-system/.claude-plugin/plugin.json +++ b/plugins/work-system/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "work-system", "description": "Generic task and worktree workflow system for Claude Code. Keep tasks as GitHub issues or local markdown files, run them in isolated git worktrees with a choice of worker agent (Claude, codex, grok, kimi, or a PATH-detected cc-harness agent), and track progress across the define/kickoff/continue/status/close lifecycle.", - "version": "1.28.2", + "version": "1.28.4", "author": { "name": "gering" }, diff --git a/plugins/work-system/scripts/close-request.sh b/plugins/work-system/scripts/close-request.sh index 40e53777..9a77b34a 100644 --- a/plugins/work-system/scripts/close-request.sh +++ b/plugins/work-system/scripts/close-request.sh @@ -308,6 +308,8 @@ out(("task", seen["task"]), ("worktree", rp(seen["worktree"])))' "$file" "$TASK_ while IFS= read -r line; do case "$line" in ''|'?? TASK.md'|'?? MANDATE.md'|'?? .ws-kicker'|'!! TASK.md'|'!! MANDATE.md'|'!! .ws-kicker') ;; + # mandate.sh's flock file: lock state that stays beside the record. + '?? .MANDATE.lock'|'!! .MANDATE.lock') ;; '!! '*) ignored=$((ignored + 1)) ;; *) dirty=$((dirty + 1)) ;; esac diff --git a/plugins/work-system/scripts/mandate.sh b/plugins/work-system/scripts/mandate.sh index 898c27ab..c12ba88e 100755 --- a/plugins/work-system/scripts/mandate.sh +++ b/plugins/work-system/scripts/mandate.sh @@ -110,6 +110,12 @@ # negative answer that is NOT a user decision — their `reason=` names it. set -eu +# Every git call closes fd 9, the record lock (acquire_round_lock). A flock lasts +# until the last descriptor on it closes, and git can leave a daemon behind +# (core.fsmonitor) that would otherwise inherit the descriptor and hold the +# lane's lock long after this script exited. +git() { command git "$@" 9>&-; } + MANDATE_FILE="MANDATE.md" MANDATE_VERSION=1 @@ -411,70 +417,82 @@ do_allows() { # so that worker's own review loop and the Manager's can book a round at the same # time. Both read used=1, both write 2, and a budget of 2 funds an unbounded # number of reviews. (This was rejected four times on a "one lane = one worker" -# invariant that the lane resolver itself breaks.) mkdir is the portable atomic -# test-and-set; the name starts with .MANDATE. so the git exclude already covers -# it. A bounded wait then a loud exit 4 — never a silent second booking, and -# never a wedged lane without saying which file to remove. -ROUND_LOCK="" -# Is the lock abandoned? Its owner recorded a pid; if that process is gone, or -# the directory is older than the bound, nobody is coming back for it. Without -# this a `kill -9` (or a herdr tab torn down mid-round) wedged the lane -# permanently — every later round exits 4, and the `.MANDATE.*` exclude hides -# the lock from `git status`, so nothing points at the cause. -LOCK_STALE_MINUTES=5 -lock_is_stale() { - local lock="$1" pid="" - [ -d "$lock" ] || return 1 - [ -f "$lock/pid" ] && pid="$(cat "$lock/pid" 2>/dev/null || true)" - case "$pid" in - ''|*[!0-9]*) ;; - *) if ! kill -0 "$pid" 2>/dev/null; then return 0; fi ;; +# invariant that the lane resolver itself breaks.) The name starts with .MANDATE. +# so the git exclude already covers it. A bounded wait then a loud exit 4 — +# never a silent second booking. +# +# flock(2), held on fd 9 of this shell, not a mkdir lock. A mkdir lock outlives a +# `kill -9`, so it needs a "the owner is gone, break it" rule, and that rule has +# no safe form: a breaker reads the dead owner's pid, the lock is meanwhile +# released and taken by a live booker, and the breaker deletes the live one — +# two writers, one lost booking (CI lost one in eight concurrent rounds). The +# kernel drops a flock when the last descriptor on it closes, so a killed owner +# frees it and nothing ever has to decide that a lock is abandoned. python3 +# takes the lock because macOS ships no flock(1); bash keeps the descriptor, so +# the lock lives exactly as long as this script (or release_round_lock). +ROUND_LOCK_WAIT=5 +# Exit 0 locked, 1 still held when the wait ran out, 2 flock itself failed (a +# filesystem without flock support; the error is on stderr), 3 fd 9 is not the +# regular file at the lock path (a symlink swapped in, a FIFO, a directory). +FLOCK_PY=' +import fcntl, os, stat, sys, time +try: + st, ln = os.fstat(9), os.lstat(sys.argv[1]) +except OSError: + sys.exit(3) +if not stat.S_ISREG(st.st_mode) or (st.st_dev, st.st_ino) != (ln.st_dev, ln.st_ino): + sys.exit(3) +end = time.monotonic() + float(sys.argv[2]) +while True: + try: + fcntl.flock(9, fcntl.LOCK_EX | fcntl.LOCK_NB) + sys.exit(0) + except BlockingIOError: + if time.monotonic() >= end: + sys.exit(1) + time.sleep(0.02) + except OSError as e: + print(e.strerror or e, file=sys.stderr) + sys.exit(2) +' + +acquire_round_lock() { + local lock="${MANDATE_PATH%/*}/.MANDATE.lock" rc=0 err + # A directory here is the mkdir lock of work-system 1.28.2 and older, left by a + # killed run or held by an old session mid-booking. Which one cannot be told + # safely (that is the bug above), so say so instead of guessing. + if [ -d "$lock" ] && [ ! -L "$lock" ]; then + echo "${0##*/}: could not persist the record — $lock is a lock directory from an older work-system. If no older session is booking for this lane right now, remove it and retry: rm -rf '$lock'" >&2 + exit 4 + fi + # Checked before the open, which would follow a symlink and block on a FIFO; + # FLOCK_PY re-checks the opened file against the path, closing the gap. + if [ -L "$lock" ] || { [ -e "$lock" ] && [ ! -f "$lock" ]; }; then + echo "${0##*/}: could not persist the record — $lock is not a lock file; remove it and retry: rm -f '$lock'" >&2 + exit 4 + fi + # The lock file stays for good, so it must never reach a `git add -A`: make + # sure of the exclude here, not only in init (a hand-written MANDATE.md, or + # an init whose exclude write failed). Best-effort, like init's. + exclude_one "${MANDATE_PATH%/*}" ".MANDATE.*" || true + { exec 9<>"$lock"; } 2>/dev/null || { + echo "${0##*/}: could not persist the record — no lock could be created beside $MANDATE_PATH" >&2 + exit 4 + } + err="$(python3 -I -c "$FLOCK_PY" "$lock" "$ROUND_LOCK_WAIT" 2>&1)" || rc=$? + case "$rc" in + 0) return 0 ;; + 1) echo "${0##*/}: could not persist the record — another process is writing this lane's record (it holds $lock). Retry once it finishes." >&2 ;; + 2) echo "${0##*/}: could not persist the record — this filesystem refused the lock on $lock: $err" >&2 ;; + 3) echo "${0##*/}: could not persist the record — $lock is not a lock file; remove it and retry: rm -f '$lock'" >&2 ;; + *) echo "${0##*/}: could not persist the record — taking the lock needs python3 (exit $rc)" >&2 ;; esac - # Age is the backstop: a pid can be reused, and `kill -0` cannot see a live - # process owned by another user. - [ -n "$(find "$lock" -maxdepth 0 -mmin "+$LOCK_STALE_MINUTES" 2>/dev/null)" ] + exec 9>&- + exit 4 } -acquire_round_lock() { - local lock="${MANDATE_PATH%/*}/.MANDATE.lock" i=0 broke=no err - while ! err="$(LC_ALL=C mkdir "$lock" 2>&1)"; do - # mkdir can fail for two very different reasons. Anything but "File exists" - # is not contention (a read-only worktree, a full disk) and retrying cannot - # help — report it in the same words as a failed write, at once, instead of - # sleeping five seconds first. Decided from mkdir's own error, not from - # whether the lock is there now: a holder can release it between the - # failed mkdir and any check, and that is contention like any other. - case "$err" in - *"File exists"*) ;; - *) echo "${0##*/}: could not persist the consumed round — no lock could be created beside $MANDATE_PATH: ${err#mkdir: }" >&2 - exit 4 ;; - esac - # A file or a symlink there is no lock anyone holds: waiting on it would - # only end in blaming a process that does not exist. - if [ -L "$lock" ] || { [ -e "$lock" ] && [ ! -d "$lock" ]; }; then - echo "${0##*/}: could not persist the consumed round — $lock is not a lock directory; remove it and retry: rm -f '$lock'" >&2 - exit 4 - fi - if [ "$broke" = no ] && lock_is_stale "$lock"; then - # Break it ONCE. A second staleness verdict in the same call would mean we - # are racing another breaker, and then waiting is the safer answer. - rm -rf "$lock" 2>/dev/null || true - broke=yes - continue - fi - i=$(( i + 1 )) - if [ "$i" -ge 50 ]; then - echo "${0##*/}: could not consume a review round — another process holds $lock and it does not look abandoned. If no other session is booking a round for this lane, remove it and retry: rm -rf '$lock'" >&2 - exit 4 - fi - sleep 0.1 - done - # Written aside and renamed in, so lock_is_stale never reads a partial pid - # (`12` of `12345` names another, likely dead, process). Until it lands the - # lock has no pid, and a young lock without one is never stale (the age - # backstop), so the mkdir-to-pid gap cannot get a live lock broken. - printf '%s\n' "$$" > "$lock/pid.$$" 2>/dev/null && mv -f "$lock/pid.$$" "$lock/pid" 2>/dev/null || true - ROUND_LOCK="$lock" +release_round_lock() { + exec 9>&- } do_round() { @@ -486,7 +504,6 @@ do_round() { # Locked BEFORE the read: the whole parse -> increment -> mv has to be one # critical section, or two readers still both see the old count. acquire_round_lock - trap 'rm -rf "$ROUND_LOCK"' EXIT parse_frontmatter "$MANDATE_PATH" budget="$FM_review_budget"; used="$FM_review_rounds_used" case "$used" in ''|*[!0-9]*) used=0 ;; esac @@ -498,7 +515,7 @@ do_round() { # `review_budget_exhausted` keeps its one meaning, "no further rounds after # this one". if [ -n "$budget" ] && [ "$used" -ge "$budget" ]; then - rm -rf "$ROUND_LOCK"; trap - EXIT + release_round_lock printf 'round_authorized=no\n' printf 'review_rounds_used=%s\n' "$used" emit_budget "$budget" "$used" @@ -506,8 +523,7 @@ do_round() { fi used=$(( used + 1 )) persist_keys "review_rounds_used=$used" - rm -rf "$ROUND_LOCK" - trap - EXIT + release_round_lock printf 'round_authorized=yes\n' printf 'review_rounds_used=%s\n' "$used" @@ -529,8 +545,8 @@ persist_keys() { # the orphan `.MANDATE.XXXXXX` is untracked in the worktree root — exactly what # a worker authorized to `git add -A` would commit. `ensure_excluded` covers # the pattern too, for the kill -9 the shell can never catch. - trap 'rm -f "$tmp"; rm -rf "$ROUND_LOCK"' EXIT - trap 'rm -f "$tmp"; rm -rf "$ROUND_LOCK"; exit 4' INT TERM HUP + trap 'rm -f "$tmp"' EXIT + trap 'rm -f "$tmp"; exit 4' INT TERM HUP # A key that is ABSENT (hand-edited away, a mandate from another tool, or one # recorded before the key existed) is inserted before the closing fence — the @@ -569,8 +585,7 @@ persist_keys() { echo "${0##*/}: could not persist the record to $MANDATE_PATH" >&2 exit 4 } - trap 'rm -rf "$ROUND_LOCK"' EXIT - trap - INT TERM HUP + trap - EXIT INT TERM HUP # Read the file back and confirm every value is really there before reporting # it. Reporting a round the file never recorded is the single failure the @@ -889,11 +904,10 @@ do_reviewed() { || die "could not record the review: $DIFF_ERR" head="${REV_COMMIT:-$(git -C "${MANDATE_PATH%/*}" rev-parse --verify -q HEAD 2>/dev/null || true)}" acquire_round_lock - trap 'rm -rf "$ROUND_LOCK"' EXIT parse_frontmatter "$MANDATE_PATH" persist_keys "reviewed_head=$head" "reviewed_diff=$DIFF_ID" "fixed_diff=" \ "acked_head=" "acked_diff=" "acked_by=" "acked_at=" - rm -rf "$ROUND_LOCK"; trap - EXIT + release_round_lock printf 'reviewed_recorded=yes\nreviewed_head=%s\nreviewed_diff=%s\n' "$head" "$DIFF_ID" } @@ -913,7 +927,6 @@ do_fixed() { after="$(git -C "${MANDATE_PATH%/*}" rev-parse --verify -q "$after^{commit}" 2>/dev/null)" \ || die "--after names no commit in this repo" acquire_round_lock - trap 'rm -rf "$ROUND_LOCK"' EXIT parse_frontmatter "$MANDATE_PATH" if [ -z "$FM_reviewed_diff" ]; then reason=no-review @@ -926,13 +939,13 @@ do_fixed() { echo "${0##*/}: the last review's fixes are already recorded — further changes need a new review" >&2 fi if [ -n "$reason" ]; then - rm -rf "$ROUND_LOCK"; trap - EXIT + release_round_lock printf 'fixed_recorded=no\nreason=%s\n' "$reason" exit 1 fi lane_diff_id "${MANDATE_PATH%/*}" worktree || die "could not record the fixes: $DIFF_ERR" persist_keys "fixed_diff=$DIFF_ID" - rm -rf "$ROUND_LOCK"; trap - EXIT + release_round_lock printf 'fixed_recorded=yes\nfixed_diff=%s\n' "$DIFF_ID" } @@ -1157,7 +1170,6 @@ do_acknowledged() { dir="${MANDATE_PATH%/*}" record_prepare "$rev" acquire_round_lock - trap 'rm -rf "$ROUND_LOCK"' EXIT parse_frontmatter "$MANDATE_PATH" if [ -z "$FM_reviewed_diff" ]; then reason=no-review @@ -1181,13 +1193,13 @@ do_acknowledged() { fi fi if [ -n "$reason" ]; then - rm -rf "$ROUND_LOCK"; trap - EXIT + release_round_lock printf 'acked_recorded=no\nreason=%s\n' "$reason" exit 1 fi when="$(date -u +%Y-%m-%dT%H:%M:%SZ)" persist_keys "acked_head=$REV_COMMIT" "acked_diff=$HEAD_ID" "acked_by=user" "acked_at=$when" - rm -rf "$ROUND_LOCK"; trap - EXIT + release_round_lock printf 'acked_recorded=yes\nacked_head=%s\nacked_diff=%s\nacked_at=%s\n' "$REV_COMMIT" "$HEAD_ID" "$when" } diff --git a/plugins/work-system/scripts/test_close_request.py b/plugins/work-system/scripts/test_close_request.py index abf28d8f..a4b67b68 100644 --- a/plugins/work-system/scripts/test_close_request.py +++ b/plugins/work-system/scripts/test_close_request.py @@ -305,6 +305,14 @@ def parse(out): check("auto: a git-excluded .ws-kicker is a lane file", o.get("decision") == "auto") (fx.wt / ".ws-kicker").unlink() (fx.main / ".git" / "info" / "exclude").write_text("TASK.md\nMANDATE.md\n") +(fx.wt / ".MANDATE.lock").write_text("") +_, o = fx.evaluate() +check("auto: mandate.sh's untracked lock file is a lane file", o.get("decision") == "auto") +(fx.main / ".git" / "info" / "exclude").write_text("TASK.md\nMANDATE.md\n/.MANDATE.*\n") +_, o = fx.evaluate() +check("auto: a git-excluded .MANDATE.lock is a lane file", o.get("decision") == "auto") +(fx.wt / ".MANDATE.lock").unlink() +(fx.main / ".git" / "info" / "exclude").write_text("TASK.md\nMANDATE.md\n") (fx.main / ".git" / "info" / "exclude").write_text("TASK.md\nMANDATE.md\nsecret.env\n") (fx.wt / "secret.env").write_text("TOKEN=x\n") _, o = fx.evaluate() diff --git a/plugins/work-system/scripts/test_mandate.py b/plugins/work-system/scripts/test_mandate.py index a937b3df..9a1bddb3 100644 --- a/plugins/work-system/scripts/test_mandate.py +++ b/plugins/work-system/scripts/test_mandate.py @@ -443,8 +443,10 @@ def init(repo, *extra): kv(run("show", str(repo4)).stdout).get("review_rounds_used"))[2] == "2") check("round kept the human body intact", "# Mandate" in (repo4 / "MANDATE.md").read_text()) +# .MANDATE.lock stays by design: deleting a flock file reopens the race it guards. check("round left no temp file behind", - not any(p.name.startswith(".MANDATE.") for p in repo4.iterdir()) + not any(p.name.startswith(".MANDATE.") and p.name != ".MANDATE.lock" + for p in repo4.iterdir()) and not (repo4 / "MANDATE.md.tmp").exists()) # The counter must persist even when the key is missing — MANDATE.md is @@ -837,38 +839,8 @@ def init(repo, *extra): counts = sorted(kv(r.stdout).get("review_rounds_used", "") for r in results) check("and each caller was told a distinct round number", counts == [str(i) for i in range(1, 9)]) -check("the lock directory is cleaned up", - not (conc / ".MANDATE.lock").exists()) -# The CI flake: mkdir fails because another booker holds the lock, and that -# booker releases it before anyone looks. "File exists" is contention however -# the lock looks a moment later, so the booking waits its turn, never exit 4. -shim = Path(tempfile.mkdtemp()) -(shim / "mkdir").write_text( - '#!/bin/sh\n' - 'case "$1" in *.MANDATE.lock) if [ ! -e "$0.raced" ]; then : > "$0.raced"\n' - ' echo "mkdir: $1: File exists" >&2; exit 1; fi ;; esac\n' - 'exec /bin/mkdir "$@"\n') -(shim / "mkdir").chmod(0o755) -raced = make_repo() -init(raced, "task=raced", "review_budget=3") -r = subprocess.run(["bash", str(SCRIPT), "round", str(raced)], capture_output=True, text=True, - timeout=120, env={**ENV, "PATH": f"{shim}:{ENV.get('PATH', '')}"}) -check("a lock released between a failed mkdir and the check is contention, not exit 4", - r.returncode == 0 and (shim / "mkdir.raced").exists() - and kv(r.stdout).get("review_rounds_used") == "1") -stray = make_repo() -init(stray, "task=stray", "review_budget=3") -(stray / ".MANDATE.lock").write_text("") -t0 = time.monotonic() -r = run("round", str(stray)) -check("a file where the lock goes is refused at once, and named", - r.returncode == 4 and time.monotonic() - t0 < 3 and "not a lock directory" in r.stderr) -check("a held lock is refused rather than ignored", - (conc / ".MANDATE.lock").mkdir() or run("round", str(conc)).returncode == 4) -check("and the refusal names the lock", ".MANDATE.lock" in run("round", str(conc)).stderr) -(conc / ".MANDATE.lock").rmdir() -check("removing the stale lock unblocks the lane", - run("round", str(conc)).returncode == 0) +check("the lock is a plain file, never a directory", + (conc / ".MANDATE.lock").is_file()) check("the lock is covered by the git exclude", subprocess.run(["git", "-C", str(conc), "check-ignore", "-q", ".MANDATE.lock"]).returncode == 0) @@ -932,33 +904,106 @@ def init(repo, *extra): check(f"and allows refuses it: {bad_key}", run("allows", "commit", str(bd)).returncode == 2) -# --- an abandoned lock must not wedge the lane forever ----------------------- -# The traps do not survive kill -9, and the .MANDATE.* exclude hides the lock -# from `git status`, so nothing would point at the cause. +# --- the record lock: a flock, so a dead owner frees it -------------------- +# A mkdir lock outlived kill -9 and needed a "break it when the owner is dead" +# rule, which deleted a live booker's lock when the dead owner's lock was +# released and re-taken between the pid read and the rm. +import fcntl st = make_repo() init(st, "task=stale", "review_budget=5") lock = st / ".MANDATE.lock" -lock.mkdir() -(lock / "pid").write_text("999999\n") # a pid that cannot be running -r = run("round", str(st)) -check("a lock whose owner is gone is broken", r.returncode == 0) -check("and the round is actually booked", kv(r.stdout).get("round_authorized") == "yes") -check("the lock is released afterwards", not lock.exists()) -# A lock held by a LIVE process is respected, not broken. -import subprocess as _sp -lock.mkdir() -(lock / "pid").write_text(f"{os.getpid()}\n") +holder = subprocess.Popen( + ["bash", "-c", 'exec 9<>"$1"; python3 -c "import fcntl; fcntl.flock(9, fcntl.LOCK_EX)"; ' + 'echo locked; exec sleep 60', "_", str(lock)], stdout=subprocess.PIPE, text=True) +check("the holder took the lock", holder.stdout.readline().strip() == "locked") +t0 = time.monotonic() r = run("round", str(st)) check("a live holder is waited for, then reported", r.returncode == 4) -check("and the message names the recovery", ".MANDATE.lock" in r.stderr) +check("within the bounded wait", time.monotonic() - t0 < 30) +check("and the message names the lock", ".MANDATE.lock" in r.stderr) check("nothing was booked while blocked", - kv(run("show", str(st)).stdout).get("review_rounds_used") == "1") -import shutil as _sh -_sh.rmtree(lock) -# A lock with no pid file at all falls back to the age bound: recent = respected. + kv(run("show", str(st)).stdout).get("review_rounds_used") == "0") +holder.kill(); holder.wait() +r = run("round", str(st)) +check("a killed holder frees the lock with no staleness rule", r.returncode == 0) +check("and the round is actually booked", kv(r.stdout).get("review_rounds_used") == "1") +# A lock left by a script that already ended is just a file: no wait at all. +t0 = time.monotonic() +check("a leftover lock file does not block", + run("round", str(st)).returncode == 0 and time.monotonic() - t0 < 3) +# The lock must not leak into a child: one that outlives the script (a git +# daemon) would hold the lane's lock forever. `fixed` runs git under the lock. +lk_main = Path(tempfile.mkdtemp()) +for args in (["init", "-q", "-b", "main"], ["-c", "user.name=t", "-c", "user.email=t@t", + "commit", "-q", "--allow-empty", "-m", "base"]): + subprocess.run(["git", "-C", str(lk_main), *args], check=True) +lk = Path(tempfile.mkdtemp()) / "lane" +subprocess.run(["git", "-C", str(lk_main), "worktree", "add", "-q", "-b", "task/lk", str(lk)], + check=True) +init(lk, "task=lk", "review_budget=3") +(lk / "f.txt").write_text("x\n") +check("leak fixture: a review is on record", run("reviewed", str(lk)).returncode == 0) +shim = Path(tempfile.mkdtemp()) +# A subshell and `true`: a failed redirect on the special builtin `:` exits a +# POSIX sh (dash), killing the shim in exactly the case that should pass. +(shim / "git").write_text('#!/bin/sh\nif ( true >&9 ) 2>/dev/null; then : > "$0.leaked"; fi\n' + 'PATH="$REAL_PATH" exec git "$@"\n') +(shim / "git").chmod(0o755) +r = subprocess.run(["bash", str(SCRIPT), "fixed", "--after", "HEAD", str(lk)], + capture_output=True, text=True, timeout=120, + env={**ENV, "REAL_PATH": ENV.get("PATH", ""), + "PATH": f"{shim}:{ENV.get('PATH', '')}"}) +check("a locked write that runs git still works: " + r.stderr.strip()[:200], r.returncode == 0) +check("git never inherits the lock descriptor", not (shim / "git.leaked").exists()) +# What sits at the lock path is checked, not followed or waited on. +lock.unlink() lock.mkdir() -check("a fresh pidless lock is still respected", run("round", str(st)).returncode == 4) +(lock / "pid").write_text("999999\n") +r = run("round", str(st)) +check("an old mkdir lock directory is refused, not guessed at", + r.returncode == 4 and "older work-system" in r.stderr and "rm -rf" in r.stderr) +import shutil as _sh _sh.rmtree(lock) +target = st / "elsewhere" +lock.symlink_to(target) +r = run("round", str(st)) +check("a symlink at the lock path is refused", r.returncode == 4 and "not a lock file" in r.stderr) +check("and the open never followed it", not target.exists()) +lock.unlink() +os.mkfifo(lock) +t0 = time.monotonic() +r = run("round", str(st)) +check("a FIFO at the lock path is refused at once", + r.returncode == 4 and "not a lock file" in r.stderr and time.monotonic() - t0 < 3) +lock.unlink() +noshim = Path(tempfile.mkdtemp()) +(noshim / "python3").write_text("#!/bin/sh\nexit 127\n") +(noshim / "python3").chmod(0o755) +before = kv(run("show", str(st)).stdout).get("review_rounds_used") +r = subprocess.run(["bash", str(SCRIPT), "round", str(st)], capture_output=True, text=True, + timeout=120, env={**ENV, "PATH": f"{noshim}:{ENV.get('PATH', '')}"}) +check("without a working python3 the booking is refused, not unlocked", + r.returncode == 4 and "python3" in r.stderr) +check("and nothing was booked", + kv(run("show", str(st)).stdout).get("review_rounds_used") == before) +# A filesystem that refuses flock (ENOTSUP/ENOLCK) is not contention: the +# lock helper exits 2 with the error, and the message says so instead of "retry". +(noshim / "python3").write_text("#!/bin/sh\necho 'Operation not supported' >&2\nexit 2\n") +r = subprocess.run(["bash", str(SCRIPT), "round", str(st)], capture_output=True, text=True, + timeout=120, env={**ENV, "PATH": f"{noshim}:{ENV.get('PATH', '')}"}) +check("a refused flock is named, not reported as another writer", + r.returncode == 4 and "refused the lock" in r.stderr + and "Operation not supported" in r.stderr and "Retry" not in r.stderr) +# The lock file outlives the call, so every writer re-ensures its exclude, not +# only init (a hand-written record, or an init whose exclude write failed). +excl_path = Path(subprocess.run(["git", "-C", str(st), "rev-parse", "--path-format=absolute", + "--git-path", "info/exclude"], + capture_output=True, text=True).stdout.strip()) +excl_path.write_text("".join(l for l in excl_path.read_text().splitlines(True) + if l.strip() != "/.MANDATE.*")) +(st / ".MANDATE.lock").unlink(missing_ok=True) +check("round restores a missing lock exclude", run("round", str(st)).returncode == 0 + and subprocess.run(["git", "-C", str(st), "check-ignore", "-q", ".MANDATE.lock"]).returncode == 0) # --- init resolves the worker's capabilities itself -------------------------- # The skill used to build `--without local-review` in a variable and rely on the diff --git a/plugins/work-system/skills/close/SKILL.md b/plugins/work-system/skills/close/SKILL.md index 168da0fa..770fa3aa 100644 --- a/plugins/work-system/skills/close/SKILL.md +++ b/plugins/work-system/skills/close/SKILL.md @@ -939,7 +939,7 @@ no proof of origin, and a close is destructive (worktree removed, branch deleted itself proves nothing. But what a forged or mistaken request can *cost* is bounded by what the teardown can lose, and `auto` requires that to be nothing: the PR is merged (`assess` confirmed), the worktree holds nothing beyond the ephemeral - `TASK.md`/`MANDATE.md`/`.ws-kicker` — no modified, untracked **or gitignored** path (`--force` + `TASK.md`/`MANDATE.md`/`.ws-kicker` (and `mandate.sh`'s `.MANDATE.lock`) — no modified, untracked **or gitignored** path (`--force` deletes a gitignored `.env` too) — the local **and** remote branch tips **are** the merged PR's head (no commit after the merge, nothing unpushed, nothing pushed later that step 9 would delete), and at most one agent — the requester — lives anywhere in diff --git a/plugins/work-system/skills/continue/SKILL.md b/plugins/work-system/skills/continue/SKILL.md index f9cdcd8f..403bcb1d 100644 --- a/plugins/work-system/skills/continue/SKILL.md +++ b/plugins/work-system/skills/continue/SKILL.md @@ -356,7 +356,8 @@ the prefix-stripped task name) — comparing the raw argument instead misroutes. ``` Exit **0** → the round is booked; read `review_budget_exhausted` from its output. Exit **4** → the round could **not** be persisted (read-only tree, - no space): stop and report it — reviewing on an in-session count while the + no space, another writer holding the lock, no python3 — stderr names + which): stop and report it — reviewing on an in-session count while the file is not being written is how a resumed session restarts the budget, the exact thing the persisted counter exists to prevent. Exit **3** → no mandate on file (nothing to bound; ask per milestone as step 3 says).