diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 1cdeeb6..11d12a5 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -18,7 +18,7 @@ "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.0" + "version": "1.28.1" }, { "name": "pr-flow", diff --git a/.claude/knowledge/features/worker-autonomy-mandate.md b/.claude/knowledge/features/worker-autonomy-mandate.md index 9f430b0..e4fd925 100644 --- a/.claude/knowledge/features/worker-autonomy-mandate.md +++ b/.claude/knowledge/features/worker-autonomy-mandate.md @@ -1,9 +1,9 @@ --- title: "Worker Autonomy Mandate (MANDATE.md)" createdAt: 2026-09-07 -updatedAt: 2026-10-20 +updatedAt: 2026-10-22 createdFrom: "branch: task/extend-worker-autonomy" -updatedFrom: "branch: task/keep-review-coverage-across-version-rebase" +updatedFrom: "branch: task/harden-mandate-diff-id-and-round-lock" pluginVersion: 1.9.0 prime: false --- @@ -243,6 +243,17 @@ 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. + ## Indentation is structure too The same pass that taught `has-bot` to skip block scalars found the mandate @@ -402,12 +413,29 @@ covers. Why it is built this way: the global git rules fold fixes into earlier commits, so SHAs move. `git patch-id` was rejected in review: it ignores whitespace and (over `-U0`) position, so a de-indented `delete_all()` read as covered. The id hashes the - `-U3` diff against the newest merge-base with local/origin main, keeping - context, function context and whitespace and dropping only the `@@` line - numbers and `index` lines (keeping numbers would break coverage on every - rebase over a busy main); an edit on main inside a lane hunk's context - changes it (fail-closed re-review). Accepted residual: one change moved - between two blocks with identical context and enclosing function. + diff against the newest merge-base with local/origin main, keeping + function context and whitespace and dropping the `@@` line numbers and + `index` lines (keeping numbers would break coverage on every rebase over a + busy main). Since 1.28.1 it is a `-U0` diff: 1.28's `-U3` carried three of + MAIN's lines around each hunk, so #89's rebase over neighbouring CLAUDE.md + edits asked to re-review lines the lane never touched. `-U0` alone is enough + (stripping context after `-U3` would keep `-U3`'s hunk merging, which main + insertions between two close lane hunks change); inter-hunk context is + pinned to 0 so a user's config cannot pull main's lines back in. A hunk's + *place* = file, hunk order, and git's function-context line — the only + position left once line numbers go (they move on any main insertion above). + Trap: with no diff driver the function context is the nearest line above + starting with a letter, `_` or `$` — in a top-level shell script that is + often the adjacent line, so a main edit to it still fails closed. Accepted + residuals (named in review round 1): a lane line moved within one header's + region, past no other lane hunk, keeps the id (e.g. above a guard in the + same function; `-U3` caught that); and a main edit that changes what a + lane hunk means — deleting a guard right above it — keeps the id too. The + latter is the point of the change, and such a rebase usually stops on a + conflict a person resolves. Older records (`context` = 1.28's masked `-U3`, + `legacy` = unmasked `-U3`) are matched exactly by `lane_compat_ids`; the + tests recompute both from the old algorithms rather than through + mandate.sh, so a change to its forms cannot move the reference too. - **Version values are masked, nothing else** (`[mandate] version_paths`). #85 hit the gap: a rebase moved the lane's bump 1.25.0 → 1.26.0, the id changed, and the only way through was a bare `gh pr merge` around the gate — worse than @@ -421,10 +449,13 @@ covers. Why it is built this way: masked, and every other version value is prefixed with `=` first, so a lane writing the literal mask token `"*"` cannot pass as a masked renumber. Accepted residual: a deeper-nested `"version"` in those files is masked too. - Old unmasked records: `covered` also hashes the legacy form and accepts an - exact match (`diff_id_form=legacy`) — it can never cover more than the old id - did; with no manifest touched both forms are byte-identical (a test pins the - pre-masking pipeline's hash). + Old records: `covered` also hashes the older forms and accepts an exact + match — `context` (1.28.0's masked `-U3`) or `legacy` (unmasked `-U3`); it + can never cover more than the old id did. With no manifest touched the two + are the same text, so a 1.23-1.27 record then reports `context` (the label + names the form, not the version). The `-U3` forms keep the old argv, + unpinned: pinning the inter-hunk context there broke records made under a + user's non-default diff config (review round 1). - **A user ack, never a worker one** (`delta` + `acknowledged`). For a small real delta, `/merge`/`/land` show `delta` (a diff of the reviewed lane diff against the head's — so unrelated main edits show nothing) and ask. The ack diff --git a/CHANGELOG.md b/CHANGELOG.md index 1510f15..3def1cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,10 @@ entries are grouped per plugin, newest first. ## work-system +### 1.28.1 — 2026-10-22 +- The merge gate's lane diff id hashes only the lane's own lines: a `-U0` diff, whose place is the file, the hunk order and git's function context. A rebase over main edits next to a lane hunk keeps `covered=yes` (#89 asked to re-review lines the lane never touched). A lane line that changes or moves (under another header, or past another lane hunk) still flips it. A main edit to the hunk's function-context line still asks for a review. Accepted residuals: a lane line moved within one function past no other lane hunk keeps the id, and so does a main edit that changes what a lane hunk means (deleting a guard right above it). Reviews recorded by 1.28.0 (`diff_id_form=context`) or earlier (`legacy`; `context` when no manifest is touched, the same text) keep matching their unchanged lane, also under a non-default diff config, and `delta` still shows context. `delta`'s `diff_form=masked` is now `context-free` (or `context`/`legacy` for an older record). +- `mandate.sh round` (and the other record writers) no longer exit 4 when a concurrent booker releases the lock between a failed `mkdir` and the check: only a `mkdir` error other than "File exists" is a lock that cannot be created. The lock's pid is written atomically, so a partial pid never marks a live lock stale, and a file or symlink at the lock path exits 4 at once with a hint instead of waiting 5 s and blaming another process. + ### 1.28.0 — 2026-10-21 - The merge gate's lane diff id masks JSON `"version"` string values on `[mandate] version_paths` (`.work-system.toml` on the main branch; default: every `.claude-plugin/plugin.json` and the root `marketplace.json`; `[]` turns it off), so a rebase that only renumbers a plugin version keeps `covered=yes` (strict SemVer values only — any other value, the literal mask token included, still counts; a renumbered CHANGELOG heading is a delta to acknowledge). Only the policy committed on the main branch counts (origin's ref when there is one, never a working tree or another checked-out branch); a committed value that cannot be read masks nothing. Any other change in those files still needs a review. A review recorded before the masking keeps matching its unchanged lane (`diff_id_form=legacy`). A lane diff holding NUL bytes gets no id (never covered): bash drops NULs, so two contents would share one. - New `mandate.sh delta [--head ] [--max-lines ]` shows what changed since the review on record (a bounded diff of the two lane diffs), and `acknowledged --head --ack-id ` records the user's acknowledgment of that delta as `acked_diff`, `acked_by` and `acked_at`. `delta` shows versions unmasked and prints `ack_id=` (a hash of the shown text) only for a complete, exact, binary-free, readable delta, and the ack is refused once the diff is no longer the one shown. `covered` answers `via=user-ack` for exactly that diff, and a new review clears it. `no-review` stays unackable, and so does a delta measured from an uncommitted review (`approximate-baseline`); a legacy review's delta is shown unmasked. `/land` asks `[a]cknowledge / [s]top` on `changed-since-review`; only the user's own answer counts, and `/continue` never acknowledges. diff --git a/plugins/work-system/.claude-plugin/plugin.json b/plugins/work-system/.claude-plugin/plugin.json index b63b2e4..8eff56c 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.0", + "version": "1.28.1", "author": { "name": "gering" }, diff --git a/plugins/work-system/README.md b/plugins/work-system/README.md index 7cb3b44..cd3be6f 100644 --- a/plugins/work-system/README.md +++ b/plugins/work-system/README.md @@ -427,9 +427,13 @@ A lane is reviewed locally (`/swarm:review`, `/cycle`), not through GitHub appro so `/land` and `/pr-flow:merge` gate the merge on that review instead. A *finished* review records what it saw: `/swarm:review` (the whole lane, committed tree, or `--pr`) and `/cycle` call `mandate.sh reviewed --head` themselves, storing `reviewed_head` and the lane's diff id — a -hash of the lane's diff against its main branch with hunk line numbers dropped (function context kept), so a -rebase or squash keeps it while any change to the code, whitespace included, does -not. Booking a round (`round`) only spends budget; it records nothing. `mandate.sh +hash of the lane's own changed lines against its main branch (a `-U0` diff; no +line numbers, no context lines from main, function context kept), so a rebase or +squash keeps it, main edits right next to a lane hunk included, while any change +to the lane's code, whitespace included, does not. Moving a lane line under +another function-context header or past another lane hunk changes it too; +moving it within one function, or a main edit that changes what a lane hunk +means (a guard deleted right above it), does not. Booking a round (`round`) only spends budget; it records nothing. `mandate.sh fixed --after ` records the state after review ``'s agreed fixes — once per review, and refused (`reason=other-review`) when `` is not the review on record. swarm records only a review of a committed, clean tree, so commit before reviewing. diff --git a/plugins/work-system/scripts/mandate.sh b/plugins/work-system/scripts/mandate.sh index 3460935..898c27a 100755 --- a/plugins/work-system/scripts/mandate.sh +++ b/plugins/work-system/scripts/mandate.sh @@ -64,9 +64,11 @@ # Exit 0 covered=yes / 1 covered=no + reason= / # 3 no mandate. The merge gate of /land and /merge. # via=review|agreed-fixes|user-ack; the diff id -# masks manifest version values ([mandate] -# version_paths), and a review recorded before -# that masking still matches (diff_id_form=legacy). +# ignores main's context lines and masks manifest +# version values ([mandate] version_paths); a +# review recorded by an older work-system still +# matches its unchanged lane +# (diff_id_form=context|legacy). # delta [--head ] [--max-lines ] [] # What changed since the review on record, as a # diff of the two lane diffs (bounded, truncated=); @@ -434,14 +436,23 @@ lock_is_stale() { } acquire_round_lock() { - local lock="${MANDATE_PATH%/*}/.MANDATE.lock" i=0 broke=no - while ! mkdir "$lock" 2>/dev/null; do - # mkdir can fail for two very different reasons. If the lock is NOT there, - # the failure was 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. - if [ ! -d "$lock" ]; then - echo "${0##*/}: could not persist the consumed round — no lock could be created beside $MANDATE_PATH" >&2 + 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 @@ -458,7 +469,11 @@ acquire_round_lock() { fi sleep 0.1 done - printf '%s\n' "$$" > "$lock/pid" 2>/dev/null || true + # 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" } @@ -578,39 +593,54 @@ persist_keys() { # so the merge gate can tell whether the head being merged is the one a review # saw. Not a SHA or a tree: /merge rebases before it merges, and the global git # rules fold fixes into earlier commits — both rewrite every SHA and leave the -# change itself alone. The diff keeps its context lines, the hunks' function -# context and every whitespace byte (a re-indented or moved line changes what -# the code does, so it changes the id); only the `@@` line numbers and `index` -# lines are dropped, so a rebase over unrelated edits on main keeps the id. An -# edit on main within the context of a lane hunk changes it — a fail-closed -# re-review, never a pass. Residual: the same change moved between two blocks -# whose ±3 context lines and enclosing function are identical keeps the id. The -# pinned diff options keep a user's diff config from changing the id between -# the review and the merge. is a commit, or `worktree` for the working -# tree (tracked + untracked, minus the lane's record files and swarm's staging -# dirs), snapshotted through a throwaway index so the real one is never touched. -# Sets DIFF_ID; returns 1 when it cannot be computed (DIFF_ERR says why). -DIFF_ID=""; DIFF_ERR=""; SNAP_TARGET=""; DIFF_TXT=""; LEGACY_SAME="" +# change itself alone. The id hashes the lane's own lines only: a -U0 diff, so +# no context line from main is in it, and a rebase over main edits right next +# to a lane hunk keeps the id. Every `+`/`-` line keeps every whitespace byte (a +# re-indented line changes what the code does, so it changes the id). A hunk's +# PLACE is its file, its order among the file's hunks, and the function context +# git prints after its `@@` (the nearest line above it that the file's diff +# driver takes as a header; with none, a line starting with a letter, `_` or +# `$`): moving a lane line past another lane hunk or under another header +# changes the id. Line numbers are not part of it — any main insertion above +# would shift them. A main edit to that header line changes the id — a +# fail-closed re-review, never a pass. Residual: a lane line moved within the +# same header's region, past no other lane hunk, keeps the id. The pinned diff +# options keep a user's diff config from changing the id between the review and +# the merge. is a commit, or `worktree` for the working tree (tracked + +# untracked, minus the lane's record files and swarm's staging dirs), +# snapshotted through a throwaway index so the real one is never touched. Sets +# DIFF_ID, and clears the compat ids (lane_compat_ids); returns 1 +# when it cannot be computed (DIFF_ERR says why). +DIFF_ID=""; DIFF_ERR=""; SNAP_TARGET="" lane_diff_id() { + read_version_paths "$1" + lane_snapshot "$1" "$2" || return 1 + lane_form_text "$1" 0 || { lane_diff_err $?; return 1; } + DIFF_ID="$(hash_text "$1" "$FORM_TXT")" || { DIFF_ID=""; DIFF_ERR="git hash-object failed"; return 1; } + [ -n "$DIFF_ID" ] || { DIFF_ERR="empty hash"; return 1; } + DIFF_ID_CTX=""; DIFF_TXT_LEGACY=""; DIFF_ID_LEGACY="" +} + +# The masked lane diff at -U into FORM_TXT: the files on VERSION_PATHS +# go last (a second diff) with their version values masked. FORM_SPLIT=no when +# no lane file is on VERSION_PATHS — the masked text is then the unmasked one. +# Returns lane_diff_text's code. Needs lane_snapshot. +FORM_TXT=""; FORM_SPLIT="" +lane_form_text() { local dir="$1" p part1 part2="" local -a inc=() exc=() - read_version_paths "$dir" - lane_snapshot "$dir" "$2" || return 1 + DIFF_U="$2" for p in ${VERSION_PATHS[@]+"${VERSION_PATHS[@]}"}; do inc+=(":(top,glob)$p"); exc+=(":(top,exclude,glob)$p") done - part1="$(lane_diff_text "$dir" no ':(top)' ${exc[@]+"${exc[@]}"})" || { lane_diff_err $?; return 1; } + part1="$(lane_diff_text "$dir" no ':(top)' ${exc[@]+"${exc[@]}"})" || return $? part1="${part1%.}" if [ ${#inc[@]} -gt 0 ]; then - part2="$(lane_diff_text "$dir" yes "${inc[@]}")" || { lane_diff_err $?; return 1; } + part2="$(lane_diff_text "$dir" yes "${inc[@]}")" || return $? part2="${part2%.}" fi - # No lane file on VERSION_PATHS: the masked text IS the legacy text. - if [ -z "$part2" ]; then LEGACY_SAME=yes; else LEGACY_SAME=no; fi - DIFF_TXT="$part1$part2" - DIFF_ID="$(hash_text "$dir" "$DIFF_TXT")" || { DIFF_ID=""; DIFF_ERR="git hash-object failed"; return 1; } - [ -n "$DIFF_ID" ] || { DIFF_ERR="empty hash"; return 1; } - DIFF_TXT_LEGACY=""; DIFF_ID_LEGACY="" + if [ -z "$part2" ]; then FORM_SPLIT=no; else FORM_SPLIT=yes; fi + FORM_TXT="$part1$part2" } # The normalized diff for , masked when is yes, on stdout @@ -651,18 +681,27 @@ hash_text() { printf '%s' "$2" | git -C "$1" hash-object --stdin } -# The same snapshot hashed the way mandate.sh did before version masking: a -# review recorded by an older work-system holds this form, and an unchanged lane -# must keep matching it. Exact, so it can never cover more than the old id did. -# Call after lane_diff_id. Sets DIFF_TXT_LEGACY + DIFF_ID_LEGACY (empty on -# failure — a "no", never a pass). -DIFF_TXT_LEGACY=""; DIFF_ID_LEGACY="" -lane_legacy_id() { - if [ "$LEGACY_SAME" = yes ]; then - DIFF_TXT_LEGACY="$DIFF_TXT"; DIFF_ID_LEGACY="$DIFF_ID"; return 0 +# The same snapshot hashed the ways older work-system versions did, so a review +# recorded by one keeps matching its unchanged lane: DIFF_ID_CTX is the -U3 +# masked form (`context`, recorded by 1.28.0), DIFF_ID_LEGACY the -U3 unmasked +# one (`legacy`, 1.23-1.27). With no lane file on VERSION_PATHS the two are the +# same text, so a 1.23-1.27 record then reports `context`: the label names the +# form that matched, not the version that wrote it. Both are exact, so neither can cover more than its old id did; +# a recorded id in one of them still changes on a main edit next to a lane +# hunk — such a review is re-done once, then the context-free id applies. +# DIFF_TXT_LEGACY is also the text `delta` shows a person: with context, so the +# lines around a change can be read. Call after lane_diff_id. Empty on failure +# — a "no", never a pass. +DIFF_ID_CTX=""; DIFF_TXT_LEGACY=""; DIFF_ID_LEGACY="" +lane_compat_ids() { + DIFF_ID_CTX=""; DIFF_TXT_LEGACY=""; DIFF_ID_LEGACY="" + lane_form_text "$1" 3 || return 0 + DIFF_ID_CTX="$(hash_text "$1" "$FORM_TXT")" || DIFF_ID_CTX="" + if [ "$FORM_SPLIT" = no ]; then + DIFF_TXT_LEGACY="$FORM_TXT"; DIFF_ID_LEGACY="$DIFF_ID_CTX"; return 0 fi DIFF_TXT_LEGACY="$(lane_diff_text "$1" no ':(top)')" \ - || { DIFF_TXT_LEGACY=""; DIFF_ID_LEGACY=""; return 0; } + || { DIFF_TXT_LEGACY=""; return 0; } DIFF_TXT_LEGACY="${DIFF_TXT_LEGACY%.}" DIFF_ID_LEGACY="$(hash_text "$1" "$DIFF_TXT_LEGACY")" || DIFF_ID_LEGACY="" } @@ -721,14 +760,22 @@ lane_version_mask() { -e "/^(\\+\\+\\+|---) /!s/(\"version\"[[:space:]]*:[[:space:]]*)\"=$SEMVER_RE\"/\\1\"*\"/g" } -# The diff keeps its context lines, the hunks' function context and every -# whitespace byte; the pinned options keep a user's diff config from changing -# the id between the review and the merge. +# The diff at -U$DIFF_U (0 for the id, 3 for the compat forms and the delta +# text) keeps the hunks' function context and every whitespace byte; the +# pinned options keep a user's diff config from changing the id between the +# review and the merge. The -U0 id also pins the indent heuristic and a zero +# inter-hunk context (which would pull main's lines back in between two +# hunks). The -U3 forms keep the argv older versions ran, unpinned there, or +# they would stop matching a record made under a non-default diff config. +DIFF_U=0 lane_git_diff() { local dir="$1"; shift + local -a pin=() + [ "$DIFF_U" != 0 ] || pin=(--indent-heuristic --inter-hunk-context=0) git -C "$dir" -c diff.noprefix=false -c diff.mnemonicPrefix=false -c diff.relative=false \ diff --no-color --no-ext-diff --no-textconv --no-renames --binary --full-index \ - --diff-algorithm=myers -U3 --src-prefix=a/ --dst-prefix=b/ "$LANE_BASE" "$SNAP_TARGET" -- "$@" + --diff-algorithm=myers ${pin[@]+"${pin[@]}"} -U"$DIFF_U" \ + --src-prefix=a/ --dst-prefix=b/ "$LANE_BASE" "$SNAP_TARGET" -- "$@" } lane_diff_norm() { sed -e 's/^@@ -[0-9,]* +[0-9,]* @@/@@/' -e '/^index /d'; } @@ -904,9 +951,10 @@ do_covered() { lane_diff_id "${MANDATE_PATH%/*}" "$REV_COMMIT" || die "could not check coverage: $DIFF_ERR" printf 'diff_id=%s\n' "$DIFF_ID" covered_verdict "$DIFF_ID" && { emit_covered ""; exit 0; } - # The legacy form (lane_legacy_id reuses the masked one when they are equal). - lane_legacy_id "${MANDATE_PATH%/*}" - if [ "$LEGACY_SAME" != yes ] && covered_verdict "$DIFF_ID_LEGACY"; then emit_covered legacy; exit 0; fi + # A record from an older work-system, in the form it was hashed in. + lane_compat_ids "${MANDATE_PATH%/*}" + covered_verdict "$DIFF_ID_CTX" && { emit_covered context; exit 0; } + covered_verdict "$DIFF_ID_LEGACY" && { emit_covered legacy; exit 0; } printf 'covered=no\nreason=changed-since-review\n'; exit 1 } @@ -929,10 +977,11 @@ emit_covered() { } # The form the review on record was hashed in, rebuilt from reviewed_head — -# the one baseline `delta` and `acknowledged` compare against. masked or -# legacy when reviewed_head's lane diff, in that form, is exactly what the -# review hashed; `approximate` when neither is (the review saw an uncommitted -# tree), and the unmasked text then stands in: a delta from a baseline that is +# the one baseline `delta` and `acknowledged` compare against. +# context-free, context or legacy (lane_compat_ids) when reviewed_head's lane +# diff, in that form, is exactly what the review hashed; `approximate` when no +# form is (the review saw an uncommitted tree), and the unmasked text then +# stands in: a delta from a baseline that is # not the reviewed state can hide a change as easily as show one, so it is # shown but never acknowledged. Then computes in the same form — a legacy # review's delta must show the version renumbering its rule refuses. Sets @@ -942,21 +991,27 @@ REC_FORM=""; HEAD_ID=""; BASE_RAW=""; HEAD_RAW="" record_form() { local dir="$1" rev="$2" lane_diff_id "$dir" "$FM_reviewed_head" || die "could not compute the reviewed diff: $DIFF_ERR" - lane_legacy_id "$dir" + lane_compat_ids "$dir" [ -n "$DIFF_ID_LEGACY" ] || die "could not compute the reviewed diff" BASE_RAW="$DIFF_TXT_LEGACY" if [ "$DIFF_ID" = "$FM_reviewed_diff" ]; then - REC_FORM=masked + REC_FORM=context-free + elif [ "$DIFF_ID_CTX" = "$FM_reviewed_diff" ]; then + REC_FORM=context elif [ "$DIFF_ID_LEGACY" = "$FM_reviewed_diff" ]; then REC_FORM=legacy else REC_FORM=approximate fi lane_diff_id "$dir" "$rev" || die "could not compute the head's diff: $DIFF_ERR" - lane_legacy_id "$dir" + lane_compat_ids "$dir" [ -n "$DIFF_ID_LEGACY" ] || die "could not compute the head's diff" HEAD_RAW="$DIFF_TXT_LEGACY" - if [ "$REC_FORM" = masked ]; then HEAD_ID="$DIFF_ID"; else HEAD_ID="$DIFF_ID_LEGACY"; fi + case "$REC_FORM" in + context-free) HEAD_ID="$DIFF_ID" ;; + context) HEAD_ID="$DIFF_ID_CTX" ;; + *) HEAD_ID="$DIFF_ID_LEGACY" ;; + esac } # `delta`: what changed in the lane between the review on record and — @@ -968,8 +1023,8 @@ record_form() { # unreadable= (then the text follows escaped), truncated=, ack_id= (set only # for a complete, exact, binary-free, readable delta: a hash of the shown # text), fence=. baseline=exact|approximate and diff_form= -# masked|legacy|approximate (record_form). Agreed fixes recorded after the -# review are inside the delta (includes_agreed_fixes=yes). Key=value lines, +# context-free|context|legacy|approximate (record_form). Agreed fixes recorded +# after the review are inside the delta (includes_agreed_fixes=yes). Key=value lines, # then a `delta_begin` line, then the text. Exit 1 + reason= when there is # nothing to compare against: no-review, or no-baseline (reviewed_head is no # commit here any more). A failed diff is exit 2, never an empty delta. diff --git a/plugins/work-system/scripts/test_mandate.py b/plugins/work-system/scripts/test_mandate.py index 1eb6ec7..a937b3d 100644 --- a/plugins/work-system/scripts/test_mandate.py +++ b/plugins/work-system/scripts/test_mandate.py @@ -26,6 +26,7 @@ import subprocess import sys import tempfile +import time from pathlib import Path HERE = Path(__file__).parent @@ -41,6 +42,13 @@ def check(name, cond): FAILS.append(name) +# A CI runner has no git identity, and a local global one hides every call +# that needs it (a rebase, an amend). Set once, for every git this process and +# the script under test start. +for _k in ("GIT_AUTHOR", "GIT_COMMITTER"): + os.environ[f"{_k}_NAME"] = "t" + os.environ[f"{_k}_EMAIL"] = "t@t" + # An inherited SETTINGS_PROJECT_ROOT would point every budget case at the # caller's repo; the one test that wants it sets it here, then removes it. ENV = {k: v for k, v in os.environ.items() if k != "SETTINGS_PROJECT_ROOT"} @@ -467,8 +475,10 @@ def init(repo, *extra): mode = ro.stat().st_mode os.chmod(ro, 0o555) try: + t0 = time.monotonic() r = run("round", str(ro)) check("an unpersistable round exits non-zero", r.returncode == 4) + check("...at once, not after the contention wait", time.monotonic() - t0 < 3) check("an unpersistable round reports no counters", "review_rounds_used=" not in r.stdout) check("an unpersistable round says so", "could not persist" in r.stderr) @@ -829,6 +839,30 @@ def init(repo, *extra): 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) @@ -1524,13 +1558,113 @@ def vcov(lane, *extra): check("version_paths: read from the main checkout, never the lane", vcov(lane)[1].get("reason") == "changed-since-review") -# A review recorded before the masking existed (the legacy, unmasked id) keeps -# matching the unchanged lane it was recorded for. -m, lane = version_lane("[mandate]\nversion_paths = []\n") +# The id hashes the lane's own lines, not main's context around them (#89: a +# rebase over main edits to the lines next to a lane hunk asked for a review of +# lines the lane never touched). +def doc(b="v1", lane_line="- **c** (v1)", top="", extra=""): + return (f"Plugins are listed below.\n{top}\n- **a** (v1)\n- **b** ({b})\n{lane_line}\n" + f"- **d** ({b})\n- **e** (v1)\n\nSecond section\n\n- x\n- y\n{extra}- z\n") + + +cm = Path(tempfile.mkdtemp()) +git(cm, "init", "-q", "-b", "main") +(cm / "doc.md").write_text(doc()) +commit_all(cm, "base") +cl = Path(tempfile.mkdtemp()) / "lane" +git(cm, "worktree", "add", "-q", "-b", "task/c", str(cl)) +run("init", str(cl), f"allow={BASE_ALLOW}", "authorized_by=user", "task=c") +(cl / "doc.md").write_text(doc(lane_line="- **c** (v2, the lane)")) +commit_all(cl, "lane") +run("reviewed", "--head", "HEAD", str(cl)) +(cm / "doc.md").write_text(doc(b="v9", top="- **0** (new on main)")) +commit_all(cm, "main edits the lines next to the lane hunk, and above it") +git(cl, "reset", "-q", "--hard", "main") +(cl / "doc.md").write_text(doc(b="v9", top="- **0** (new on main)", lane_line="- **c** (v2, the lane)")) +commit_all(cl, "lane, rebased") +rc, c = vcov(cl) +check("context-free: main's edits next to a lane hunk keep the coverage", + rc == 0 and c.get("via") == "review" and "diff_id_form" not in c) +r = run("delta", str(cl)) +body = r.stdout.split("delta_begin\n", 1)[1] +check("context-free: the delta still shows main's changed neighbours to a person", + r.returncode == 0 and "(v9)" in body and "the lane" in body) +lane_head = git(cl, "rev-parse", "HEAD") +(cm / "doc.md").write_text((cm / "doc.md").read_text().replace("Plugins are listed", "Plugins are kept")) +commit_all(cm, "main edits the hunk's header line") +git(cl, "rebase", "-q", "main") +check("context-free: a main edit to the hunk's header line fails closed", + vcov(cl)[1].get("reason") == "changed-since-review") +git(cl, "reset", "-q", "--hard", lane_head) +(cl / "doc.md").write_text(doc(b="v9", top="- **0** (new on main)", lane_line="- **c** (v3, the lane)")) +commit_all(cl, "the lane changes its own line") +check("context-free: the lane changing its own line is not covered", + vcov(cl)[1].get("reason") == "changed-since-review") +(cl / "doc.md").write_text(doc(b="v9", top="- **0** (new on main)", extra="- **c** (v2, the lane)\n") + .replace("- **c** (v1)\n", "", 1)) +commit_all(cl, "the lane moves its hunk under another header") +check("context-free: the lane moving its hunk is not covered", + vcov(cl)[1].get("reason") == "changed-since-review") +# A move that stays under the same header but passes another lane hunk. +(cl / "doc.md").write_text(doc(b="v9", top="- **0** (new on main)", lane_line="- **c** (v2, the lane)") + .replace("- **e** (v1)", "- **e** (lane too)")) +commit_all(cl, "a second lane hunk below") +run("reviewed", "--head", "HEAD", str(cl)) +(cl / "doc.md").write_text(doc(b="v9", top="- **0** (new on main)") + .replace("- **e** (v1)", "- **e** (lane too)\n- **c** (v2, the lane)") + .replace("- **c** (v1)\n", "", 1)) +commit_all(cl, "the first lane hunk now follows the second") +check("context-free: a lane hunk moved past another one is not covered", + vcov(cl)[1].get("reason") == "changed-since-review") + +# Reviews recorded by older work-system versions, in the form each hashed: +# `legacy` (1.23-1.27: the -U3 diff) and `context` (1.28: the -U3 diff with +# manifest versions masked). Recomputed here from the old algorithms, so a +# change to mandate.sh's own forms cannot move the reference along with it. +OLD_DIFF = ["-c", "diff.noprefix=false", "-c", "diff.mnemonicPrefix=false", "-c", + "diff.relative=false", "diff", "--no-color", "--no-ext-diff", "--no-textconv", + "--no-renames", "--binary", "--full-index", "--diff-algorithm=myers", "-U3", + "--src-prefix=a/", "--dst-prefix=b/"] +OLD_VERSION_PATHS = [":(top,glob)**/.claude-plugin/plugin.json", + ":(top,glob).claude-plugin/marketplace.json"] + + +def old_diff(lane, *spec, mask=False): + base = git(lane, "merge-base", "HEAD", "main") + raw = subprocess.run(["git", *OLD_DIFF, base, "HEAD", "--", *spec], cwd=lane, + capture_output=True, check=True).stdout + raw = re.sub(rb"(?m)^index .*\n", b"", raw) + raw = re.sub(rb"(?m)^@@ -[0-9,]* \+[0-9,]* @@", b"@@", raw) + if mask: + raw = b"".join( + ln if ln.startswith((b"+++ ", b"--- ")) else + re.sub(rb'("version"\s*:\s*)"=[0-9]+\.[0-9]+\.[0-9]+"', rb'\1"*"', + re.sub(rb'("version"\s*:\s*)"', rb'\1"=', ln)) + for ln in raw.splitlines(keepends=True)) + return raw + + +def old_id(lane, form): + """The diff id work-system 1.23-1.27 (legacy) or 1.28 (context) recorded for HEAD.""" + if form == "legacy": + text = old_diff(lane, ":(top)") + else: + text = (old_diff(lane, ":(top)", *[":(top,exclude," + p[6:] for p in OLD_VERSION_PATHS]) + + old_diff(lane, *OLD_VERSION_PATHS, mask=True)) + return subprocess.run(["git", "hash-object", "--stdin"], cwd=lane, input=text, + capture_output=True, check=True).stdout.decode().strip() + + +def old_review(lane, form): + """A review of HEAD as an older work-system recorded it.""" + run("reviewed", "--head", "HEAD", str(lane)) + f = lane / "MANDATE.md" + f.write_text(re.sub(r"(?m)^reviewed_diff: .*$", "reviewed_diff: " + old_id(lane, form), + f.read_text(), count=1)) + + +m, lane = version_lane() lane_change(lane, "1.2.0") -run("reviewed", "--head", "HEAD", str(lane)) -(m / ".work-system.toml").unlink() -commit_all(m, "drop the policy: the default applies") +old_review(lane, "legacy") rc, c = vcov(lane) check("legacy: an old unmasked review still covers its unchanged lane", rc == 0 and c.get("via") == "review" and c.get("diff_id_form") == "legacy") @@ -1546,23 +1680,50 @@ def vcov(lane, *extra): ack(lane, "HEAD").returncode == 0) check("legacy: ...and the ack covers exactly it", vcov(lane)[1].get("via") == "user-ack") -# The legacy form is the id older work-system versions computed, byte for byte. +# A 1.28 review (masked, with context) keeps what 1.28 granted it: its +# unchanged lane, and a version-only rebase. m, lane = version_lane() -(lane / "code.sh").write_text((lane / "code.sh").read_text().replace("echo 5\n", "echo five\n")) -commit_all(lane, "code only") -rid = kv(run("reviewed", "--head", "HEAD", str(lane)).stdout).get("reviewed_diff") -def pre_masking_id(lane): - """The diff id work-system 1.23-1.27 computed, piped straight into hash-object.""" - return subprocess.run( - "set -o pipefail; git -c diff.noprefix=false -c diff.mnemonicPrefix=false -c diff.relative=false " - "diff --no-color --no-ext-diff --no-textconv --no-renames --binary --full-index " - "--diff-algorithm=myers -U3 --src-prefix=a/ --dst-prefix=b/ \"$(git merge-base HEAD main)\" HEAD " - "| sed -e 's/^@@ -[0-9,]* +[0-9,]* @@/@@/' -e '/^index /d' | git hash-object --stdin", - shell=True, cwd=lane, capture_output=True, text=True, executable="/bin/bash").stdout.strip() +lane_change(lane, "1.2.0") +old_review(lane, "context") +rc, c = vcov(lane) +check("context: a 1.28 review still covers its unchanged lane", + rc == 0 and c.get("via") == "review" and c.get("diff_id_form") == "context") +main_ships(m, lane, "1.2.0") +lane_change(lane, "1.3.0") +rc, c = vcov(lane) +check("context: ...and its version-only rebase", + rc == 0 and c.get("via") == "review" and c.get("diff_id_form") == "context") +check("context: the delta is shown in that form", + kv(run("delta", str(lane)).stdout.split("delta_begin\n")[0]).get("diff_form") == "context") +(lane / "code.sh").write_text((lane / "code.sh").read_text().replace("echo 9\n", "echo nine\n")) +commit_all(lane, "a lane line") +check("context: ...but not a lane change after it", + vcov(lane)[1].get("reason") == "changed-since-review") +check("context: the user can ack that delta", ack(lane, "HEAD").returncode == 0) +check("context: ...and the ack covers exactly it", vcov(lane)[1].get("via") == "user-ack") +# Old versions ran the -U3 diff under the user's diff config; the compat forms +# must too, or an unchanged lane stops matching its record. +m, lane = version_lane() +git(lane, "config", "diff.interHunkContext", "10") +(lane / "code.sh").write_text((lane / "code.sh").read_text() + .replace("echo 3\n", "echo three\n").replace("echo 15\n", "echo fifteen\n")) +commit_all(lane, "two hunks a non-default inter-hunk context merges") +old_review(lane, "legacy") +rc, c = vcov(lane) +check("legacy: a record made under a non-default diff config still covers", + rc == 0 and c.get("via") == "review") +git(lane, "config", "--unset", "diff.interHunkContext") -old = pre_masking_id(lane) -check("legacy: with no manifest touched the id equals the pre-masking algorithm's", rid == old and rid) +# With no manifest touched, 1.27 and 1.28 hashed the same text. +m, lane = version_lane() +(lane / "code.sh").write_text((lane / "code.sh").read_text().replace("echo 5\n", "echo five\n")) +commit_all(lane, "code only") +old_review(lane, "legacy") +rc, c = vcov(lane) +check("legacy: with no manifest touched an old review covers via the -U3 form", + rc == 0 and old_id(lane, "legacy") == old_id(lane, "context") + and c.get("diff_id_form") == "context") # The ack binds the diff the user was shown: a head that moved since is refused, # and so is a delta no one can read (a binary patch). @@ -1602,11 +1763,10 @@ def pre_masking_id(lane): r = run("acknowledged", "--head", "HEAD", "--ack-id", rid or "0" * 40, str(lane)) check("acknowledged: ...and cannot be acked even with the right id", r.returncode == 1 and kv(r.stdout).get("reason") == "binary-delta") -old = pre_masking_id(lane) -(m / ".work-system.toml").write_text("[mandate]\nversion_paths = []\n") -commit_all(m, "no masking") -rid = kv(run("reviewed", "--head", "HEAD", str(lane)).stdout).get("reviewed_diff") -check("legacy: a diff ending in a binary patch keeps the pre-masking id", rid and rid == old) +old_review(lane, "legacy") +rc, c = vcov(lane) +check("legacy: a diff ending in a binary patch keeps its old id", + rc == 0 and c.get("via") == "review" and c.get("diff_id_form") in ("context", "legacy")) # A working-tree review whose tree is not its HEAD: the delta is approximate, # so it is shown but cannot be acknowledged.