From 48064dd53f8de28413bd388af1b5ad96cd3f3f74 Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Wed, 30 Sep 2026 17:13:46 -0700 Subject: [PATCH 1/9] chore: start #4155 claim-required hook Co-Authored-By: Claude Sonnet 5.5 From d7d1c4041197bd87eb2a17d820ff0724f9d443d7 Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Wed, 30 Sep 2026 17:32:27 -0700 Subject: [PATCH 2/9] feat: require a session claim before commit/push on a PR branch - closes #4155 Add hooks/no-pr-work-without-claim.py (PreToolUse, Bash). On a branch with an open Morrison-Lab PR it denies `git commit` / `git push` unless a claim comment (agent marker plus hold-off wording) names this session by session id or worktree path, and denies when another session's claim is newer. Before a push it warns on pushes since the claim that local history lacks, read from the forge activity endpoint. Fails open with a visible warning on any gh/network failure. Escape hatch: ALLOW_UNCLAIMED_PR_WORK=1. Adds 37 cases in hooks/test-no-pr-work-without-claim.py (8 of 8 mutants killed), registers the hook in hooks/hooks.json and the generated plugin mirror, documents it in README.md, and adds a Do/Don't pair to shared/workflow/claim-pr.md citing #4155 and PR #4134. Local half only: hooks are inert in remote/web sessions (#2004). Co-Authored-By: Claude Sonnet 5.5 --- README.md | 2 + hooks/hooks.json | 7 + hooks/no-pr-work-without-claim.py | 349 +++++++++++++++++++++++ hooks/test-no-pr-work-without-claim.py | 244 ++++++++++++++++ plugins/ai-config-hooks/hooks/hooks.json | 7 + shared/workflow/claim-pr.md | 18 ++ 6 files changed, 627 insertions(+) create mode 100644 hooks/no-pr-work-without-claim.py create mode 100644 hooks/test-no-pr-work-without-claim.py diff --git a/README.md b/README.md index e47f0ab29..3d11725c7 100644 --- a/README.md +++ b/README.md @@ -292,6 +292,7 @@ Settings this repo's own tooling reads from the environment. | `AI_CONFIG_PR_REVIEWERS` | Comma-separated GitHub logins to request as reviewers on a PR the orchestrator opens. **Unset means no reviewer is requested**, which is deliberate: this repo is used by people other than its author, so there is no login that could be a correct default. Before this existed the value was hardcoded, and every request named a login that exists for nobody (ai-config#2627). | | `AI_CONFIG_DOTFILES_FORCE` | Install dotfiles on a machine that fails the environment gate --- see [`dotfiles/shiva/README.md`](dotfiles/shiva/README.md). | | `ALLOW_FORCE_PUSH` | Escape valve for `hooks/no-clobbering-push.py`, for a case the guard did not foresee. Using it means stating why. | +| `ALLOW_UNCLAIMED_PR_WORK` | Escape valve for `hooks/no-pr-work-without-claim.py`, which otherwise denies a `git commit` or `git push` on a branch with an open Morrison-Lab PR that has no claim comment naming this session. Using it means saying why. | | `ALLOW_COMMIT_AND_PUSH` | Escape valve for `hooks/no-commit-chained-to-push.py`, which otherwise refuses a `git commit` and a `git push` in one Bash call. Using it means saying why the call could not be split. | | `ALLOW_BREAKING_SLIDE` | Escape valve for `hooks/guard-slide-major-tag.py`, recording a deliberate override when a reusable workflow permission addition has already been prepared in consumers. | @@ -501,6 +502,7 @@ The payload gaps that remain and the per-guard status are in | `flag-stale-adjacent-comment.py` | `PreToolUse` (Bash) | warns, never blocks, when a `git commit` changes a literal value while an unchanged comment within ten lines still asserts the old one | | `warn-unparseable-staged-config.py` | `PreToolUse` (Bash) | warns when `git commit` would commit staged `.toml`, `.json`, or `.yaml`/`.yml` config files that fail parser validation (`tomllib`, `json`, `yaml.safe_load`) | | `no-delete-branch-under-stacked-pr.py` | `PreToolUse` (Bash) | warns when `gh pr merge --delete-branch` or `gh pr close --delete-branch` would delete a branch that is an open PR's base. GitHub's documented behaviour is to retarget such a PR, but a measured case closed it instead, and a closed PR can be neither retargeted nor reopened while its base is gone. Silent when nothing is stacked, when the query fails or returns an unexpected shape, when `gh` is absent, when the command carries no `-R` or PR target, and on `--delete-branch=false` | +| `no-pr-work-without-claim.py` | `PreToolUse` (Bash) | denies a `git commit` or `git push` on a branch whose open Morrison-Lab PR has no claim comment (agent marker plus hold-off wording) naming this session by session id or worktree path, and denies when another session's claim is newer than yours. Before a push it warns, never denies, about pushes since your claim that local history lacks, read from the forge activity endpoint. Fails open and says so on any `gh` or network failure. Local half of ai-config#4155: hooks are inert in remote and web sessions. | | `no-clobbering-push.py` | `PreToolUse` (Bash) | refuses a bare `git push --force`/`-f`, whose remedy (`--force-with-lease --force-if-includes`) costs one word. Warns on every other push whose remote tip a live, read-only `git ls-remote` shows is not an ancestor of the ref being pushed (which is `HEAD` only when the refspec says so, resolved in the directory the push runs in rather than the session's -- a `cd`, scoped to its subshell but not to a brace group, and declined where a compound statement's body, a short-circuited alternative, or a fork into a background job or a pipeline means the pushing shell never takes its effect, then the push's own `-C`, declined in turn when the shell would have had to expand it), names that directory and qualifies its remediation commands with `git -C` when it is not the call's own, declines the reading when the directory is indeterminate or `--git-dir`/`--work-tree`/`GIT_DIR=` redirected the repository, and stays silent on a fast-forward | | `no-commit-chained-to-push.py` | `PreToolUse` (Bash) | denies a Bash call that chains a `git commit` into a later `git push`. A PreToolUse deny rejects the whole invocation, so a guard refusing the push discards the commit too while its message speaks only about the push (ai-config#2992). Denies rather than warns because the refusal stops the chain reaching the sibling guards at all, and its remedy -- two Bash calls -- is always available. Clearable with `ALLOW_COMMIT_AND_PUSH=1`, either prefixing the commit or push or as the call's own leading assignment (a subshell or short-circuited one sets nothing and does not count). Matches over an argv split (`scripts/lib/shellcmd.py`), so a quoted commit message, a heredoc body and `git commit-tree` cannot trip it, while `timeout 60 git push`, `/usr/bin/git push` and `{ git commit; } && git push` all resolve -- the guard has to fire wherever its siblings would. There is no exemption for a `--dry-run` or `--delete` command: one was written and removed after a review measured `git commit ... && git push --force --delete` and `... --dry-run --no-dry-run --force` both going silent while `no-clobbering-push.py` denied them | | `flag-chained-push.py` | `PreToolUse` (Bash) | warns, never blocks, when a `git push` is chained after another command with `&&`, `;`, or `\|\|`, piped onward with `\|`, or suffixed by a redirection (`>`, `>>`, or an fd form like `2>&1`). `no-clobbering-push.py` and the plugin's own `no-push-without-self-review.py` push policy both parse the WHOLE command text for a push rather than the isolated segment, so a trailing `2>&1` hands either parser a bare `2` sitting where a commit-ish token would sit in other shapes, and a chained prefix reads as part of the same invocation; a refused chain runs NOTHING, which the refusal naming only the push invites the author to misread as the prefix having succeeded. Measured on Lacaedemon/sparta, 2026-09-05: three refusals in one session | diff --git a/hooks/hooks.json b/hooks/hooks.json index bdd4cdc8c..439a4f44a 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -451,6 +451,13 @@ "script": "no-commit-chained-to-push.py", "why": "shared/workflow/check-before-pushing.md, plus ai-config#2992, which is a REPORT rather than a measurement -- its author states they verified the mechanism from the hook registration and did not reproduce the lost commit. A PreToolUse deny rejects the WHOLE Bash invocation, so a call that chains `git commit` into `git push` loses the commit when any guard refuses the push -- and no-push-without-self-review.py and no-clobbering-push.py are both registered PreToolUse on Bash. Their refusal names only the push, so it reads as \"the push was blocked\" while the change is still an uncommitted working-tree edit. Reported 2026-09-02, caught only because an adversarial reviewer checked whether HEAD had moved. Note that on a HEREDOC commit neither sibling currently sees a push at all (they carry the #2993 splitter defect this hook's library fixes), so for that shape today nothing would be lost -- a reason to fix #2993, not to exempt the shape, and the deny message is worded so it never asserts a sibling would have refused a particular call. DENIES rather than warns, unlike its warn-dupe-check-chained-to-create.py sibling of the same shape: an advisory additionalContext is attached while the call proceeds, so a sibling's deny in the same pass still discards the commit, and whether the author is even TOLD depends on unverified harness behaviour (does a non-denying hook's context survive a sibling's deny -- see the hook docstring for the ten-minute experiment that settles it). The refusal does not depend on that answer: it stops the chain reaching the siblings at all. The refusal is always satisfiable (issue the same two commands as two calls) and clearable with an `ALLOW_COMMIT_AND_PUSH=1` env-assignment prefix. Matches over an argv split (scripts/lib/shellcmd.py) rather than the raw string, so a quoted commit message, a heredoc body, and `git commit-tree`/`git commit-graph` cannot trip it. Requires commit BEFORE push: push-then-commit loses nothing. No exemption for a --dry-run or --delete command: one was written on review advice and removed when a second review measured `git push --force --delete` and `--dry-run --no-dry-run --force` going silent here while no-clobbering-push.py denied both -- a refused dry-run costs one tool call, a missed force-delete costs the commit. Fails open. Tracked as #2992; the shared splitter's heredoc defect and the eight unmigrated copies are #2993 (eight, not seven: derive the set from `grep -rlF '_SHELL_OPS = set(\"();|&\")' hooks/`, since remind-ci-crosscheck-sim-verdict.py spells its copy without the leading underscore)." }, + { + "type": "command", + "command": "python3 \"${CLAUDE_PLUGIN_ROOT}/hooks/no-pr-work-without-claim.py\"", + "timeout": 30, + "script": "no-pr-work-without-claim.py", + "why": "ai-config#4155, measured 2026-09-30: two agent sessions worked the same three PR branches at once and neither posted a claim, so a cloud session's restore commit on PR #4134 left memories/preferences.md at 706 lines instead of 1177. shared/workflow/claim-pr.md says to claim first and nothing enforced it; ListAgents cannot see a cloud session. DENIES a `git commit` or `git push` on a branch with an open Morrison-Lab PR unless a claim comment (agent marker plus hold-off wording) names THIS session by session id or worktree path, and denies when another session's claim is newer than yours. Before a push it WARNS (never denies) about pushes since your claim that are not in local history, read from the forge activity endpoint. Fails OPEN and says so on any network or gh failure. Local half only: hooks are inert in remote/web sessions (#2004). Clear with ALLOW_UNCLAIMED_PR_WORK=1. Timeout is 30s because it makes up to three forge reads." + }, { "type": "command", "command": "python3 \"${CLAUDE_PLUGIN_ROOT}/hooks/flag-chained-push.py\"", diff --git a/hooks/no-pr-work-without-claim.py b/hooks/no-pr-work-without-claim.py new file mode 100644 index 000000000..1335f4733 --- /dev/null +++ b/hooks/no-pr-work-without-claim.py @@ -0,0 +1,349 @@ +#!/usr/bin/env python3 +"""PreToolUse guard: `git commit` / `git push` on a PR branch needs this session's claim. + +## The gap this closes + +On 2026-09-30 two agent sessions worked the same three PR branches at once and +neither posted a claim comment (ai-config#4155). A cloud session's "restore" +commit on PR #4134 left `memories/preferences.md` at 706 lines instead of 1177. +`shared/workflow/claim-pr.md` already says to claim before working; nothing +enforced it, and `ListAgents` cannot see a cloud session, so the local session +had no other way to find the peer. A rule is read at planning time and skipped +at commit time, so the check moves onto the commit itself. + +## What it does + +Matches a `git commit` or `git push` (over an argv split from +`scripts/lib/shellcmd.py`, so a quoted message or a heredoc body naming either +command cannot trip it; `--dry-run` is skipped, as is a `--delete` push). + +When the checkout's branch has an OPEN PR in a Morrison-Lab repo it reads the +PR's issue comments and DENIES unless one is a claim from THIS session: + + * a claim is a comment carrying the agent marker (`Posted by Claude Code (AI + agent)`) and the `hold off` (or legacy `paws off`) wording, per + `skills/claim-pr/SKILL.md`; + * it is THIS session's when its body contains the payload's `session_id` or + this worktree's path. claim-pr's stock wording carries neither -- the + forge login is shared by every session under one account, so the comment + must say which session it is. The deny text gives the one-line addition. + * a claim from a DIFFERENT session posted AFTER this session's latest claim + is a takeover: DENY. (A newer claim that names no session at all is not + attributable, so it only warns.) + +Before a PUSH it also reads the forge activity endpoint +(`repos///activity?ref=refs/heads/`) and WARNS (never denies) +when a push since this session's claim landed a commit that is not an ancestor +of local HEAD: that is a push this session neither made nor merged, which is +what the 2026-09-30 collision looked like from the forge side. + +## What it does not do + +It cannot see the cloud half: hooks are inert in remote and web sessions +(ai-config#2004), so a cloud session is only covered by its own instructions or +a server-side check. This is the local half of #4155 only. + +A claim's 2-hour expiry (claim-pr.md) is not evaluated. An own claim is +accepted whatever its age; staleness over-approximation via `updatedAt` would +mark every session's own pushes as fresh activity. Known limit. + +## Failure policy + +Fails OPEN on any parse trouble, outside a git repo, off a Morrison-Lab remote, +with no open PR, when `gh` is missing, on a network error or timeout -- and in +every network-failure case says so in `additionalContext` and on stderr rather +than going quiet, because a silent open is indistinguishable from a guard that +never ran (`shared/principles/fail-fast.md`). + +## The override + +`ALLOW_UNCLAIMED_PR_WORK=1`, as an env prefix on the commit or push, as the +call's leading `export`, or in the process environment. It is for a case this +guard did not foresee; reaching for it means saying why. + +`PR_CLAIM_GH_CMD` replaces the `gh` executable (shlex-split); tests use it. +""" +from __future__ import annotations + +import json +import os +import re +import shlex +import subprocess +import sys + +OVERRIDE = "ALLOW_UNCLAIMED_PR_WORK" +OWNERS = {"morrison-lab"} +AGENT_MARKER = "posted by claude code (ai agent)" +CLAIM_PHRASES = ("hold off", "paws off") +NET_TIMEOUT = 8 +SKIP_BRANCHES = {"HEAD", "main", "master"} + +try: + _LIB = os.path.join( + os.path.dirname(os.path.dirname(os.path.realpath(__file__))), + "scripts", "lib") + if _LIB not in sys.path: + sys.path.insert(0, _LIB) + from shellcmd import env_value, git_subcommand, simple_commands +except Exception as _exc: # broken install: fail open, and say so + print(f"no-pr-work-without-claim: cannot load scripts/lib/shellcmd.py " + f"({_exc}); not evaluating", file=sys.stderr) + env_value = git_subcommand = simple_commands = None + +LEADING_OVERRIDE = re.compile( + r"\A[ \t]*(?:export[ \t]+)?" + OVERRIDE + r"=1[ \t]*(?:;|&&|\|\||\r?\n|\Z)") + +DENY_NO_CLAIM = """\ +`git {verb}` on branch `{branch}`, which has open PR {pr_url}, but that PR has +no claim comment from THIS session. + +Two sessions on one branch is how #4134 lost 471 lines of memories/preferences.md +on 2026-09-30 (ai-config#4155). Post a claim first, naming this session so a +second session under the same login can tell the two apart: + + Claude Code CLI (local session) is working on this --- please hold off on pushing to this branch until I'm done. + + Session worktree: `{worktree}` Session id: `{session_id}` + + _Posted by Claude Code (AI agent) --- not written by a human._ + +Post it with `gh pr comment {pr_number} --body-file `, then retry. If a +claim from another session is already live on the PR, do not post over it: +that session is working this branch, and the second one stands down. + +{override}=1 clears this refusal (env prefix on the command, a leading +`export`, or the process environment) -- for a case this guard did not foresee, +and say why. +""" + +DENY_SUPERSEDED = """\ +`git {verb}` on branch `{branch}` ({pr_url}): another session claimed this PR +after you did. + + your latest claim: {mine_at} + their claim: {theirs_url} ({theirs_at}) + +That session is the live owner now. Stop and read their claim before touching +the branch; take over only by posting a fresh claim of your own once theirs has +lapsed or been released (shared/workflow/claim-pr.md, 2-hour rule). + +{override}=1 clears this refusal; say why. +""" + + +def emit(decision=None, reason=None, context=None): + out = {"hookSpecificOutput": {"hookEventName": "PreToolUse"}} + spec = out["hookSpecificOutput"] + if decision: + spec["permissionDecision"] = decision + spec["permissionDecisionReason"] = reason + if context: + spec["additionalContext"] = context + print(json.dumps(out)) + + +def warn_open(message): + """Visible fail-open: stderr plus context, never a silent allow.""" + print(f"no-pr-work-without-claim: {message}", file=sys.stderr) + emit(context=f"no-pr-work-without-claim could not verify a claim: " + f"{message}. Allowed; check the PR's claim comments by hand.") + + +def run(argv, cwd=None): + return subprocess.run(argv, cwd=cwd, capture_output=True, text=True, + timeout=NET_TIMEOUT) + + +def git(cwd, *args): + r = run(["git", *args], cwd=cwd) + return r.stdout.strip() if r.returncode == 0 else None + + +def gh_json(path): + """GET `path` through `gh api`; raises on any failure (caller fails open).""" + cmd = shlex.split(os.environ.get("PR_CLAIM_GH_CMD", "gh")) + r = run([*cmd, "api", "--paginate", path]) + if r.returncode != 0: + raise RuntimeError(f"gh api {path} failed: {r.stderr.strip()[:200]}") + text = r.stdout.strip() + if not text: + return [] + # --paginate concatenates one JSON array per page: ][ -> , + return json.loads(re.sub(r"\]\s*\[", ",", text)) + + +def norm(text): + return text.replace("\\", "/").lower() + + +def is_claim(body): + low = (body or "").lower() + return AGENT_MARKER in low and any(p in low for p in CLAIM_PHRASES) + + +def names_session(body, session_id, worktree): + low = norm(body or "") + if session_id and session_id.lower() in low: + return True + if worktree: + wt = re.escape(norm(worktree).rstrip("/")) + return re.search(wt + r"(?![\w.-])", low) is not None + return False + + +def classify_claims(comments, session_id, worktree): + """(mine, theirs, anonymous): claim comments split by attributable session.""" + mine, theirs, anonymous = [], [], [] + for c in comments: + if not is_claim(c.get("body")): + continue + if names_session(c["body"], session_id, worktree): + mine.append(c) + elif re.search(r"session (?:id|worktree)", c["body"], re.I): + theirs.append(c) + else: + anonymous.append(c) + return mine, theirs, anonymous + + +def matched_command(command): + """(verb, env) for the first commit/push worth guarding, else None.""" + cmds = simple_commands(command) + if not cmds: + return None + for argv in cmds: + parsed = git_subcommand(argv) + if parsed is None: + continue + sub, rest, env = parsed + if sub not in ("commit", "push"): + continue + if "--dry-run" in rest or (sub == "push" and ( + "--delete" in rest or "-d" in rest)): + continue + if env_value(env, OVERRIDE) == "1": + continue + return sub, env + return None + + +def activity_warning(owner, repo, branch, since, cwd): + """Foreign pushes since `since`, as a warning string or None.""" + items = gh_json(f"repos/{owner}/{repo}/activity" + f"?ref=refs/heads/{branch}&per_page=50") + foreign = [] + for it in items: + if it.get("activity_type") not in ("push", "force_push"): + continue + if (it.get("timestamp") or "") <= since: + continue + after = it.get("after") or "" + if not after: + continue + if run(["git", "merge-base", "--is-ancestor", after, "HEAD"], + cwd=cwd).returncode == 0: + continue + actor = (it.get("actor") or {}).get("login", "?") + foreign.append(f"{it.get('timestamp')} {actor} -> {after[:8]}") + if not foreign: + return None + return ("Pushes to this branch since your claim that are NOT in your local " + "history (another session or person, or a force-push of yours): " + + "; ".join(foreign[:5]) + + ". Fetch and read them before pushing " + "(shared/workflow/claim-pr.md; ai-config#4155).") + + +def evaluate(payload): + """Returns None (silent allow) or a dict of emit() kwargs.""" + command = (payload.get("tool_input") or {}).get("command") or "" + if not isinstance(command, str) or not command.strip(): + return None + if simple_commands is None: + return None + if os.environ.get(OVERRIDE) == "1" or LEADING_OVERRIDE.match(command): + return None + hit = matched_command(command) + if hit is None: + return None + verb = hit[0] + + cwd = payload.get("cwd") or os.getcwd() + branch = git(cwd, "rev-parse", "--abbrev-ref", "HEAD") + if not branch or branch in SKIP_BRANCHES: + return None + m = re.search(r"github\.com[:/]([^/\s]+)/([^/\s]+?)(?:\.git)?$", + git(cwd, "remote", "get-url", "origin") or "") + if not m or m.group(1).lower() not in OWNERS: + return None + owner, repo = m.group(1), m.group(2) + worktree = git(cwd, "rev-parse", "--show-toplevel") or "" + session_id = payload.get("session_id") or "" + + try: + prs = gh_json(f"repos/{owner}/{repo}/pulls?state=open" + f"&head={owner}:{branch}") + if not prs: + return None + pr = prs[0] + comments = gh_json(f"repos/{owner}/{repo}/issues/{pr['number']}" + f"/comments?per_page=100") + mine, theirs, anonymous = classify_claims(comments, session_id, + worktree) + pr_url = pr.get("html_url") or f"#{pr['number']}" + if not mine: + return {"decision": "deny", "reason": DENY_NO_CLAIM.format( + verb=verb, branch=branch, pr_url=pr_url, pr_number=pr["number"], + worktree=worktree, session_id=session_id or "", + override=OVERRIDE)} + latest_mine = max(c["created_at"] for c in mine) + newer = [c for c in theirs if c["created_at"] > latest_mine] + if newer: + t = max(newer, key=lambda c: c["created_at"]) + return {"decision": "deny", "reason": DENY_SUPERSEDED.format( + verb=verb, branch=branch, pr_url=pr_url, mine_at=latest_mine, + theirs_url=t.get("html_url", "?"), theirs_at=t["created_at"], + override=OVERRIDE)} + notes = [] + if any(c["created_at"] > latest_mine for c in anonymous): + notes.append("A claim that names no session was posted after " + "yours; it may be another session's. Read the PR " + "comments before continuing.") + if verb == "push": + warning = activity_warning(owner, repo, branch, latest_mine, cwd) + if warning: + notes.append(warning) + return {"context": " ".join(notes)} if notes else None + except Exception as exc: # network, gh missing, bad JSON: visible fail-open + return {"open_error": f"{type(exc).__name__}: {exc}"} + + +def main(): + try: + payload = json.load(sys.stdin) + except Exception as exc: + print(f"no-pr-work-without-claim: unreadable hook input ({exc})", + file=sys.stderr) + return 0 + if not isinstance(payload, dict) or payload.get("tool_name") not in ( + "Bash", "bash", "run_command", "execute_command", "terminal", + "shell"): + return 0 + try: + result = evaluate(payload) + except Exception as exc: + warn_open(f"could not evaluate ({exc})") + return 0 + if result is None: + return 0 + if "open_error" in result: + warn_open(result["open_error"]) + else: + emit(result.get("decision"), result.get("reason"), + result.get("context")) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/hooks/test-no-pr-work-without-claim.py b/hooks/test-no-pr-work-without-claim.py new file mode 100644 index 000000000..cb73248b4 --- /dev/null +++ b/hooks/test-no-pr-work-without-claim.py @@ -0,0 +1,244 @@ +#!/usr/bin/env python3 +"""Tests for no-pr-work-without-claim.py (ai-config#4155). + +Each case runs the hook as a subprocess against a REAL temporary git repo whose +`origin` is a Morrison-Lab URL, with `gh` replaced by a fake (PR_CLAIM_GH_CMD) +that serves canned forge responses. The fake is a fixture, so these cases show +the hook's decisions given those responses, not what the real forge returns +(shared/workflow/fixtures-are-not-evidence.md). + +The negatives carry the weight: this guard DENIES, and `git commit` / `git push` +are the commonest strings in the corpus. + +Run: python3 hooks/test-no-pr-work-without-claim.py [hooks/no-pr-work-without-claim.py] +""" +import json +import os +import subprocess +import sys +import tempfile + +HERE = os.path.dirname(os.path.realpath(__file__)) +SUBJECT = sys.argv[1] if len(sys.argv) > 1 else os.path.join( + HERE, "no-pr-work-without-claim.py") + +MARKER = "_Posted by Claude Code (AI agent) --- not written by a human._" +SID = "session_test_AAAA" +FAKE_GH = """\ +import json, os, sys +data = json.load(open(os.environ["FAKE_GH_DATA"])) +path = sys.argv[-1] +for needle, resp in data.items(): + if needle in path: + if resp == "FAIL": + sys.stderr.write("HTTP 502 simulated") + sys.exit(1) + print(json.dumps(resp)) + sys.exit(0) +print("[]") +""" + +FAILURES = [] +COUNT = [0] + + +def sh(cwd, *args): + return subprocess.run(args, cwd=cwd, capture_output=True, text=True, + check=True).stdout.strip() + + +def make_repo(tmp, remote="https://github.com/Morrison-Lab/test-repo.git", + branch="feat/x"): + repo = os.path.join(tmp, "wt-one") + os.makedirs(repo) + sh(repo, "git", "init", "-q", "-b", "main") + sh(repo, "git", "config", "user.email", "t@example.com") + sh(repo, "git", "config", "user.name", "t") + sh(repo, "git", "commit", "-q", "--allow-empty", "-m", "init") + if branch != "main": + sh(repo, "git", "checkout", "-q", "-b", branch) + sh(repo, "git", "remote", "add", "origin", remote) + return repo, sh(repo, "git", "rev-parse", "--show-toplevel") + + +def claim(body_extra="", marker=True, phrase="hold off", at="2026-09-30T20:00:00Z", + url="https://github.com/x/y/pull/1#c1"): + body = (f"Claude Code CLI (local session) is working on this --- please " + f"{phrase} on pushing to this branch.\n\n{body_extra}\n\n") + return {"body": body + (MARKER if marker else ""), "created_at": at, + "html_url": url} + + +def run(name, command, expect, *, comments=None, prs="open", repo_kwargs=None, + activity=None, env=None, stdin_raw=None, tool="Bash", branch="feat/x", + gh_fail=False, check_in_ctx=None, extra_payload=None): + """expect: None (silent) | 'deny' | 'ctx' (additionalContext, no decision).""" + COUNT[0] += 1 + with tempfile.TemporaryDirectory() as tmp: + repo, top = make_repo(tmp, branch=branch, **(repo_kwargs or {})) + if callable(comments): + comments = comments(top) + pr_list = ([{"number": 7, "html_url": "https://github.com/Morrison-Lab/" + "test-repo/pull/7"}] if prs == "open" else []) + data = {"pulls?state=open": "FAIL" if gh_fail else pr_list, + "/comments": comments or [], + "/activity": activity if activity is not None else []} + data_path = os.path.join(tmp, "data.json") + fake_path = os.path.join(tmp, "fakegh.py") + with open(data_path, "w") as f: + json.dump(data, f) + with open(fake_path, "w") as f: + f.write(FAKE_GH) + e = dict(os.environ) + e.pop("ALLOW_UNCLAIMED_PR_WORK", None) + e["FAKE_GH_DATA"] = data_path + e["PR_CLAIM_GH_CMD"] = (f'"{sys.executable.replace(chr(92), "/")}" ' + f'"{fake_path.replace(chr(92), "/")}"') + e.update(env or {}) + payload = {"tool_name": tool, "tool_input": {"command": command}, + "cwd": repo, "session_id": SID} + payload.update(extra_payload or {}) + stdin = stdin_raw if stdin_raw is not None else json.dumps(payload) + r = subprocess.run([sys.executable, SUBJECT], input=stdin, + capture_output=True, text=True, env=e, timeout=60) + out = r.stdout.strip() + spec = json.loads(out)["hookSpecificOutput"] if out else {} + got = ("deny" if spec.get("permissionDecision") == "deny" + else "ctx" if spec.get("additionalContext") else None) + ok = got == expect and r.returncode == 0 + if ok and check_in_ctx: + blob = (spec.get("additionalContext", "") + r.stderr + + spec.get("permissionDecisionReason", "")) + ok = check_in_ctx in blob + if not ok: + FAILURES.append(f"{name}: expected {expect}, got {got} " + f"(rc={r.returncode}) out={out[:300]!r} " + f"err={r.stderr[:200]!r}") + return + + +def mine(top): + return [claim(f"Session worktree: `{top}`")] + + +COMMIT = 'git commit -m "fix: x"' +PUSH = "git push origin feat/x" + +# --- silent cases --------------------------------------------------------- +run("N1 unrelated command", "git status", None) +run("N2 quoted mention of commit", 'echo "git commit -m x && git push"', None) +run("N3 commit-tree is not commit", "git commit-tree HEAD^{tree}", None) +run("N4 dry-run push", "git push --dry-run origin feat/x", None) +run("N5 branch deletion push", "git push --delete origin old", None) +run("N6 on main", COMMIT, None, branch="main") +run("N7 foreign owner", COMMIT, None, + repo_kwargs={"remote": "https://github.com/someone/else.git"}) +run("N8 no open PR", COMMIT, None, prs="none") +run("N9 non-Bash tool", COMMIT, None, tool="Read") +run("N10 malformed stdin", COMMIT, None, stdin_raw="{not json") +run("N11 heredoc body naming commit", + "cat <<'EOF'\ngit commit -m x\nEOF", None) + +# --- the claim requirement ------------------------------------------------- +run("D1 commit, PR has no claim", COMMIT, "deny") +run("D2 push, PR has no claim", PUSH, "deny") +run("D3 claim without session identity", COMMIT, "deny", + comments=[claim()]) +run("D4 claim naming session lacks the agent marker", COMMIT, "deny", + comments=[claim(f"Session id: {SID}", marker=False)]) +run("D5 claim lacks the hold-off wording", COMMIT, "deny", + comments=[claim(f"Session id: {SID}", phrase="please note")]) +run("D6 claim names a prefix-colliding worktree", COMMIT, "deny", + comments=lambda top: [claim(f"Session worktree: `{top}-ab`")]) +run("A1 claim naming session id", COMMIT, None, + comments=[claim(f"Session id: `{SID}`")]) +run("A2 claim naming worktree path", PUSH, None, comments=mine) +run("A3 worktree path written with backslashes", COMMIT, None, + comments=lambda top: [claim("Session worktree: `" + + top.replace("/", chr(92)) + "`")]) +run("A4 legacy 'paws off' wording", COMMIT, None, + comments=lambda top: [claim(f"Session worktree: `{top}`", + phrase="paws off")]) +run("A5 commit chained before push", f"{COMMIT} && {PUSH}", None, + comments=mine) + +# --- takeover -------------------------------------------------------------- +other_newer = lambda top: [ + claim(f"Session worktree: `{top}`", at="2026-09-30T20:00:00Z"), + claim("Session id: `session_other_BBBB`", at="2026-09-30T21:00:00Z")] +other_older = lambda top: [ + claim("Session id: `session_other_BBBB`", at="2026-09-30T19:00:00Z"), + claim(f"Session worktree: `{top}`", at="2026-09-30T20:00:00Z")] +anon_newer = lambda top: [ + claim(f"Session worktree: `{top}`", at="2026-09-30T20:00:00Z"), + claim(at="2026-09-30T21:00:00Z")] +run("T1 another session claimed after mine", COMMIT, "deny", + comments=other_newer) +run("T2 another session claimed before mine", COMMIT, None, + comments=other_older) +run("T3 anonymous newer claim only warns", COMMIT, "ctx", + comments=anon_newer, check_in_ctx="names no session") + +# --- override -------------------------------------------------------------- +run("O1 env prefix", f"ALLOW_UNCLAIMED_PR_WORK=1 {COMMIT}", None) +run("O2 leading export", f"export ALLOW_UNCLAIMED_PR_WORK=1 && {COMMIT}", None) +run("O3 process environment", COMMIT, None, + env={"ALLOW_UNCLAIMED_PR_WORK": "1"}) +run("O4 override =0 does not clear", f"ALLOW_UNCLAIMED_PR_WORK=0 {COMMIT}", + "deny") +run("O5 override on an unrelated command does not clear", + f"ALLOW_UNCLAIMED_PR_WORK=1 git status && {COMMIT}", "deny") + +# --- fail open, visibly ---------------------------------------------------- +run("F1 forge error allows and says so", COMMIT, "ctx", gh_fail=True, + check_in_ctx="could not verify") +run("F2 missing gh allows and says so", COMMIT, "ctx", + env={"PR_CLAIM_GH_CMD": "definitely-not-a-real-gh-binary"}, + check_in_ctx="could not verify") + +# --- activity warning on push ---------------------------------------------- +foreign = [{"activity_type": "push", "timestamp": "2026-09-30T22:00:00Z", + "after": "d" * 40, "actor": {"login": "peer"}}] +old_foreign = [{"activity_type": "push", "timestamp": "2026-09-30T10:00:00Z", + "after": "d" * 40, "actor": {"login": "peer"}}] +branch_del = [{"activity_type": "branch_deletion", + "timestamp": "2026-09-30T22:00:00Z", "after": "d" * 40, + "actor": {"login": "peer"}}] +run("P1 foreign push since claim warns", PUSH, "ctx", comments=mine, + activity=foreign, check_in_ctx="NOT in your local history") +run("P2 foreign push BEFORE claim is silent", PUSH, None, comments=mine, + activity=old_foreign) +run("P3 foreign push but verb is commit: no activity check", COMMIT, None, + comments=mine, activity=foreign) +run("P4 non-push activity ignored", PUSH, None, comments=mine, + activity=branch_del) + +# P6: own push already in local history is silent. HEAD sha is only known once +# the repo exists, so this case builds its own activity from inside the repo. +COUNT[0] += 1 +with tempfile.TemporaryDirectory() as tmp: + repo, top = make_repo(tmp) + head = sh(repo, "git", "rev-parse", "HEAD") + data = {"pulls?state=open": [{"number": 7, "html_url": "u"}], + "/comments": mine(top), + "/activity": [{"activity_type": "push", + "timestamp": "2026-09-30T22:00:00Z", "after": head, + "actor": {"login": "me"}}]} + dp, fp = os.path.join(tmp, "d.json"), os.path.join(tmp, "f.py") + json.dump(data, open(dp, "w")) + open(fp, "w").write(FAKE_GH) + e = dict(os.environ, FAKE_GH_DATA=dp, PR_CLAIM_GH_CMD=( + f'"{sys.executable.replace(chr(92), "/")}" "{fp.replace(chr(92), "/")}"')) + e.pop("ALLOW_UNCLAIMED_PR_WORK", None) + r = subprocess.run([sys.executable, SUBJECT], capture_output=True, + text=True, env=e, input=json.dumps( + {"tool_name": "Bash", "cwd": repo, + "session_id": SID, + "tool_input": {"command": PUSH}})) + if r.stdout.strip(): + FAILURES.append(f"P6 own push in history should be silent: {r.stdout}") + +print(f"{COUNT[0]} cases, {len(FAILURES)} failures") +for f in FAILURES: + print("FAIL", f) +sys.exit(1 if FAILURES else 0) diff --git a/plugins/ai-config-hooks/hooks/hooks.json b/plugins/ai-config-hooks/hooks/hooks.json index 3ac684e17..2e190fdd1 100644 --- a/plugins/ai-config-hooks/hooks/hooks.json +++ b/plugins/ai-config-hooks/hooks/hooks.json @@ -444,6 +444,13 @@ "script": "no-commit-chained-to-push.py", "why": "shared/workflow/check-before-pushing.md, plus ai-config#2992, which is a REPORT rather than a measurement -- its author states they verified the mechanism from the hook registration and did not reproduce the lost commit. A PreToolUse deny rejects the WHOLE Bash invocation, so a call that chains `git commit` into `git push` loses the commit when any guard refuses the push -- and no-push-without-self-review.py and no-clobbering-push.py are both registered PreToolUse on Bash. Their refusal names only the push, so it reads as \"the push was blocked\" while the change is still an uncommitted working-tree edit. Reported 2026-09-02, caught only because an adversarial reviewer checked whether HEAD had moved. Note that on a HEREDOC commit neither sibling currently sees a push at all (they carry the #2993 splitter defect this hook's library fixes), so for that shape today nothing would be lost -- a reason to fix #2993, not to exempt the shape, and the deny message is worded so it never asserts a sibling would have refused a particular call. DENIES rather than warns, unlike its warn-dupe-check-chained-to-create.py sibling of the same shape: an advisory additionalContext is attached while the call proceeds, so a sibling's deny in the same pass still discards the commit, and whether the author is even TOLD depends on unverified harness behaviour (does a non-denying hook's context survive a sibling's deny -- see the hook docstring for the ten-minute experiment that settles it). The refusal does not depend on that answer: it stops the chain reaching the siblings at all. The refusal is always satisfiable (issue the same two commands as two calls) and clearable with an `ALLOW_COMMIT_AND_PUSH=1` env-assignment prefix. Matches over an argv split (scripts/lib/shellcmd.py) rather than the raw string, so a quoted commit message, a heredoc body, and `git commit-tree`/`git commit-graph` cannot trip it. Requires commit BEFORE push: push-then-commit loses nothing. No exemption for a --dry-run or --delete command: one was written on review advice and removed when a second review measured `git push --force --delete` and `--dry-run --no-dry-run --force` going silent here while no-clobbering-push.py denied both -- a refused dry-run costs one tool call, a missed force-delete costs the commit. Fails open. Tracked as #2992; the shared splitter's heredoc defect and the eight unmigrated copies are #2993 (eight, not seven: derive the set from `grep -rlF '_SHELL_OPS = set(\"();|&\")' hooks/`, since remind-ci-crosscheck-sim-verdict.py spells its copy without the leading underscore)." }, + { + "type": "command", + "command": "${CLAUDE_PLUGIN_ROOT}/run-hook.sh 'python3 \"${CLAUDE_PLUGIN_ROOT}/../../hooks/no-pr-work-without-claim.py\"'", + "timeout": 30, + "script": "no-pr-work-without-claim.py", + "why": "ai-config#4155, measured 2026-09-30: two agent sessions worked the same three PR branches at once and neither posted a claim, so a cloud session's restore commit on PR #4134 left memories/preferences.md at 706 lines instead of 1177. shared/workflow/claim-pr.md says to claim first and nothing enforced it; ListAgents cannot see a cloud session. DENIES a `git commit` or `git push` on a branch with an open Morrison-Lab PR unless a claim comment (agent marker plus hold-off wording) names THIS session by session id or worktree path, and denies when another session's claim is newer than yours. Before a push it WARNS (never denies) about pushes since your claim that are not in local history, read from the forge activity endpoint. Fails OPEN and says so on any network or gh failure. Local half only: hooks are inert in remote/web sessions (#2004). Clear with ALLOW_UNCLAIMED_PR_WORK=1. Timeout is 30s because it makes up to three forge reads." + }, { "type": "command", "command": "${CLAUDE_PLUGIN_ROOT}/run-hook.sh 'python3 \"${CLAUDE_PLUGIN_ROOT}/../../hooks/flag-chained-push.py\"'", diff --git a/shared/workflow/claim-pr.md b/shared/workflow/claim-pr.md index f880d352a..7b35911bb 100644 --- a/shared/workflow/claim-pr.md +++ b/shared/workflow/claim-pr.md @@ -114,6 +114,24 @@ while under-respecting a live one costs a collision. issue claims last 2 hours from the most recent push or comment; if it's been longer than that, reassert your claim.") +**Claim before the first commit on a PR branch, and look for other writers before you push.** +Two agent sessions, neither claiming, worked the same three PR branches on 2026-09-30 ([#4155](https://github.com/Morrison-Lab/ai-config/issues/4155)). +A cloud session's "restore" commit on [PR #4134](https://github.com/Morrison-Lab/ai-config/pull/4134) left `memories/preferences.md` at 706 lines instead of 1177. +The local session found the collision only through its pre-push `git ls-remote`, after its own commits existed. +`ListAgents` cannot see a cloud session, so a claim comment and the forge's own record are the only evidence a peer is there. + +Every session that comments under one account shares one forge login, so a claim must also say which session it is. +Add a `Session worktree:` line (the checkout path) or a `Session id:` line to the claim body. +[`hooks/no-pr-work-without-claim.py`](../../hooks/no-pr-work-without-claim.py) denies a `git commit` or `git push` on a branch with an open Morrison-Lab PR unless one claim comment carries the agent marker and names this session. +It also denies when a claim naming a different session is newer than yours, and before a push it warns about pushes since your claim that your local history lacks. +The check reads `repos///activity?ref=refs/heads/`. +Hooks are inert in remote and web sessions, so a cloud session relies on its own instructions to run the same two reads. + +- **Do:** post the claim, with a `Session worktree:` or `Session id:` line, before the first commit on a PR branch you did not just create. +- **Do:** read the activity endpoint (or `git ls-remote` plus `git log`) for pushes you did not make before every push, and treat one as a peer. +- **Don't:** commit locally on a PR branch first and claim "when you push" --- the commit is the work a collision strands. +- **Don't:** treat a quiet `ListAgents` as proof nobody else is on the branch. + **Every detector of a claim matches the OLD wording as well as the new one, and dropping the old alternation is the one edit that fails silently.** There were **two** retired invariants, not one, and enumerating them from the file in front of you is how the second was missed for a whole review round. Most emitters carried the `paws off` invariant --- `claim-pr`, `gi`, `st`, `gip`, `pr-on-claim`, `post-merge`, `handoff` (as "still claimed, paws off.") and the orchestrator (as "paws off until done"). From 77d2c90d003b617acda1f1da1ed809f509b21858 Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:48:45 -0700 Subject: [PATCH 3/9] fix: address adversarial review of the claim-required hook (#4155) - Resolve the directory a command runs in from earlier `cd`s and `git -C`; an unresolvable move (`cd -`, `$VAR`, `--git-dir`) is a visible fail-open. - Match a worktree path or session id only as a whole token, so a nested worktree or a longer id is not read as this session's claim. - Decode `gh api --paginate` pages one at a time instead of rewriting `][`, which corrupted comment bodies containing `] [`; read the activity endpoint as a single page. - Give the activity read its own try so a 403/404 keeps the claim outcomes. - Dry-run grammar: push `-n`, last occurrence wins for `--no-dry-run`. - A claim needs `working on this` plus hold-off wording and must not be an unclaim; a standing peer claim gets stand-down advice instead of "post a claim". - Document the known limits (claim age, release comments, fork PRs, cost). - 56 cases now; 20 of 20 mutants killed. Co-Authored-By: Claude Sonnet 5.5 --- README.md | 2 +- hooks/no-pr-work-without-claim.py | 198 +++++++++++++++++++++---- hooks/test-no-pr-work-without-claim.py | 86 +++++++++-- shared/workflow/claim-pr.md | 1 + 4 files changed, 247 insertions(+), 40 deletions(-) diff --git a/README.md b/README.md index 3d11725c7..24a820684 100644 --- a/README.md +++ b/README.md @@ -502,7 +502,7 @@ The payload gaps that remain and the per-guard status are in | `flag-stale-adjacent-comment.py` | `PreToolUse` (Bash) | warns, never blocks, when a `git commit` changes a literal value while an unchanged comment within ten lines still asserts the old one | | `warn-unparseable-staged-config.py` | `PreToolUse` (Bash) | warns when `git commit` would commit staged `.toml`, `.json`, or `.yaml`/`.yml` config files that fail parser validation (`tomllib`, `json`, `yaml.safe_load`) | | `no-delete-branch-under-stacked-pr.py` | `PreToolUse` (Bash) | warns when `gh pr merge --delete-branch` or `gh pr close --delete-branch` would delete a branch that is an open PR's base. GitHub's documented behaviour is to retarget such a PR, but a measured case closed it instead, and a closed PR can be neither retargeted nor reopened while its base is gone. Silent when nothing is stacked, when the query fails or returns an unexpected shape, when `gh` is absent, when the command carries no `-R` or PR target, and on `--delete-branch=false` | -| `no-pr-work-without-claim.py` | `PreToolUse` (Bash) | denies a `git commit` or `git push` on a branch whose open Morrison-Lab PR has no claim comment (agent marker plus hold-off wording) naming this session by session id or worktree path, and denies when another session's claim is newer than yours. Before a push it warns, never denies, about pushes since your claim that local history lacks, read from the forge activity endpoint. Fails open and says so on any `gh` or network failure. Local half of ai-config#4155: hooks are inert in remote and web sessions. | +| `no-pr-work-without-claim.py` | `PreToolUse` (Bash) | denies a `git commit` or `git push` on a branch whose open Morrison-Lab PR has no claim comment (agent marker plus hold-off wording) naming this session by session id or worktree path, and denies when another session's claim is newer than yours. Before a push it warns, never denies, about pushes since your claim that local history lacks, read from the forge activity endpoint. Fails open and says so on any `gh` or network failure. A claim's age and release comments are not evaluated, and a fork PR is not seen. Local half of ai-config#4155: hooks are inert in remote and web sessions. | | `no-clobbering-push.py` | `PreToolUse` (Bash) | refuses a bare `git push --force`/`-f`, whose remedy (`--force-with-lease --force-if-includes`) costs one word. Warns on every other push whose remote tip a live, read-only `git ls-remote` shows is not an ancestor of the ref being pushed (which is `HEAD` only when the refspec says so, resolved in the directory the push runs in rather than the session's -- a `cd`, scoped to its subshell but not to a brace group, and declined where a compound statement's body, a short-circuited alternative, or a fork into a background job or a pipeline means the pushing shell never takes its effect, then the push's own `-C`, declined in turn when the shell would have had to expand it), names that directory and qualifies its remediation commands with `git -C` when it is not the call's own, declines the reading when the directory is indeterminate or `--git-dir`/`--work-tree`/`GIT_DIR=` redirected the repository, and stays silent on a fast-forward | | `no-commit-chained-to-push.py` | `PreToolUse` (Bash) | denies a Bash call that chains a `git commit` into a later `git push`. A PreToolUse deny rejects the whole invocation, so a guard refusing the push discards the commit too while its message speaks only about the push (ai-config#2992). Denies rather than warns because the refusal stops the chain reaching the sibling guards at all, and its remedy -- two Bash calls -- is always available. Clearable with `ALLOW_COMMIT_AND_PUSH=1`, either prefixing the commit or push or as the call's own leading assignment (a subshell or short-circuited one sets nothing and does not count). Matches over an argv split (`scripts/lib/shellcmd.py`), so a quoted commit message, a heredoc body and `git commit-tree` cannot trip it, while `timeout 60 git push`, `/usr/bin/git push` and `{ git commit; } && git push` all resolve -- the guard has to fire wherever its siblings would. There is no exemption for a `--dry-run` or `--delete` command: one was written and removed after a review measured `git commit ... && git push --force --delete` and `... --dry-run --no-dry-run --force` both going silent while `no-clobbering-push.py` denied them | | `flag-chained-push.py` | `PreToolUse` (Bash) | warns, never blocks, when a `git push` is chained after another command with `&&`, `;`, or `\|\|`, piped onward with `\|`, or suffixed by a redirection (`>`, `>>`, or an fd form like `2>&1`). `no-clobbering-push.py` and the plugin's own `no-push-without-self-review.py` push policy both parse the WHOLE command text for a push rather than the isolated segment, so a trailing `2>&1` hands either parser a bare `2` sitting where a commit-ish token would sit in other shapes, and a chained prefix reads as part of the same invocation; a refused chain runs NOTHING, which the refusal naming only the push invites the author to misread as the prefix having succeeded. Measured on Lacaedemon/sparta, 2026-09-05: three refusals in one session | diff --git a/hooks/no-pr-work-without-claim.py b/hooks/no-pr-work-without-claim.py index 1335f4733..31136c028 100644 --- a/hooks/no-pr-work-without-claim.py +++ b/hooks/no-pr-work-without-claim.py @@ -47,6 +47,22 @@ accepted whatever its age; staleness over-approximation via `updatedAt` would mark every session's own pushes as fresh activity. Known limit. +A release ("unclaiming") comment is not modelled either: it only keeps a +comment containing `unclaim` from counting as a claim. A claim whose session +later released it still satisfies this check. Known limit. + +A PR from a fork (head repo under another owner) is not found by the +`head=:` query, so the hook stays silent for it. Known limit. + +The directory a command runs in is the payload `cwd` moved by earlier `cd`s +and the command's own `git -C` (`scripts/lib/shellcmd.py`'s +`resolve_cd_target`); an unresolvable move (`cd -`, `$VAR`, `--git-dir`) is a +visible fail-open, not a guess. Subshell scoping of a `cd` is not modelled +(`simple_commands` flattens it). + +Cost: one to three forge reads per matched commit or push, uncached. Accepted +because a stale cached "claimed" answer is exactly the failure being guarded. + ## Failure policy Fails OPEN on any parse trouble, outside a git repo, off a Morrison-Lab remote, @@ -85,11 +101,17 @@ "scripts", "lib") if _LIB not in sys.path: sys.path.insert(0, _LIB) - from shellcmd import env_value, git_subcommand, simple_commands + from shellcmd import (GIT_VALUE_OPTS, env_value, resolve_cd_target, + simple_commands, strip_env) except Exception as _exc: # broken install: fail open, and say so print(f"no-pr-work-without-claim: cannot load scripts/lib/shellcmd.py " f"({_exc}); not evaluating", file=sys.stderr) - env_value = git_subcommand = simple_commands = None + env_value = simple_commands = strip_env = resolve_cd_target = None + GIT_VALUE_OPTS = frozenset() + +# Raised when the directory a command runs in cannot be named statically. +class Indeterminate(Exception): + pass LEADING_OVERRIDE = re.compile( r"\A[ \t]*(?:export[ \t]+)?" + OVERRIDE + r"=1[ \t]*(?:;|&&|\|\||\r?\n|\Z)") @@ -117,6 +139,22 @@ and say why. """ +DENY_PEER_CLAIM = """\ +`git {verb}` on branch `{branch}` ({pr_url}): the PR has a claim that does not +name this session, and this session has none of its own. + + that claim: {peer_url} ({peer_at}) + +It is either another session's, or your own posted without a `Session +worktree:` / `Session id:` line (re-post it with one). If it is another +session's, that session is working this branch now. Do not post a competing +claim over it and do not commit: stand down, or take over only after theirs has lapsed +or been released (shared/workflow/claim-pr.md, 2-hour rule), by posting a fresh +claim that names this session. + +{override}=1 clears this refusal; say why. +""" + DENY_SUPERSEDED = """\ `git {verb}` on branch `{branch}` ({pr_url}): another session claimed this PR after you did. @@ -160,17 +198,29 @@ def git(cwd, *args): return r.stdout.strip() if r.returncode == 0 else None -def gh_json(path): - """GET `path` through `gh api`; raises on any failure (caller fails open).""" +def gh_json(path, paginate=True): + """GET `path` through `gh api`; raises on any failure (caller fails open). + + `--paginate` prints one JSON array per page, back to back. They are decoded + one at a time with `raw_decode` rather than joined by rewriting `][`, which + would also rewrite a comment body containing `] [`. + """ cmd = shlex.split(os.environ.get("PR_CLAIM_GH_CMD", "gh")) - r = run([*cmd, "api", "--paginate", path]) + r = run([*cmd, "api", *(["--paginate"] if paginate else []), path]) if r.returncode != 0: raise RuntimeError(f"gh api {path} failed: {r.stderr.strip()[:200]}") - text = r.stdout.strip() - if not text: - return [] - # --paginate concatenates one JSON array per page: ][ -> , - return json.loads(re.sub(r"\]\s*\[", ",", text)) + text, items, pos = r.stdout, [], 0 + decoder = json.JSONDecoder() + while True: + while pos < len(text) and text[pos].isspace(): + pos += 1 + if pos >= len(text): + return items + page, pos = decoder.raw_decode(text, pos) + if isinstance(page, list): + items.extend(page) + else: + items.append(page) def norm(text): @@ -178,17 +228,31 @@ def norm(text): def is_claim(body): + """A claim comment: agent marker, `working on this`, and hold-off wording. + + A release or status comment can contain "hold off" in other senses ("no + need to hold off"), so the opening claim phrase is required as well. + """ low = (body or "").lower() - return AGENT_MARKER in low and any(p in low for p in CLAIM_PHRASES) + return (AGENT_MARKER in low and "working on this" in low + and any(p in low for p in CLAIM_PHRASES) + and "unclaim" not in low) def names_session(body, session_id, worktree): + """True when `body` names this session's id or this exact worktree path. + + Both tests are token-bounded: an id or path that is merely a PREFIX of a + longer one (`...-ab`, `.../.claude/worktrees/x`) does not match. + """ low = norm(body or "") - if session_id and session_id.lower() in low: - return True + if session_id: + sid = re.escape(session_id.lower()) + if re.search(r"(?= len(rest): + continue + sub, args = rest[1 + idx], rest[2 + idx:] if sub not in ("commit", "push"): continue - if "--dry-run" in rest or (sub == "push" and ( - "--delete" in rest or "-d" in rest)): + if is_dry_run(sub, args) or (sub == "push" and ( + "--delete" in args or "-d" in args)): continue if env_value(env, OVERRIDE) == "1": continue - return sub, env + workdir = cur + for d in c_dirs: + if workdir is None: + break + workdir = d if os.path.isabs(d) else os.path.join(workdir, d) + if workdir is None: + raise Indeterminate("a `cd` target that cannot be resolved statically") + return sub, workdir return None def activity_warning(owner, repo, branch, since, cwd): - """Foreign pushes since `since`, as a warning string or None.""" + """Foreign pushes since `since`, as a warning string or None. + + One page only: the endpoint lists newest first and only entries newer than + the claim matter, so walking the branch's whole history (`--paginate`) + would spend the network timeout for nothing. + """ items = gh_json(f"repos/{owner}/{repo}/activity" - f"?ref=refs/heads/{branch}&per_page=50") + f"?ref=refs/heads/{branch}&per_page=50", paginate=False) foreign = [] for it in items: if it.get("activity_type") not in ("push", "force_push"): @@ -264,12 +387,14 @@ def evaluate(payload): return None if os.environ.get(OVERRIDE) == "1" or LEADING_OVERRIDE.match(command): return None - hit = matched_command(command) + try: + hit = matched_command(command, payload.get("cwd") or os.getcwd()) + except Indeterminate as exc: + return {"open_error": f"cannot tell which repository the command " + f"runs in ({exc})"} if hit is None: return None - verb = hit[0] - - cwd = payload.get("cwd") or os.getcwd() + verb, cwd = hit branch = git(cwd, "rev-parse", "--abbrev-ref", "HEAD") if not branch or branch in SKIP_BRANCHES: return None @@ -292,6 +417,12 @@ def evaluate(payload): mine, theirs, anonymous = classify_claims(comments, session_id, worktree) pr_url = pr.get("html_url") or f"#{pr['number']}" + if not mine and (theirs or anonymous): + peer = max(theirs + anonymous, key=lambda c: c["created_at"]) + return {"decision": "deny", "reason": DENY_PEER_CLAIM.format( + verb=verb, branch=branch, pr_url=pr_url, + peer_url=peer.get("html_url", "?"), peer_at=peer["created_at"], + override=OVERRIDE)} if not mine: return {"decision": "deny", "reason": DENY_NO_CLAIM.format( verb=verb, branch=branch, pr_url=pr_url, pr_number=pr["number"], @@ -311,7 +442,16 @@ def evaluate(payload): "yours; it may be another session's. Read the PR " "comments before continuing.") if verb == "push": - warning = activity_warning(owner, repo, branch, latest_mine, cwd) + # Its own try: a 403/404 from the activity endpoint (it needs push + # access) must not discard the claim outcomes already computed. + try: + warning = activity_warning(owner, repo, branch, latest_mine, + cwd) + except Exception as exc: + warning = (f"could not read the forge activity for this " + f"branch ({type(exc).__name__}: {exc}); check for " + f"other writers by hand before pushing.") + print(f"no-pr-work-without-claim: {warning}", file=sys.stderr) if warning: notes.append(warning) return {"context": " ".join(notes)} if notes else None diff --git a/hooks/test-no-pr-work-without-claim.py b/hooks/test-no-pr-work-without-claim.py index cb73248b4..9fc8917f9 100644 --- a/hooks/test-no-pr-work-without-claim.py +++ b/hooks/test-no-pr-work-without-claim.py @@ -33,7 +33,11 @@ if resp == "FAIL": sys.stderr.write("HTTP 502 simulated") sys.exit(1) - print(json.dumps(resp)) + if isinstance(resp, dict) and "pages" in resp: + for page in resp["pages"]: # --paginate: one array per page + print(json.dumps(page)) + else: + print(json.dumps(resp)) sys.exit(0) print("[]") """ @@ -48,8 +52,8 @@ def sh(cwd, *args): def make_repo(tmp, remote="https://github.com/Morrison-Lab/test-repo.git", - branch="feat/x"): - repo = os.path.join(tmp, "wt-one") + branch="feat/x", name="wt-one"): + repo = os.path.join(tmp, name) os.makedirs(repo) sh(repo, "git", "init", "-q", "-b", "main") sh(repo, "git", "config", "user.email", "t@example.com") @@ -71,11 +75,18 @@ def claim(body_extra="", marker=True, phrase="hold off", at="2026-09-30T20:00:00 def run(name, command, expect, *, comments=None, prs="open", repo_kwargs=None, activity=None, env=None, stdin_raw=None, tool="Bash", branch="feat/x", - gh_fail=False, check_in_ctx=None, extra_payload=None): - """expect: None (silent) | 'deny' | 'ctx' (additionalContext, no decision).""" + gh_fail=False, check_in_ctx=None, extra_payload=None, cwd_other=False): + """expect: None (silent) | 'deny' | 'ctx' (additionalContext, no decision). + + `command` may be a callable (repo_path, other_path) -> str. `other` is a + second checkout on `main`; `cwd_other` makes it the payload cwd. + """ COUNT[0] += 1 with tempfile.TemporaryDirectory() as tmp: repo, top = make_repo(tmp, branch=branch, **(repo_kwargs or {})) + other, other_top = make_repo(tmp, branch="main", name="wt-other") + if callable(command): + command = command(top, other_top) if callable(comments): comments = comments(top) pr_list = ([{"number": 7, "html_url": "https://github.com/Morrison-Lab/" @@ -85,9 +96,9 @@ def run(name, command, expect, *, comments=None, prs="open", repo_kwargs=None, "/activity": activity if activity is not None else []} data_path = os.path.join(tmp, "data.json") fake_path = os.path.join(tmp, "fakegh.py") - with open(data_path, "w") as f: + with open(data_path, "w", encoding="utf-8") as f: json.dump(data, f) - with open(fake_path, "w") as f: + with open(fake_path, "w", encoding="utf-8") as f: f.write(FAKE_GH) e = dict(os.environ) e.pop("ALLOW_UNCLAIMED_PR_WORK", None) @@ -96,7 +107,7 @@ def run(name, command, expect, *, comments=None, prs="open", repo_kwargs=None, f'"{fake_path.replace(chr(92), "/")}"') e.update(env or {}) payload = {"tool_name": tool, "tool_input": {"command": command}, - "cwd": repo, "session_id": SID} + "cwd": other if cwd_other else repo, "session_id": SID} payload.update(extra_payload or {}) stdin = stdin_raw if stdin_raw is not None else json.dumps(payload) r = subprocess.run([sys.executable, SUBJECT], input=stdin, @@ -225,8 +236,10 @@ def mine(top): "timestamp": "2026-09-30T22:00:00Z", "after": head, "actor": {"login": "me"}}]} dp, fp = os.path.join(tmp, "d.json"), os.path.join(tmp, "f.py") - json.dump(data, open(dp, "w")) - open(fp, "w").write(FAKE_GH) + with open(dp, "w", encoding="utf-8") as f: + json.dump(data, f) + with open(fp, "w", encoding="utf-8") as f: + f.write(FAKE_GH) e = dict(os.environ, FAKE_GH_DATA=dp, PR_CLAIM_GH_CMD=( f'"{sys.executable.replace(chr(92), "/")}" "{fp.replace(chr(92), "/")}"')) e.pop("ALLOW_UNCLAIMED_PR_WORK", None) @@ -238,6 +251,59 @@ def mine(top): if r.stdout.strip(): FAILURES.append(f"P6 own push in history should be silent: {r.stdout}") +# --- review round 1 (adversarial-reviewer, 2026-09-30) ---------------------- +# Nested worktree path: a claim naming a checkout UNDER mine is not mine. +run("R1-1 claim names a worktree nested under mine", COMMIT, "deny", + comments=lambda top: [claim(f"Session worktree: `{top}/.claude/worktrees/x`")]) +# Session-id prefix collision. +run("R1-2 claim names a longer session id", COMMIT, "deny", + comments=[claim(f"Session id: `{SID}B`")]) +# Pagination join: a body containing `] [` must survive, and an empty page +# must not corrupt the decode. +run("R1-3 multi-page comments, `] [` in a body", COMMIT, None, + comments={"pages": [[claim("notes [a] [b] [c]")], + [claim(f"Session id: `{SID}`")]]}) +run("R1-4 empty first page", COMMIT, None, + comments={"pages": [[], [claim(f"Session id: `{SID}`")]]}) +# Directory: cd and git -C move the repository the command acts on. +run("R1-5 cd to a main checkout before commit is silent", + lambda top, other: f"cd '{other}' && git commit -m x", None) +run("R1-6 git -C a main checkout is silent", + lambda top, other: f"git -C '{other}' commit -m x", None) +run("R1-7 cwd on main but git -C the claimed-less PR checkout denies", + lambda top, other: f"git -C '{top}' commit -m x", "deny", cwd_other=True) +run("R1-8 cd - is indeterminate: visible fail-open", "cd - && " + COMMIT, + "ctx", check_in_ctx="cannot tell which repository") +run("R1-9 --git-dir is indeterminate: visible fail-open", + "git --git-dir=/elsewhere/.git commit -m x", "ctx", + check_in_ctx="cannot tell which repository") +# Dry-run grammar: -n on push, last occurrence wins, -n on commit is --no-verify. +run("R1-10 push -n is a dry run", "git push -n origin feat/x", None) +run("R1-11 --dry-run --no-dry-run is a live push", "git push --dry-run " + "--no-dry-run origin feat/x", "deny") +run("R1-12 commit -n is --no-verify, not a dry run", "git commit -n -m x", + "deny") +run("R1-13 commit --dry-run creates nothing", "git commit --dry-run", None) +# Activity read failure must not discard the claim outcomes. +run("R1-14 activity failure is a visible note", PUSH, "ctx", comments=mine, + activity="FAIL", check_in_ctx="could not read the forge activity") +run("R1-15 activity failure keeps the anonymous-claim note", PUSH, "ctx", + comments=anon_newer, activity="FAIL", check_in_ctx="names no session") +# Claim recognition: a release or a stray "hold off" is not a claim. +run("R1-16 release comment naming this session is not a claim", COMMIT, "deny", + comments=[{"body": f"Was working on this; you can stop having to hold " + f"off now --- unclaiming. Session id: `{SID}`\n\n{MARKER}", + "created_at": "2026-09-30T20:00:00Z", "html_url": "u"}]) +run("R1-17 'no need to hold off' status comment is not a claim", COMMIT, "deny", + comments=[{"body": f"No need to hold off. Session id: `{SID}`\n\n{MARKER}", + "created_at": "2026-09-30T20:00:00Z", "html_url": "u"}]) +# Message split: a standing peer claim gets stand-down advice, not "post a claim". +run("R1-18 peer claim: stand down, do not post over it", COMMIT, "deny", + comments=[claim("Session id: `session_other_BBBB`")], + check_in_ctx="Do not post a competing") +run("R1-19 no claim at all: how to post one", COMMIT, "deny", + check_in_ctx="gh pr comment 7") + print(f"{COUNT[0]} cases, {len(FAILURES)} failures") for f in FAILURES: print("FAIL", f) diff --git a/shared/workflow/claim-pr.md b/shared/workflow/claim-pr.md index 7b35911bb..eebcfb5aa 100644 --- a/shared/workflow/claim-pr.md +++ b/shared/workflow/claim-pr.md @@ -124,6 +124,7 @@ Every session that comments under one account shares one forge login, so a claim Add a `Session worktree:` line (the checkout path) or a `Session id:` line to the claim body. [`hooks/no-pr-work-without-claim.py`](../../hooks/no-pr-work-without-claim.py) denies a `git commit` or `git push` on a branch with an open Morrison-Lab PR unless one claim comment carries the agent marker and names this session. It also denies when a claim naming a different session is newer than yours, and before a push it warns about pushes since your claim that your local history lacks. +The hook's docstring lists its known limits: claim age and release comments are not evaluated, and a PR from a fork is not seen. The check reads `repos///activity?ref=refs/heads/`. Hooks are inert in remote and web sessions, so a cloud session relies on its own instructions to run the same two reads. From a3ed8d4d3da2df967afa9670f3a86e41ee1bc9d5 Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Wed, 30 Sep 2026 19:28:50 -0700 Subject: [PATCH 4/9] fix: second review round on the claim-required hook (#4155) - Accept every claim wording skills/ emits: marker plus `hold off` or `paws off`, minus an unclaim and a negated "no need to hold off". Drop the `working on this` requirement that rejected the ardi, handoff and review-only wordings. - Add the `Session worktree:` line to the skills/claim-pr PR template, the one the hook matches on; the other emitters are tracked in #4160. - Worktree match is bounded on both sides: a sentence-final period is fine, an extension of the path (either end) is not. - Judge a push by its refspec's destination branch, skip tag pushes. - URL-encode the branch in the pulls and activity queries; accept a remote URL with a trailing slash. - Cap the activity SHA checks and put every subprocess under one 20s budget (PR_CLAIM_TOTAL_BUDGET) below the 30s hook timeout. - Tests log each requested forge path and assert the head= and ref= queries; 69 cases, every mutant added this round killed by one targeted case. Co-Authored-By: Claude Sonnet 5.5 --- hooks/hooks.json | 2 +- hooks/no-pr-work-without-claim.py | 110 +++++++++++++++++++---- hooks/test-no-pr-work-without-claim.py | 70 ++++++++++++++- plugins/ai-config-hooks/hooks/hooks.json | 2 +- skills/claim-pr/SKILL.md | 5 ++ 5 files changed, 168 insertions(+), 21 deletions(-) diff --git a/hooks/hooks.json b/hooks/hooks.json index 439a4f44a..4b0e5aa19 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -456,7 +456,7 @@ "command": "python3 \"${CLAUDE_PLUGIN_ROOT}/hooks/no-pr-work-without-claim.py\"", "timeout": 30, "script": "no-pr-work-without-claim.py", - "why": "ai-config#4155, measured 2026-09-30: two agent sessions worked the same three PR branches at once and neither posted a claim, so a cloud session's restore commit on PR #4134 left memories/preferences.md at 706 lines instead of 1177. shared/workflow/claim-pr.md says to claim first and nothing enforced it; ListAgents cannot see a cloud session. DENIES a `git commit` or `git push` on a branch with an open Morrison-Lab PR unless a claim comment (agent marker plus hold-off wording) names THIS session by session id or worktree path, and denies when another session's claim is newer than yours. Before a push it WARNS (never denies) about pushes since your claim that are not in local history, read from the forge activity endpoint. Fails OPEN and says so on any network or gh failure. Local half only: hooks are inert in remote/web sessions (#2004). Clear with ALLOW_UNCLAIMED_PR_WORK=1. Timeout is 30s because it makes up to three forge reads." + "why": "ai-config#4155, measured 2026-09-30: two agent sessions worked the same three PR branches at once and neither posted a claim, so a cloud session's restore commit on PR #4134 left memories/preferences.md at 706 lines instead of 1177. shared/workflow/claim-pr.md says to claim first and nothing enforced it; ListAgents cannot see a cloud session. DENIES a `git commit` or `git push` on a branch with an open Morrison-Lab PR unless a claim comment (agent marker plus hold-off wording) names THIS session by session id or worktree path, and denies when another session's claim is newer than yours. Before a push it WARNS (never denies) about pushes since your claim that are not in local history, read from the forge activity endpoint. Fails OPEN and says so on any network or gh failure. Local half only: hooks are inert in remote/web sessions (#2004). Clear with ALLOW_UNCLAIMED_PR_WORK=1. Every subprocess shares one 20s budget (PR_CLAIM_TOTAL_BUDGET), under this 30s timeout, so a slow forge is a visible fail-open rather than a harness kill. The skills/claim-pr template now carries the Session worktree line the hook matches on; other claim emitters (ardi, handoff, gi, st, gip, pr-on-claim) do not yet, so their first commit is denied once with instructions." }, { "type": "command", diff --git a/hooks/no-pr-work-without-claim.py b/hooks/no-pr-work-without-claim.py index 31136c028..34d6a7dc6 100644 --- a/hooks/no-pr-work-without-claim.py +++ b/hooks/no-pr-work-without-claim.py @@ -60,6 +60,19 @@ visible fail-open, not a guess. Subshell scoping of a `cd` is not modelled (`simple_commands` flattens it). +A push is judged by its first refspec's destination branch (`git push origin +HEAD:foo` checks `foo`; a tag push is skipped); with no refspec it is the +checkout's current branch. A push of several branches is judged by the first. + +The activity warning compares each push's `after` SHA to local `HEAD`, so a +session that amended, rebased or force-pushed its own earlier pushes sees them +as foreign, and so does a main-sync merge pushed by the @claude bot. It is a +warning, never a deny, for that reason. At most MAX_ACTIVITY_CHECKS pushes are +examined. + +Every subprocess shares one TOTAL_BUDGET-second deadline (under the hooks.json +timeout); running out is a visible fail-open. + Cost: one to three forge reads per matched commit or push, uncached. Accepted because a stale cached "claimed" answer is exactly the failure being guarded. @@ -77,7 +90,9 @@ call's leading `export`, or in the process environment. It is for a case this guard did not foresee; reaching for it means saying why. -`PR_CLAIM_GH_CMD` replaces the `gh` executable (shlex-split); tests use it. +`PR_CLAIM_GH_CMD` replaces the `gh` executable (shlex-split) and +`PR_CLAIM_TOTAL_BUDGET` sets the shared subprocess budget in seconds +(default 20); tests use both. """ from __future__ import annotations @@ -87,13 +102,23 @@ import shlex import subprocess import sys +import time +from urllib.parse import quote OVERRIDE = "ALLOW_UNCLAIMED_PR_WORK" OWNERS = {"morrison-lab"} AGENT_MARKER = "posted by claude code (ai agent)" -CLAIM_PHRASES = ("hold off", "paws off") NET_TIMEOUT = 8 +# One budget for every subprocess the hook runs, kept under the hooks.json +# timeout (30s) so a slow forge is a visible fail-open rather than a harness +# kill. Set at the start of main(). +TOTAL_BUDGET = float(os.environ.get("PR_CLAIM_TOTAL_BUDGET", "20")) +_DEADLINE = [None] +MAX_ACTIVITY_CHECKS = 10 SKIP_BRANCHES = {"HEAD", "main", "master"} +# `git push` options that consume the NEXT token as a value. +PUSH_VALUE_OPTS = {"-o", "--push-option", "--repo", "--receive-pack", "--exec", + "--recurse-submodules", "--signed"} try: _LIB = os.path.join( @@ -189,8 +214,13 @@ def warn_open(message): def run(argv, cwd=None): + timeout = NET_TIMEOUT + if _DEADLINE[0] is not None: + timeout = min(timeout, _DEADLINE[0] - time.monotonic()) + if timeout <= 0: + raise TimeoutError(f"the hook's {TOTAL_BUDGET}s budget is spent") return subprocess.run(argv, cwd=cwd, capture_output=True, text=True, - timeout=NET_TIMEOUT) + timeout=timeout) def git(cwd, *args): @@ -230,12 +260,15 @@ def norm(text): def is_claim(body): """A claim comment: agent marker, `working on this`, and hold-off wording. - A release or status comment can contain "hold off" in other senses ("no - need to hold off"), so the opening claim phrase is required as well. + Every emitter in skills/ carries one of the two wordings (claim-pr, ardi, + handoff, and the review-only form all say `hold off`; the older emitters + say `paws off`), so that is the invariant. A release comment ("unclaiming") + and a negated mention ("no need to hold off") are not claims. """ low = (body or "").lower() - return (AGENT_MARKER in low and "working on this" in low - and any(p in low for p in CLAIM_PHRASES) + return (AGENT_MARKER in low + and re.search(r"(? MAX_ACTIVITY_CHECKS: + capped = True + break if run(["git", "merge-base", "--is-ancestor", after, "HEAD"], cwd=cwd).returncode == 0: continue @@ -374,6 +449,7 @@ def activity_warning(owner, repo, branch, since, cwd): return ("Pushes to this branch since your claim that are NOT in your local " "history (another session or person, or a force-push of yours): " + "; ".join(foreign[:5]) + + (" (more pushes not examined)" if capped else "") + ". Fetch and read them before pushing " "(shared/workflow/claim-pr.md; ai-config#4155).") @@ -394,11 +470,14 @@ def evaluate(payload): f"runs in ({exc})"} if hit is None: return None - verb, cwd = hit - branch = git(cwd, "rev-parse", "--abbrev-ref", "HEAD") + verb, cwd, target = hit + if target == "!skip": + return None + branch = (target if target not in (None, "HEAD") + else git(cwd, "rev-parse", "--abbrev-ref", "HEAD")) if not branch or branch in SKIP_BRANCHES: return None - m = re.search(r"github\.com[:/]([^/\s]+)/([^/\s]+?)(?:\.git)?$", + m = re.search(r"github\.com[:/]([^/\s]+)/([^/\s]+?)(?:\.git)?/?$", git(cwd, "remote", "get-url", "origin") or "") if not m or m.group(1).lower() not in OWNERS: return None @@ -408,7 +487,7 @@ def evaluate(payload): try: prs = gh_json(f"repos/{owner}/{repo}/pulls?state=open" - f"&head={owner}:{branch}") + f"&head={quote(owner + ':' + branch, safe=':/')}") if not prs: return None pr = prs[0] @@ -460,6 +539,7 @@ def evaluate(payload): def main(): + _DEADLINE[0] = time.monotonic() + TOTAL_BUDGET try: payload = json.load(sys.stdin) except Exception as exc: diff --git a/hooks/test-no-pr-work-without-claim.py b/hooks/test-no-pr-work-without-claim.py index 9fc8917f9..2891acf09 100644 --- a/hooks/test-no-pr-work-without-claim.py +++ b/hooks/test-no-pr-work-without-claim.py @@ -25,9 +25,13 @@ MARKER = "_Posted by Claude Code (AI agent) --- not written by a human._" SID = "session_test_AAAA" FAKE_GH = """\ -import json, os, sys -data = json.load(open(os.environ["FAKE_GH_DATA"])) +import json, os, sys, time +data = json.load(open(os.environ["FAKE_GH_DATA"], encoding="utf-8")) path = sys.argv[-1] +with open(os.environ["FAKE_GH_LOG"], "a", encoding="utf-8") as log: + log.write(path + "\\n") +if os.environ.get("FAKE_GH_SLEEP"): + time.sleep(float(os.environ["FAKE_GH_SLEEP"])) for needle, resp in data.items(): if needle in path: if resp == "FAIL": @@ -75,7 +79,8 @@ def claim(body_extra="", marker=True, phrase="hold off", at="2026-09-30T20:00:00 def run(name, command, expect, *, comments=None, prs="open", repo_kwargs=None, activity=None, env=None, stdin_raw=None, tool="Bash", branch="feat/x", - gh_fail=False, check_in_ctx=None, extra_payload=None, cwd_other=False): + gh_fail=False, check_in_ctx=None, extra_payload=None, cwd_other=False, + expect_paths=None): """expect: None (silent) | 'deny' | 'ctx' (additionalContext, no decision). `command` may be a callable (repo_path, other_path) -> str. `other` is a @@ -103,6 +108,8 @@ def run(name, command, expect, *, comments=None, prs="open", repo_kwargs=None, e = dict(os.environ) e.pop("ALLOW_UNCLAIMED_PR_WORK", None) e["FAKE_GH_DATA"] = data_path + log_path = os.path.join(tmp, "requests.log") + e["FAKE_GH_LOG"] = log_path e["PR_CLAIM_GH_CMD"] = (f'"{sys.executable.replace(chr(92), "/")}" ' f'"{fake_path.replace(chr(92), "/")}"') e.update(env or {}) @@ -117,6 +124,16 @@ def run(name, command, expect, *, comments=None, prs="open", repo_kwargs=None, got = ("deny" if spec.get("permissionDecision") == "deny" else "ctx" if spec.get("additionalContext") else None) ok = got == expect and r.returncode == 0 + if ok and expect_paths: + requested = "" + if os.path.exists(log_path): + with open(log_path, encoding="utf-8") as f: + requested = f.read() + ok = all(p in requested for p in expect_paths) + if not ok: + FAILURES.append(f"{name}: requested paths lacked " + f"{expect_paths}: {requested!r}") + return if ok and check_in_ctx: blob = (spec.get("additionalContext", "") + r.stderr + spec.get("permissionDecisionReason", "")) @@ -240,7 +257,8 @@ def mine(top): json.dump(data, f) with open(fp, "w", encoding="utf-8") as f: f.write(FAKE_GH) - e = dict(os.environ, FAKE_GH_DATA=dp, PR_CLAIM_GH_CMD=( + e = dict(os.environ, FAKE_GH_DATA=dp, + FAKE_GH_LOG=os.path.join(tmp, "requests.log"), PR_CLAIM_GH_CMD=( f'"{sys.executable.replace(chr(92), "/")}" "{fp.replace(chr(92), "/")}"')) e.pop("ALLOW_UNCLAIMED_PR_WORK", None) r = subprocess.run([sys.executable, SUBJECT], capture_output=True, @@ -304,6 +322,50 @@ def mine(top): run("R1-19 no claim at all: how to post one", COMMIT, "deny", check_in_ctx="gh pr comment 7") +# --- review round 2 -------------------------------------------------------- +# Claim wordings emitted elsewhere in skills/ must still count. +ardi_claim = lambda top: [{ + "body": f"Driving this PR to clean --- please hold off until done.\n\n" + f"Session id: `{SID}`\n\n{MARKER}", + "created_at": "2026-09-30T20:00:00Z", "html_url": "u"}] +review_claim = lambda top: [{ + "body": f"claude is reviewing this PR --- please hold off on pushing to " + f"this branch until the review comment lands.\n\nSession id: " + f"`{SID}`\n\n{MARKER}", + "created_at": "2026-09-30T20:00:00Z", "html_url": "u"}] +run("R2-1 ardi wording claim counts", COMMIT, None, comments=ardi_claim) +run("R2-2 review-only wording claim counts", COMMIT, None, + comments=review_claim) +# Path boundaries: sentence-final period is fine, a left-extension is not. +run("R2-3 worktree path ending a sentence (no backticks)", COMMIT, None, + comments=lambda top: [claim(f"Session worktree: {top}.")]) +run("R2-4 worktree path inside a longer path is not mine", COMMIT, "deny", + comments=lambda top: [claim(f"Session worktree: `x{top}`")]) +# Push refspec: the destination branch is the one checked. +run("R2-5 HEAD:refs/heads/foo checks foo", "git push origin HEAD:refs/heads/foo", + "deny", expect_paths=["head=Morrison-Lab:foo"]) +run("R2-6 plain refspec checks that branch", "git push origin other-branch", + "deny", expect_paths=["head=Morrison-Lab:other-branch"]) +run("R2-7 a tag push is not a branch", "git push origin refs/tags/v1", None) +run("R2-8 git push -u origin HEAD checks the current branch", + "git push -u origin HEAD", "deny", expect_paths=["head=Morrison-Lab:feat/x"]) +# URL encoding and remote shapes. +run("R2-9 branch with # is encoded in the pulls query", COMMIT, "deny", + branch="feat/a#b", expect_paths=["head=Morrison-Lab:feat/a%23b"]) +run("R2-10 branch with # is encoded in the activity query", "git push origin HEAD", + None, branch="feat/a#b", comments=mine, + expect_paths=["ref=refs/heads/feat/a%23b"]) +run("R2-11 remote URL with a trailing slash", COMMIT, "deny", + repo_kwargs={"remote": "https://github.com/Morrison-Lab/test-repo/"}) +# Activity cap and total budget. +many = [{"activity_type": "push", "timestamp": "2026-09-30T22:00:00Z", + "after": "d" * 40, "actor": {"login": "peer"}}] * 14 +run("R2-12 many foreign pushes are capped", PUSH, "ctx", comments=mine, + activity=many, check_in_ctx="more pushes not examined") +run("R2-13 a spent time budget is a visible fail-open", COMMIT, "ctx", + env={"PR_CLAIM_TOTAL_BUDGET": "1", "FAKE_GH_SLEEP": "4"}, + check_in_ctx="could not verify") + print(f"{COUNT[0]} cases, {len(FAILURES)} failures") for f in FAILURES: print("FAIL", f) diff --git a/plugins/ai-config-hooks/hooks/hooks.json b/plugins/ai-config-hooks/hooks/hooks.json index 2e190fdd1..799ba420d 100644 --- a/plugins/ai-config-hooks/hooks/hooks.json +++ b/plugins/ai-config-hooks/hooks/hooks.json @@ -449,7 +449,7 @@ "command": "${CLAUDE_PLUGIN_ROOT}/run-hook.sh 'python3 \"${CLAUDE_PLUGIN_ROOT}/../../hooks/no-pr-work-without-claim.py\"'", "timeout": 30, "script": "no-pr-work-without-claim.py", - "why": "ai-config#4155, measured 2026-09-30: two agent sessions worked the same three PR branches at once and neither posted a claim, so a cloud session's restore commit on PR #4134 left memories/preferences.md at 706 lines instead of 1177. shared/workflow/claim-pr.md says to claim first and nothing enforced it; ListAgents cannot see a cloud session. DENIES a `git commit` or `git push` on a branch with an open Morrison-Lab PR unless a claim comment (agent marker plus hold-off wording) names THIS session by session id or worktree path, and denies when another session's claim is newer than yours. Before a push it WARNS (never denies) about pushes since your claim that are not in local history, read from the forge activity endpoint. Fails OPEN and says so on any network or gh failure. Local half only: hooks are inert in remote/web sessions (#2004). Clear with ALLOW_UNCLAIMED_PR_WORK=1. Timeout is 30s because it makes up to three forge reads." + "why": "ai-config#4155, measured 2026-09-30: two agent sessions worked the same three PR branches at once and neither posted a claim, so a cloud session's restore commit on PR #4134 left memories/preferences.md at 706 lines instead of 1177. shared/workflow/claim-pr.md says to claim first and nothing enforced it; ListAgents cannot see a cloud session. DENIES a `git commit` or `git push` on a branch with an open Morrison-Lab PR unless a claim comment (agent marker plus hold-off wording) names THIS session by session id or worktree path, and denies when another session's claim is newer than yours. Before a push it WARNS (never denies) about pushes since your claim that are not in local history, read from the forge activity endpoint. Fails OPEN and says so on any network or gh failure. Local half only: hooks are inert in remote/web sessions (#2004). Clear with ALLOW_UNCLAIMED_PR_WORK=1. Every subprocess shares one 20s budget (PR_CLAIM_TOTAL_BUDGET), under this 30s timeout, so a slow forge is a visible fail-open rather than a harness kill. The skills/claim-pr template now carries the Session worktree line the hook matches on; other claim emitters (ardi, handoff, gi, st, gip, pr-on-claim) do not yet, so their first commit is denied once with instructions." }, { "type": "command", diff --git a/skills/claim-pr/SKILL.md b/skills/claim-pr/SKILL.md index 6d4279896..c2103edb3 100644 --- a/skills/claim-pr/SKILL.md +++ b/skills/claim-pr/SKILL.md @@ -52,12 +52,17 @@ Past 2 hours the claim has expired; re-post it before resuming. ```bash gh pr comment --body "Claude Code CLI (local session) is working on this — please hold off on pushing to this branch until I'm done. +Session worktree: + _Posted by Claude Code (AI agent) --- not written by a human._" # COMMENT_PR gh issue comment --body "Claude Code CLI (local session) is working on this — please hold off until I'm done. _Posted by Claude Code (AI agent) --- not written by a human._" # COMMENT_ISSUE ``` +The `Session worktree:` line (or a `Session id:` line) is what lets `hooks/no-pr-work-without-claim.py` tell this session's claim from another session's under the same login; +without it the hook denies the first `git commit` on the PR branch. + A review-only session uses the same `hold off` invariant so existing detectors still match, and names the review so authors know when they can push again: From 841c98c20c23a2f39c5ae047bc839b583c87d593 Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Wed, 30 Sep 2026 19:58:08 -0700 Subject: [PATCH 5/9] fix: third review round on the claim-required hook (#4155) - A claim that names no session now WARNS instead of denying, so the claim emitters that predate the session line (#4160) keep working; only no claim, or claims naming a different session, deny. Deny text no longer implies a lapse/release check the hook does not make. - Read a Git Bash `/c/x` path as `C:/x` on both sides of the worktree match; the claim-pr template asks for `git rev-parse --show-toplevel`. - Scan commands nested in a shell's `-c`: unlocatable, so a visible fail-open rather than a silent bypass. - Skip option values (`-m`, `-am msg`, ...) when reading `--dry-run`; stop treating `--signed` / `--recurse-submodules` as value-taking. - Skip pushes the lookup cannot judge: colon-refspec deletes, `--tags` / `--all` / `--mirror`, and non-origin remotes. - Document the shared-checkout, unfetched-commit and shallow-clone limits. - 80 cases; real-commit ancestry cases replace the made-up SHA; each mutant added this round is killed by a targeted case. Co-Authored-By: Claude Sonnet 5.5 --- README.md | 2 +- hooks/hooks.json | 2 +- hooks/no-pr-work-without-claim.py | 110 ++++++++++++++++++----- hooks/test-no-pr-work-without-claim.py | 110 +++++++++++++++++------ plugins/ai-config-hooks/hooks/hooks.json | 2 +- shared/workflow/claim-pr.md | 3 +- skills/claim-pr/SKILL.md | 2 +- 7 files changed, 176 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index 24a820684..4fc6cc41d 100644 --- a/README.md +++ b/README.md @@ -502,7 +502,7 @@ The payload gaps that remain and the per-guard status are in | `flag-stale-adjacent-comment.py` | `PreToolUse` (Bash) | warns, never blocks, when a `git commit` changes a literal value while an unchanged comment within ten lines still asserts the old one | | `warn-unparseable-staged-config.py` | `PreToolUse` (Bash) | warns when `git commit` would commit staged `.toml`, `.json`, or `.yaml`/`.yml` config files that fail parser validation (`tomllib`, `json`, `yaml.safe_load`) | | `no-delete-branch-under-stacked-pr.py` | `PreToolUse` (Bash) | warns when `gh pr merge --delete-branch` or `gh pr close --delete-branch` would delete a branch that is an open PR's base. GitHub's documented behaviour is to retarget such a PR, but a measured case closed it instead, and a closed PR can be neither retargeted nor reopened while its base is gone. Silent when nothing is stacked, when the query fails or returns an unexpected shape, when `gh` is absent, when the command carries no `-R` or PR target, and on `--delete-branch=false` | -| `no-pr-work-without-claim.py` | `PreToolUse` (Bash) | denies a `git commit` or `git push` on a branch whose open Morrison-Lab PR has no claim comment (agent marker plus hold-off wording) naming this session by session id or worktree path, and denies when another session's claim is newer than yours. Before a push it warns, never denies, about pushes since your claim that local history lacks, read from the forge activity endpoint. Fails open and says so on any `gh` or network failure. A claim's age and release comments are not evaluated, and a fork PR is not seen. Local half of ai-config#4155: hooks are inert in remote and web sessions. | +| `no-pr-work-without-claim.py` | `PreToolUse` (Bash) | denies a `git commit` or `git push` on a branch whose open Morrison-Lab PR has no claim comment (agent marker plus hold-off wording) naming this session by session id or worktree path (a claim naming no session only warns, so existing claim flows keep working until #4160), and denies when another session's claim is newer than yours. Before a push it warns, never denies, about pushes since your claim that local history lacks (including any this clone has not fetched), read from the forge activity endpoint. Fails open and says so on any `gh` or network failure. A claim's age and release comments are not evaluated, and a fork PR is not seen. Local half of ai-config#4155: hooks are inert in remote and web sessions. | | `no-clobbering-push.py` | `PreToolUse` (Bash) | refuses a bare `git push --force`/`-f`, whose remedy (`--force-with-lease --force-if-includes`) costs one word. Warns on every other push whose remote tip a live, read-only `git ls-remote` shows is not an ancestor of the ref being pushed (which is `HEAD` only when the refspec says so, resolved in the directory the push runs in rather than the session's -- a `cd`, scoped to its subshell but not to a brace group, and declined where a compound statement's body, a short-circuited alternative, or a fork into a background job or a pipeline means the pushing shell never takes its effect, then the push's own `-C`, declined in turn when the shell would have had to expand it), names that directory and qualifies its remediation commands with `git -C` when it is not the call's own, declines the reading when the directory is indeterminate or `--git-dir`/`--work-tree`/`GIT_DIR=` redirected the repository, and stays silent on a fast-forward | | `no-commit-chained-to-push.py` | `PreToolUse` (Bash) | denies a Bash call that chains a `git commit` into a later `git push`. A PreToolUse deny rejects the whole invocation, so a guard refusing the push discards the commit too while its message speaks only about the push (ai-config#2992). Denies rather than warns because the refusal stops the chain reaching the sibling guards at all, and its remedy -- two Bash calls -- is always available. Clearable with `ALLOW_COMMIT_AND_PUSH=1`, either prefixing the commit or push or as the call's own leading assignment (a subshell or short-circuited one sets nothing and does not count). Matches over an argv split (`scripts/lib/shellcmd.py`), so a quoted commit message, a heredoc body and `git commit-tree` cannot trip it, while `timeout 60 git push`, `/usr/bin/git push` and `{ git commit; } && git push` all resolve -- the guard has to fire wherever its siblings would. There is no exemption for a `--dry-run` or `--delete` command: one was written and removed after a review measured `git commit ... && git push --force --delete` and `... --dry-run --no-dry-run --force` both going silent while `no-clobbering-push.py` denied them | | `flag-chained-push.py` | `PreToolUse` (Bash) | warns, never blocks, when a `git push` is chained after another command with `&&`, `;`, or `\|\|`, piped onward with `\|`, or suffixed by a redirection (`>`, `>>`, or an fd form like `2>&1`). `no-clobbering-push.py` and the plugin's own `no-push-without-self-review.py` push policy both parse the WHOLE command text for a push rather than the isolated segment, so a trailing `2>&1` hands either parser a bare `2` sitting where a commit-ish token would sit in other shapes, and a chained prefix reads as part of the same invocation; a refused chain runs NOTHING, which the refusal naming only the push invites the author to misread as the prefix having succeeded. Measured on Lacaedemon/sparta, 2026-09-05: three refusals in one session | diff --git a/hooks/hooks.json b/hooks/hooks.json index 4b0e5aa19..5fd84a08b 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -456,7 +456,7 @@ "command": "python3 \"${CLAUDE_PLUGIN_ROOT}/hooks/no-pr-work-without-claim.py\"", "timeout": 30, "script": "no-pr-work-without-claim.py", - "why": "ai-config#4155, measured 2026-09-30: two agent sessions worked the same three PR branches at once and neither posted a claim, so a cloud session's restore commit on PR #4134 left memories/preferences.md at 706 lines instead of 1177. shared/workflow/claim-pr.md says to claim first and nothing enforced it; ListAgents cannot see a cloud session. DENIES a `git commit` or `git push` on a branch with an open Morrison-Lab PR unless a claim comment (agent marker plus hold-off wording) names THIS session by session id or worktree path, and denies when another session's claim is newer than yours. Before a push it WARNS (never denies) about pushes since your claim that are not in local history, read from the forge activity endpoint. Fails OPEN and says so on any network or gh failure. Local half only: hooks are inert in remote/web sessions (#2004). Clear with ALLOW_UNCLAIMED_PR_WORK=1. Every subprocess shares one 20s budget (PR_CLAIM_TOTAL_BUDGET), under this 30s timeout, so a slow forge is a visible fail-open rather than a harness kill. The skills/claim-pr template now carries the Session worktree line the hook matches on; other claim emitters (ardi, handoff, gi, st, gip, pr-on-claim) do not yet, so their first commit is denied once with instructions." + "why": "ai-config#4155, measured 2026-09-30: two agent sessions worked the same three PR branches at once and neither posted a claim, so a cloud session's restore commit on PR #4134 left memories/preferences.md at 706 lines instead of 1177. shared/workflow/claim-pr.md says to claim first and nothing enforced it; ListAgents cannot see a cloud session. DENIES a `git commit` or `git push` on a branch with an open Morrison-Lab PR unless a claim comment (agent marker plus hold-off wording) names THIS session by session id or worktree path, and denies when another session's claim is newer than yours. Before a push it WARNS (never denies) about pushes since your claim that are not in local history, read from the forge activity endpoint. Fails OPEN and says so on any network or gh failure. Local half only: hooks are inert in remote/web sessions (#2004). Clear with ALLOW_UNCLAIMED_PR_WORK=1. Every subprocess shares one 20s budget (PR_CLAIM_TOTAL_BUDGET), under this 30s timeout, so a slow forge is a visible fail-open rather than a harness kill. The skills/claim-pr template now carries the Session worktree line the hook matches on; the other claim emitters (ardi, handoff, gi, st, gip, pr-on-claim, tracked in #4160) do not yet, so a claim that names no session only WARNS (it may be the session's own) instead of denying. A commit nested in `bash -c` is a visible fail-open, and two sessions sharing one checkout cannot be told apart (known limits in the hook docstring)." }, { "type": "command", diff --git a/hooks/no-pr-work-without-claim.py b/hooks/no-pr-work-without-claim.py index 34d6a7dc6..964c581db 100644 --- a/hooks/no-pr-work-without-claim.py +++ b/hooks/no-pr-work-without-claim.py @@ -24,9 +24,13 @@ agent)`) and the `hold off` (or legacy `paws off`) wording, per `skills/claim-pr/SKILL.md`; * it is THIS session's when its body contains the payload's `session_id` or - this worktree's path. claim-pr's stock wording carries neither -- the - forge login is shared by every session under one account, so the comment - must say which session it is. The deny text gives the one-line addition. + this worktree's path (`/c/x` and `C:/x` spellings are equal). The forge + login is shared by every session under one account, so the comment must + say which session it is; the skills/claim-pr template now does. + * no claim at all, or only claims naming a DIFFERENT session: DENY. + * only claims that name NO session (every other emitter's template, until + ai-config#4160 lands): WARN, because such a claim may be this session's + own and a hard deny would block every existing claim flow. * a claim from a DIFFERENT session posted AFTER this session's latest claim is a takeover: DENY. (A newer claim that names no session at all is not attributable, so it only warns.) @@ -51,6 +55,15 @@ comment containing `unclaim` from counting as a claim. A claim whose session later released it still satisfies this check. Known limit. +Session identity is the worktree path (the model rarely knows its own +`session_id`), so two sessions sharing ONE checkout cannot be told apart, and +neither can an isolated subagent worktree from its parent. Known limit; it is +the same-checkout shape of the 2026-09-30 incident that this hook cannot see. + +A commit or push nested in a shell's `-c` starts in a directory this scan +cannot know, so it is a visible fail-open (not evaluated) unless the piece +`cd`s to an absolute path first. + A PR from a fork (head repo under another owner) is not found by the `head=:` query, so the hook stays silent for it. Known limit. @@ -66,7 +79,9 @@ The activity warning compares each push's `after` SHA to local `HEAD`, so a session that amended, rebased or force-pushed its own earlier pushes sees them -as foreign, and so does a main-sync merge pushed by the @claude bot. It is a +as foreign, and so does a main-sync merge pushed by the @claude bot, and so +does any push whose commit this clone has not fetched (a stale or shallow +clone). It is a warning, never a deny, for that reason. At most MAX_ACTIVITY_CHECKS pushes are examined. @@ -116,9 +131,16 @@ _DEADLINE = [None] MAX_ACTIVITY_CHECKS = 10 SKIP_BRANCHES = {"HEAD", "main", "master"} -# `git push` options that consume the NEXT token as a value. -PUSH_VALUE_OPTS = {"-o", "--push-option", "--repo", "--receive-pack", "--exec", - "--recurse-submodules", "--signed"} +# Options that consume the NEXT token as a value when written without `=`. +# (`git push --signed` and `--recurse-submodules` take their value attached +# with `=` only, so they are deliberately absent.) +PUSH_VALUE_OPTS = {"-o", "--push-option", "--repo", "--receive-pack", "--exec"} +COMMIT_VALUE_OPTS = {"-m", "--message", "-F", "--file", "-C", "--reuse-message", + "-c", "--reedit-message", "--author", "--date", + "--cleanup", "-t", "--template", "--fixup", "--squash"} +# A short-option cluster ending in one of these takes the next token as value +# (`git commit -am "msg"`). +SHORT_VALUE_LETTERS = {"commit": "mFCct", "push": "o"} try: _LIB = os.path.join( @@ -127,11 +149,12 @@ if _LIB not in sys.path: sys.path.insert(0, _LIB) from shellcmd import (GIT_VALUE_OPTS, env_value, resolve_cd_target, - simple_commands, strip_env) + shell_c_expansions, simple_commands, strip_env) except Exception as _exc: # broken install: fail open, and say so print(f"no-pr-work-without-claim: cannot load scripts/lib/shellcmd.py " f"({_exc}); not evaluating", file=sys.stderr) env_value = simple_commands = strip_env = resolve_cd_target = None + shell_c_expansions = None GIT_VALUE_OPTS = frozenset() # Raised when the directory a command runs in cannot be named statically. @@ -165,17 +188,15 @@ class Indeterminate(Exception): """ DENY_PEER_CLAIM = """\ -`git {verb}` on branch `{branch}` ({pr_url}): the PR has a claim that does not -name this session, and this session has none of its own. +`git {verb}` on branch `{branch}` ({pr_url}): the PR has a claim naming a +different session, and this session has none of its own. that claim: {peer_url} ({peer_at}) -It is either another session's, or your own posted without a `Session -worktree:` / `Session id:` line (re-post it with one). If it is another -session's, that session is working this branch now. Do not post a competing -claim over it and do not commit: stand down, or take over only after theirs has lapsed -or been released (shared/workflow/claim-pr.md, 2-hour rule), by posting a fresh -claim that names this session. +That session is working this branch now. Do not post a competing claim over +it and do not commit: stand down. To take over, first confirm theirs has +lapsed or been released (shared/workflow/claim-pr.md, 2-hour rule; this hook +does not check either), then post a fresh claim that names this session. {override}=1 clears this refusal; say why. """ @@ -189,7 +210,8 @@ class Indeterminate(Exception): That session is the live owner now. Stop and read their claim before touching the branch; take over only by posting a fresh claim of your own once theirs has -lapsed or been released (shared/workflow/claim-pr.md, 2-hour rule). +lapsed or been released (shared/workflow/claim-pr.md, 2-hour rule; this hook +checks neither). {override}=1 clears this refusal; say why. """ @@ -254,7 +276,14 @@ def gh_json(path, paginate=True): def norm(text): - return text.replace("\\", "/").lower() + """Lowercase, forward slashes, and Git Bash `/c/x` read as `c:/x`. + + Applied to the claim body and the worktree path alike, so a session that + wrote its path from `pwd` in Git Bash still matches `git rev-parse + --show-toplevel`'s `C:/x` form. + """ + text = text.replace("\\", "/").lower() + return re.sub(r"(?//activity?ref=refs/heads/`. Hooks are inert in remote and web sessions, so a cloud session relies on its own instructions to run the same two reads. diff --git a/skills/claim-pr/SKILL.md b/skills/claim-pr/SKILL.md index c2103edb3..7e7860787 100644 --- a/skills/claim-pr/SKILL.md +++ b/skills/claim-pr/SKILL.md @@ -52,7 +52,7 @@ Past 2 hours the claim has expired; re-post it before resuming. ```bash gh pr comment --body "Claude Code CLI (local session) is working on this — please hold off on pushing to this branch until I'm done. -Session worktree: +Session worktree: _Posted by Claude Code (AI agent) --- not written by a human._" # COMMENT_PR gh issue comment --body "Claude Code CLI (local session) is working on this — please hold off until I'm done. From 5258485792182d9cdec9d21dab84833f4de2b1d2 Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Wed, 30 Sep 2026 20:28:00 -0700 Subject: [PATCH 6/9] fix: fourth review round on the claim-required hook (#4155) - A shell-built push destination (`$BRANCH`, `$(...)`) falls back to the current branch instead of being looked up as a literal name, which found no PR and passed silently. - The push activity warning now also runs when the only claim names no session (it uses that claim's time as the baseline). - Apply claim-pr's 2-hour rule to a PEER claim through the PR's updated_at: a peer claim on a PR idle that long warns instead of denying. - Recognize "releasing my claim" style releases as non-claims. - A broken shellcmd import is a visible fail-open on a git command, not a silent no-op; accept `github.com-` SSH remotes. - skills/claim-pr/SKILL.md no longer says the hook denies a session-less claim (it warns). - 88 cases; each mutant added this round killed by a targeted case. Co-Authored-By: Claude Sonnet 5.5 --- README.md | 2 +- hooks/no-pr-work-without-claim.py | 114 ++++++++++++++++++------- hooks/test-no-pr-work-without-claim.py | 53 +++++++++++- shared/workflow/claim-pr.md | 2 +- skills/claim-pr/SKILL.md | 2 +- 5 files changed, 135 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index 4fc6cc41d..47c8fc547 100644 --- a/README.md +++ b/README.md @@ -502,7 +502,7 @@ The payload gaps that remain and the per-guard status are in | `flag-stale-adjacent-comment.py` | `PreToolUse` (Bash) | warns, never blocks, when a `git commit` changes a literal value while an unchanged comment within ten lines still asserts the old one | | `warn-unparseable-staged-config.py` | `PreToolUse` (Bash) | warns when `git commit` would commit staged `.toml`, `.json`, or `.yaml`/`.yml` config files that fail parser validation (`tomllib`, `json`, `yaml.safe_load`) | | `no-delete-branch-under-stacked-pr.py` | `PreToolUse` (Bash) | warns when `gh pr merge --delete-branch` or `gh pr close --delete-branch` would delete a branch that is an open PR's base. GitHub's documented behaviour is to retarget such a PR, but a measured case closed it instead, and a closed PR can be neither retargeted nor reopened while its base is gone. Silent when nothing is stacked, when the query fails or returns an unexpected shape, when `gh` is absent, when the command carries no `-R` or PR target, and on `--delete-branch=false` | -| `no-pr-work-without-claim.py` | `PreToolUse` (Bash) | denies a `git commit` or `git push` on a branch whose open Morrison-Lab PR has no claim comment (agent marker plus hold-off wording) naming this session by session id or worktree path (a claim naming no session only warns, so existing claim flows keep working until #4160), and denies when another session's claim is newer than yours. Before a push it warns, never denies, about pushes since your claim that local history lacks (including any this clone has not fetched), read from the forge activity endpoint. Fails open and says so on any `gh` or network failure. A claim's age and release comments are not evaluated, and a fork PR is not seen. Local half of ai-config#4155: hooks are inert in remote and web sessions. | +| `no-pr-work-without-claim.py` | `PreToolUse` (Bash) | denies a `git commit` or `git push` on a branch whose open Morrison-Lab PR has no claim comment (agent marker plus hold-off wording) naming this session by session id or worktree path (a claim naming no session only warns, so existing claim flows keep working until #4160), and denies when another session's claim is newer than yours. Before a push it warns, never denies, about pushes since your claim that local history lacks (including any this clone has not fetched), read from the forge activity endpoint. Fails open and says so on any `gh` or network failure. An own claim's age and most release wordings are not evaluated (a peer claim on a PR idle over 2 hours only warns), and a fork PR is not seen. Local half of ai-config#4155: hooks are inert in remote and web sessions. | | `no-clobbering-push.py` | `PreToolUse` (Bash) | refuses a bare `git push --force`/`-f`, whose remedy (`--force-with-lease --force-if-includes`) costs one word. Warns on every other push whose remote tip a live, read-only `git ls-remote` shows is not an ancestor of the ref being pushed (which is `HEAD` only when the refspec says so, resolved in the directory the push runs in rather than the session's -- a `cd`, scoped to its subshell but not to a brace group, and declined where a compound statement's body, a short-circuited alternative, or a fork into a background job or a pipeline means the pushing shell never takes its effect, then the push's own `-C`, declined in turn when the shell would have had to expand it), names that directory and qualifies its remediation commands with `git -C` when it is not the call's own, declines the reading when the directory is indeterminate or `--git-dir`/`--work-tree`/`GIT_DIR=` redirected the repository, and stays silent on a fast-forward | | `no-commit-chained-to-push.py` | `PreToolUse` (Bash) | denies a Bash call that chains a `git commit` into a later `git push`. A PreToolUse deny rejects the whole invocation, so a guard refusing the push discards the commit too while its message speaks only about the push (ai-config#2992). Denies rather than warns because the refusal stops the chain reaching the sibling guards at all, and its remedy -- two Bash calls -- is always available. Clearable with `ALLOW_COMMIT_AND_PUSH=1`, either prefixing the commit or push or as the call's own leading assignment (a subshell or short-circuited one sets nothing and does not count). Matches over an argv split (`scripts/lib/shellcmd.py`), so a quoted commit message, a heredoc body and `git commit-tree` cannot trip it, while `timeout 60 git push`, `/usr/bin/git push` and `{ git commit; } && git push` all resolve -- the guard has to fire wherever its siblings would. There is no exemption for a `--dry-run` or `--delete` command: one was written and removed after a review measured `git commit ... && git push --force --delete` and `... --dry-run --no-dry-run --force` both going silent while `no-clobbering-push.py` denied them | | `flag-chained-push.py` | `PreToolUse` (Bash) | warns, never blocks, when a `git push` is chained after another command with `&&`, `;`, or `\|\|`, piped onward with `\|`, or suffixed by a redirection (`>`, `>>`, or an fd form like `2>&1`). `no-clobbering-push.py` and the plugin's own `no-push-without-self-review.py` push policy both parse the WHOLE command text for a push rather than the isolated segment, so a trailing `2>&1` hands either parser a bare `2` sitting where a commit-ish token would sit in other shapes, and a chained prefix reads as part of the same invocation; a refused chain runs NOTHING, which the refusal naming only the push invites the author to misread as the prefix having succeeded. Measured on Lacaedemon/sparta, 2026-09-05: three refusals in one session | diff --git a/hooks/no-pr-work-without-claim.py b/hooks/no-pr-work-without-claim.py index 964c581db..22a3eb512 100644 --- a/hooks/no-pr-work-without-claim.py +++ b/hooks/no-pr-work-without-claim.py @@ -47,13 +47,21 @@ (ai-config#2004), so a cloud session is only covered by its own instructions or a server-side check. This is the local half of #4155 only. -A claim's 2-hour expiry (claim-pr.md) is not evaluated. An own claim is -accepted whatever its age; staleness over-approximation via `updatedAt` would -mark every session's own pushes as fresh activity. Known limit. - -A release ("unclaiming") comment is not modelled either: it only keeps a -comment containing `unclaim` from counting as a claim. A claim whose session -later released it still satisfies this check. Known limit. +claim-pr.md's 2-hour expiry is applied to a PEER's claim only, through the +PR's `updated_at` (which over-approximates freshness, so a stale verdict is +definitive): when the only claim names another session and the PR has shown no +activity for 2 hours, the hook warns instead of denying. An OWN claim is +accepted whatever its age, because every session's own pushes would otherwise +count as fresh activity. Known limit. + +A release comment is not modelled beyond keeping an "unclaiming" / "releasing +my claim" comment from counting as a claim. A claim whose session later +released it with other wording still satisfies this check. Known limit. + +Push destinations built by the shell (`$BRANCH`, `$(git branch --show-current)`) +cannot be read statically and fall back to the checkout's current branch. A +remote URL whose host is an SSH alias beginning `github.com` is accepted; any +other alias is not matched and the hook stays silent. Session identity is the worktree path (the model rarely knows its own `session_id`), so two sessions sharing ONE checkout cannot be told apart, and @@ -118,6 +126,7 @@ import subprocess import sys import time +from datetime import datetime, timezone from urllib.parse import quote OVERRIDE = "ALLOW_UNCLAIMED_PR_WORK" @@ -130,6 +139,8 @@ TOTAL_BUDGET = float(os.environ.get("PR_CLAIM_TOTAL_BUDGET", "20")) _DEADLINE = [None] MAX_ACTIVITY_CHECKS = 10 +# claim-pr.md: a claim is live for 2 hours from the PR's last push or comment. +STALE_HOURS = 2 SKIP_BRANCHES = {"HEAD", "main", "master"} # Options that consume the NEXT token as a value when written without `=`. # (`git push --signed` and `--recurse-submodules` take their value attached @@ -287,18 +298,37 @@ def norm(text): def is_claim(body): - """A claim comment: agent marker, `working on this`, and hold-off wording. + """A claim comment: the agent marker plus hold-off wording. Every emitter in skills/ carries one of the two wordings (claim-pr, ardi, handoff, and the review-only form all say `hold off`; the older emitters - say `paws off`), so that is the invariant. A release comment ("unclaiming") - and a negated mention ("no need to hold off") are not claims. + say `paws off`), so that is the invariant. A release comment ("unclaiming", + "releasing my claim") and a negated mention ("no need to hold off") are + not claims. """ low = (body or "").lower() return (AGENT_MARKER in low and re.search(r"(? STALE_HOURS * 3600 def names_session(body, session_id, worktree): @@ -464,7 +494,9 @@ def push_target(args): if spec.startswith(":"): return "!skip" # `:old` deletes a remote branch dst = spec.split(":", 1)[1] if ":" in spec else spec - if dst in ("", "HEAD"): + if dst in ("", "HEAD") or any(c in dst for c in "$`("): + # `HEAD`, or a name built by the shell (`$BRANCH`, `$(git branch + # --show-current)`) that cannot be read statically: the current branch. return "HEAD" if dst.startswith("refs/heads/"): return dst[len("refs/heads/"):] @@ -517,6 +549,11 @@ def evaluate(payload): if not isinstance(command, str) or not command.strip(): return None if simple_commands is None: + # A broken install must not be a silent bypass; only commands that + # mention git are worth a warning. + if re.search(r"\bgit\b", command): + return {"open_error": "scripts/lib/shellcmd.py could not be " + "loaded, so this command was not examined"} return None if os.environ.get(OVERRIDE) == "1" or LEADING_OVERRIDE.match(command): return None @@ -534,7 +571,7 @@ def evaluate(payload): else git(cwd, "rev-parse", "--abbrev-ref", "HEAD")) if not branch or branch in SKIP_BRANCHES: return None - m = re.search(r"github\.com[:/]([^/\s]+)/([^/\s]+?)(?:\.git)?/?$", + m = re.search(r"github\.com[\w.-]*[:/]([^/\s]+)/([^/\s]+?)(?:\.git)?/?$", git(cwd, "remote", "get-url", "origin") or "") if not m or m.group(1).lower() not in OWNERS: return None @@ -553,47 +590,58 @@ def evaluate(payload): mine, theirs, anonymous = classify_claims(comments, session_id, worktree) pr_url = pr.get("html_url") or f"#{pr['number']}" - if not mine and theirs: + notes = [] + if not mine and theirs and pr_is_stale(pr): + # claim-pr.md's 2-hour rule: a claim on a PR idle that long has + # lapsed. `updated_at` over-approximates freshness, so "stale" is + # definitive; a PR touched since then keeps the deny below. + notes.append( + f"The only claim on {pr_url} names another session, but the " + f"PR has shown no activity for over {STALE_HOURS} hours, so " + f"it is treated as lapsed. Post your own claim naming this " + f"session (`Session worktree: {worktree}`) before continuing.") + since = max(c["created_at"] for c in theirs) + elif not mine and theirs: peer = max(theirs, key=lambda c: c["created_at"]) return {"decision": "deny", "reason": DENY_PEER_CLAIM.format( verb=verb, branch=branch, pr_url=pr_url, peer_url=peer.get("html_url", "?"), peer_at=peer["created_at"], override=OVERRIDE)} - if not mine and anonymous: + elif not mine and anonymous: # A claim that names no session may be this session's own, posted # with a template that predates the session line (ai-config#4160). # Unattributable, so it warns rather than denies: a hard deny here # would block every existing claim flow on first commit. - return {"context": ( + notes.append( f"The PR {pr_url} has a claim that names no session " f"({anonymous[-1].get('html_url', '?')}). It may be yours or " f"another session's, and this hook cannot tell. Re-post it " f"with a `Session worktree: {worktree}` line so it can be " - f"attributed; if it is another session's, stand down.")} - if not mine: + f"attributed; if it is another session's, stand down.") + since = max(c["created_at"] for c in anonymous) + elif not mine: return {"decision": "deny", "reason": DENY_NO_CLAIM.format( verb=verb, branch=branch, pr_url=pr_url, pr_number=pr["number"], worktree=worktree, session_id=session_id or "", override=OVERRIDE)} - latest_mine = max(c["created_at"] for c in mine) - newer = [c for c in theirs if c["created_at"] > latest_mine] - if newer: - t = max(newer, key=lambda c: c["created_at"]) - return {"decision": "deny", "reason": DENY_SUPERSEDED.format( - verb=verb, branch=branch, pr_url=pr_url, mine_at=latest_mine, - theirs_url=t.get("html_url", "?"), theirs_at=t["created_at"], - override=OVERRIDE)} - notes = [] - if any(c["created_at"] > latest_mine for c in anonymous): - notes.append("A claim that names no session was posted after " - "yours; it may be another session's. Read the PR " - "comments before continuing.") + else: + since = max(c["created_at"] for c in mine) + newer = [c for c in theirs if c["created_at"] > since] + if newer: + t = max(newer, key=lambda c: c["created_at"]) + return {"decision": "deny", "reason": DENY_SUPERSEDED.format( + verb=verb, branch=branch, pr_url=pr_url, mine_at=since, + theirs_url=t.get("html_url", "?"), + theirs_at=t["created_at"], override=OVERRIDE)} + if any(c["created_at"] > since for c in anonymous): + notes.append("A claim that names no session was posted after " + "yours; it may be another session's. Read the PR " + "comments before continuing.") if verb == "push": # Its own try: a 403/404 from the activity endpoint (it needs push # access) must not discard the claim outcomes already computed. try: - warning = activity_warning(owner, repo, branch, latest_mine, - cwd) + warning = activity_warning(owner, repo, branch, since, cwd) except Exception as exc: warning = (f"could not read the forge activity for this " f"branch ({type(exc).__name__}: {exc}); check for " diff --git a/hooks/test-no-pr-work-without-claim.py b/hooks/test-no-pr-work-without-claim.py index 81dfd61fd..f812620f4 100644 --- a/hooks/test-no-pr-work-without-claim.py +++ b/hooks/test-no-pr-work-without-claim.py @@ -81,7 +81,7 @@ def claim(body_extra="", marker=True, phrase="hold off", at="2026-09-30T20:00:00 def run(name, command, expect, *, comments=None, prs="open", repo_kwargs=None, activity=None, env=None, stdin_raw=None, tool="Bash", branch="feat/x", gh_fail=False, check_in_ctx=None, extra_payload=None, cwd_other=False, - expect_paths=None): + expect_paths=None, pr_updated_at=None): """expect: None (silent) | 'deny' | 'ctx' (additionalContext, no decision). `command` may be a callable (repo_path, other_path) -> str. `other` is a @@ -96,7 +96,9 @@ def run(name, command, expect, *, comments=None, prs="open", repo_kwargs=None, if callable(comments): comments = comments(top) pr_list = ([{"number": 7, "html_url": "https://github.com/Morrison-Lab/" - "test-repo/pull/7"}] if prs == "open" else []) + "test-repo/pull/7", + **({"updated_at": pr_updated_at} if pr_updated_at + else {})}] if prs == "open" else []) data = {"pulls?state=open": "FAIL" if gh_fail else pr_list, "/comments": comments or [], "/activity": activity if activity is not None else []} @@ -418,6 +420,53 @@ def side_branch_sha(repo): "git push --signed origin feat/x", "deny", expect_paths=["head=Morrison-Lab:feat/x"]) +# --- review round 4 -------------------------------------------------------- +from datetime import datetime, timezone # noqa: E402 + +NOW = datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") +peer = lambda top: [claim("Session id: `session_other_BBBB`")] +run("R4-1 push to a $VAR destination falls back to the current branch", + 'git push origin "$BRANCH"', "deny", + expect_paths=["head=Morrison-Lab:feat/x"]) +run("R4-2 HEAD:$B destination falls back too", "git push origin HEAD:$B", + "deny", expect_paths=["head=Morrison-Lab:feat/x"]) +run("R4-3 anonymous-only claim still gets the push activity warning", PUSH, + "ctx", comments=[claim()], activity=foreign, + check_in_ctx="NOT in your local history") +run("R4-4 peer claim on a PR idle for years is treated as lapsed", COMMIT, + "ctx", comments=peer, pr_updated_at="2020-01-01T00:00:00Z", + check_in_ctx="treated as lapsed") +run("R4-5 peer claim on a PR touched just now still denies", COMMIT, "deny", + comments=peer, pr_updated_at=NOW) +run("R4-6 releasing-my-claim comment is not a claim", COMMIT, "deny", + comments=[{"body": f"Releasing my claim, hold off no more. Session id: " + f"`{SID}`\n\n{MARKER}", + "created_at": "2026-09-30T20:00:00Z", "html_url": "u"}]) +run("R4-7 ssh host-alias remote", COMMIT, "deny", + repo_kwargs={"remote": "git@github.com-work:Morrison-Lab/test-repo.git"}) + +# A broken install (no scripts/lib beside the hook) must be visible, not inert. +COUNT[0] += 1 +with tempfile.TemporaryDirectory() as tmp: + os.makedirs(os.path.join(tmp, "hooks")) + lone = os.path.join(tmp, "hooks", "no-pr-work-without-claim.py") + with open(SUBJECT, encoding="utf-8") as src, open( + lone, "w", encoding="utf-8") as dst: + dst.write(src.read()) + r = subprocess.run([sys.executable, lone], capture_output=True, text=True, + input=json.dumps({"tool_name": "Bash", "cwd": tmp, + "tool_input": {"command": COMMIT}})) + quiet = subprocess.run([sys.executable, lone], capture_output=True, + text=True, input=json.dumps( + {"tool_name": "Bash", "cwd": tmp, + "tool_input": {"command": "ls"}})) + if "could not be loaded" not in r.stdout: + FAILURES.append(f"R4-8 broken install must warn on a git command: " + f"{r.stdout!r} {r.stderr!r}") + if quiet.stdout.strip(): + FAILURES.append(f"R4-9 broken install must stay quiet on `ls`: " + f"{quiet.stdout!r}") + print(f"{COUNT[0]} cases, {len(FAILURES)} failures") for f in FAILURES: print("FAIL", f) diff --git a/shared/workflow/claim-pr.md b/shared/workflow/claim-pr.md index 51cb7e8b6..750d0e82e 100644 --- a/shared/workflow/claim-pr.md +++ b/shared/workflow/claim-pr.md @@ -124,7 +124,7 @@ Every session that comments under one account shares one forge login, so a claim Add a `Session worktree:` line (the checkout path) or a `Session id:` line to the claim body. [`hooks/no-pr-work-without-claim.py`](../../hooks/no-pr-work-without-claim.py) denies a `git commit` or `git push` on a branch with an open Morrison-Lab PR unless one claim comment carries the agent marker and names this session. It also denies when a claim naming a different session is newer than yours, and before a push it warns about pushes since your claim that your local history lacks. -The hook's docstring lists its known limits: claim age and release comments are not evaluated, a PR from a fork is not seen, and two sessions sharing one checkout cannot be told apart. +The hook's docstring lists its known limits: an own claim's age and most release wordings are not evaluated (a peer claim on a PR idle over 2 hours only warns), a PR from a fork is not seen, and two sessions sharing one checkout cannot be told apart. A claim that names no session only warns, until every claim emitter carries the session line ([#4160](https://github.com/Morrison-Lab/ai-config/issues/4160)). The check reads `repos///activity?ref=refs/heads/`. Hooks are inert in remote and web sessions, so a cloud session relies on its own instructions to run the same two reads. diff --git a/skills/claim-pr/SKILL.md b/skills/claim-pr/SKILL.md index 7e7860787..977d6d17d 100644 --- a/skills/claim-pr/SKILL.md +++ b/skills/claim-pr/SKILL.md @@ -61,7 +61,7 @@ _Posted by Claude Code (AI agent) --- not written by a human._" # COMMENT_ISSU ``` The `Session worktree:` line (or a `Session id:` line) is what lets `hooks/no-pr-work-without-claim.py` tell this session's claim from another session's under the same login; -without it the hook denies the first `git commit` on the PR branch. +without it the hook can only warn that the claim names no session, and it denies a commit when the PR has no claim or only another session's. A review-only session uses the same `hold off` invariant so existing detectors still match, and names the review so authors know when they From d19c45f716257fdc57f6a5fdbe5b942e915c204f Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Wed, 30 Sep 2026 20:48:53 -0700 Subject: [PATCH 7/9] fix: fifth review round on the claim-required hook (#4155) Use the canonical release terms (unclaim, released, PR is free, now mergeable), accept a worktree path with a trailing slash, state the subagent-worktree and busy-branch limits accurately, and reword the prose that said the hook denies a session-less claim. 94 cases; four new mutants killed. Co-Authored-By: Claude Sonnet 5.5 --- README.md | 2 +- hooks/hooks.json | 2 +- hooks/no-pr-work-without-claim.py | 28 +++++++++++++++++------- hooks/test-no-pr-work-without-claim.py | 22 +++++++++++++++++++ plugins/ai-config-hooks/hooks/hooks.json | 2 +- shared/workflow/claim-pr.md | 5 +++-- 6 files changed, 48 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 47c8fc547..c8350fcbd 100644 --- a/README.md +++ b/README.md @@ -502,7 +502,7 @@ The payload gaps that remain and the per-guard status are in | `flag-stale-adjacent-comment.py` | `PreToolUse` (Bash) | warns, never blocks, when a `git commit` changes a literal value while an unchanged comment within ten lines still asserts the old one | | `warn-unparseable-staged-config.py` | `PreToolUse` (Bash) | warns when `git commit` would commit staged `.toml`, `.json`, or `.yaml`/`.yml` config files that fail parser validation (`tomllib`, `json`, `yaml.safe_load`) | | `no-delete-branch-under-stacked-pr.py` | `PreToolUse` (Bash) | warns when `gh pr merge --delete-branch` or `gh pr close --delete-branch` would delete a branch that is an open PR's base. GitHub's documented behaviour is to retarget such a PR, but a measured case closed it instead, and a closed PR can be neither retargeted nor reopened while its base is gone. Silent when nothing is stacked, when the query fails or returns an unexpected shape, when `gh` is absent, when the command carries no `-R` or PR target, and on `--delete-branch=false` | -| `no-pr-work-without-claim.py` | `PreToolUse` (Bash) | denies a `git commit` or `git push` on a branch whose open Morrison-Lab PR has no claim comment (agent marker plus hold-off wording) naming this session by session id or worktree path (a claim naming no session only warns, so existing claim flows keep working until #4160), and denies when another session's claim is newer than yours. Before a push it warns, never denies, about pushes since your claim that local history lacks (including any this clone has not fetched), read from the forge activity endpoint. Fails open and says so on any `gh` or network failure. An own claim's age and most release wordings are not evaluated (a peer claim on a PR idle over 2 hours only warns), and a fork PR is not seen. Local half of ai-config#4155: hooks are inert in remote and web sessions. | +| `no-pr-work-without-claim.py` | `PreToolUse` (Bash) | denies a `git commit` or `git push` on a branch whose open Morrison-Lab PR has no claim comment (agent marker plus hold-off wording) naming this session by session id or worktree path (a claim naming no session only warns, so existing claim flows keep working until #4160), and denies when a claim naming another session is newer than yours. Before a push it warns, never denies, about pushes since your claim that local history lacks (including any this clone has not fetched), read from the forge activity endpoint. Fails open and says so on any `gh` or network failure. An own claim's age and most release wordings are not evaluated (a peer claim on a PR idle over 2 hours only warns), and a fork PR is not seen. Local half of ai-config#4155: hooks are inert in remote and web sessions. | | `no-clobbering-push.py` | `PreToolUse` (Bash) | refuses a bare `git push --force`/`-f`, whose remedy (`--force-with-lease --force-if-includes`) costs one word. Warns on every other push whose remote tip a live, read-only `git ls-remote` shows is not an ancestor of the ref being pushed (which is `HEAD` only when the refspec says so, resolved in the directory the push runs in rather than the session's -- a `cd`, scoped to its subshell but not to a brace group, and declined where a compound statement's body, a short-circuited alternative, or a fork into a background job or a pipeline means the pushing shell never takes its effect, then the push's own `-C`, declined in turn when the shell would have had to expand it), names that directory and qualifies its remediation commands with `git -C` when it is not the call's own, declines the reading when the directory is indeterminate or `--git-dir`/`--work-tree`/`GIT_DIR=` redirected the repository, and stays silent on a fast-forward | | `no-commit-chained-to-push.py` | `PreToolUse` (Bash) | denies a Bash call that chains a `git commit` into a later `git push`. A PreToolUse deny rejects the whole invocation, so a guard refusing the push discards the commit too while its message speaks only about the push (ai-config#2992). Denies rather than warns because the refusal stops the chain reaching the sibling guards at all, and its remedy -- two Bash calls -- is always available. Clearable with `ALLOW_COMMIT_AND_PUSH=1`, either prefixing the commit or push or as the call's own leading assignment (a subshell or short-circuited one sets nothing and does not count). Matches over an argv split (`scripts/lib/shellcmd.py`), so a quoted commit message, a heredoc body and `git commit-tree` cannot trip it, while `timeout 60 git push`, `/usr/bin/git push` and `{ git commit; } && git push` all resolve -- the guard has to fire wherever its siblings would. There is no exemption for a `--dry-run` or `--delete` command: one was written and removed after a review measured `git commit ... && git push --force --delete` and `... --dry-run --no-dry-run --force` both going silent while `no-clobbering-push.py` denied them | | `flag-chained-push.py` | `PreToolUse` (Bash) | warns, never blocks, when a `git push` is chained after another command with `&&`, `;`, or `\|\|`, piped onward with `\|`, or suffixed by a redirection (`>`, `>>`, or an fd form like `2>&1`). `no-clobbering-push.py` and the plugin's own `no-push-without-self-review.py` push policy both parse the WHOLE command text for a push rather than the isolated segment, so a trailing `2>&1` hands either parser a bare `2` sitting where a commit-ish token would sit in other shapes, and a chained prefix reads as part of the same invocation; a refused chain runs NOTHING, which the refusal naming only the push invites the author to misread as the prefix having succeeded. Measured on Lacaedemon/sparta, 2026-09-05: three refusals in one session | diff --git a/hooks/hooks.json b/hooks/hooks.json index 5fd84a08b..7a3e6fae7 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -456,7 +456,7 @@ "command": "python3 \"${CLAUDE_PLUGIN_ROOT}/hooks/no-pr-work-without-claim.py\"", "timeout": 30, "script": "no-pr-work-without-claim.py", - "why": "ai-config#4155, measured 2026-09-30: two agent sessions worked the same three PR branches at once and neither posted a claim, so a cloud session's restore commit on PR #4134 left memories/preferences.md at 706 lines instead of 1177. shared/workflow/claim-pr.md says to claim first and nothing enforced it; ListAgents cannot see a cloud session. DENIES a `git commit` or `git push` on a branch with an open Morrison-Lab PR unless a claim comment (agent marker plus hold-off wording) names THIS session by session id or worktree path, and denies when another session's claim is newer than yours. Before a push it WARNS (never denies) about pushes since your claim that are not in local history, read from the forge activity endpoint. Fails OPEN and says so on any network or gh failure. Local half only: hooks are inert in remote/web sessions (#2004). Clear with ALLOW_UNCLAIMED_PR_WORK=1. Every subprocess shares one 20s budget (PR_CLAIM_TOTAL_BUDGET), under this 30s timeout, so a slow forge is a visible fail-open rather than a harness kill. The skills/claim-pr template now carries the Session worktree line the hook matches on; the other claim emitters (ardi, handoff, gi, st, gip, pr-on-claim, tracked in #4160) do not yet, so a claim that names no session only WARNS (it may be the session's own) instead of denying. A commit nested in `bash -c` is a visible fail-open, and two sessions sharing one checkout cannot be told apart (known limits in the hook docstring)." + "why": "ai-config#4155, measured 2026-09-30: two agent sessions worked the same three PR branches at once and neither posted a claim, so a cloud session's restore commit on PR #4134 left memories/preferences.md at 706 lines instead of 1177. shared/workflow/claim-pr.md says to claim first and nothing enforced it; ListAgents cannot see a cloud session. DENIES a `git commit` or `git push` on a branch with an open Morrison-Lab PR unless a claim comment (agent marker plus hold-off wording) names THIS session by session id or worktree path, and denies when a claim naming another session is newer than yours. Before a push it WARNS (never denies) about pushes since your claim that are not in local history, read from the forge activity endpoint. Fails OPEN and says so on any network or gh failure. Local half only: hooks are inert in remote/web sessions (#2004). Clear with ALLOW_UNCLAIMED_PR_WORK=1. Every subprocess shares one 20s budget (PR_CLAIM_TOTAL_BUDGET), under this 30s timeout, so a slow forge is a visible fail-open rather than a harness kill. The skills/claim-pr template now carries the Session worktree line the hook matches on; the other claim emitters (ardi, handoff, gi, st, gip, pr-on-claim, tracked in #4160) do not yet, so a claim that names no session only WARNS (it may be the session's own) instead of denying. A commit nested in `bash -c` is a visible fail-open, and two sessions sharing one checkout cannot be told apart (known limits in the hook docstring)." }, { "type": "command", diff --git a/hooks/no-pr-work-without-claim.py b/hooks/no-pr-work-without-claim.py index 22a3eb512..8b96a92a4 100644 --- a/hooks/no-pr-work-without-claim.py +++ b/hooks/no-pr-work-without-claim.py @@ -64,9 +64,13 @@ other alias is not matched and the hook stays silent. Session identity is the worktree path (the model rarely knows its own -`session_id`), so two sessions sharing ONE checkout cannot be told apart, and -neither can an isolated subagent worktree from its parent. Known limit; it is -the same-checkout shape of the 2026-09-30 incident that this hook cannot see. +`session_id`), so two sessions sharing ONE checkout cannot be told apart. +Known limit; it is the same-checkout shape of the 2026-09-30 incident that this +hook cannot see. The converse also holds: an isolated subagent worktree has a +different path from its parent, so a PR branch the parent claimed reads as a +PEER's claim to the subagent, which is denied until it posts a claim of its +own (or sets the override). That is the intended reading for a second writer +on a claimed branch, and subagents normally work branches of their own. A commit or push nested in a shell's `-c` starts in a directory this scan cannot know, so it is a visible fail-open (not evaluated) unless the piece @@ -84,14 +88,17 @@ A push is judged by its first refspec's destination branch (`git push origin HEAD:foo` checks `foo`; a tag push is skipped); with no refspec it is the checkout's current branch. A push of several branches is judged by the first. +An argument-less `git push` goes to the branch's upstream remote, which is +assumed to be `origin`. The activity warning compares each push's `after` SHA to local `HEAD`, so a session that amended, rebased or force-pushed its own earlier pushes sees them as foreign, and so does a main-sync merge pushed by the @claude bot, and so does any push whose commit this clone has not fetched (a stale or shallow clone). It is a -warning, never a deny, for that reason. At most MAX_ACTIVITY_CHECKS pushes are -examined. +warning, never a deny, for that reason. Only the newest page (50 entries) of +the activity feed is read, and at most MAX_ACTIVITY_CHECKS pushes of it are +examined, so on a busy branch older foreign pushes are not seen. Every subprocess shares one TOTAL_BUDGET-second deadline (under the hooks.json timeout); running out is a visible fail-open. @@ -139,6 +146,12 @@ TOTAL_BUDGET = float(os.environ.get("PR_CLAIM_TOTAL_BUDGET", "20")) _DEADLINE = [None] MAX_ACTIVITY_CHECKS = 10 +# The release terms claim-pr.md already tells every claim detector to check +# for ("unclaiming", the retired "paws off released", the "PR is free" and +# "now mergeable" forms), plus "releasing my claim". A bare "will release the +# claim when done" is a claim, not a release. +RELEASE_TERMS = (r"unclaim|released|pr is free|now mergeable" + r"|releasing (?:my |the |this )?claim") # claim-pr.md: a claim is live for 2 hours from the PR's last push or comment. STALE_HOURS = 2 SKIP_BRANCHES = {"HEAD", "main", "master"} @@ -310,8 +323,7 @@ def is_claim(body): return (AGENT_MARKER in low and re.search(r"(?//activity?ref=refs/heads/`. From a447b3bc441f702400370a877e639f02bddb5ca2 Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Wed, 30 Sep 2026 21:08:18 -0700 Subject: [PATCH 8/9] fix: sixth review round on the claim-required hook (#4155) Strip the Session worktree / Session id lines before reading a claim for release terms (a worktree named after an issue slug like fix-unclaim-wording made a correct claim fail), accept the back-off wording claim-pr.md lists, and treat a missing cwd as a quiet pass. 102 cases; wrapped git invocations (/usr/bin/git, timeout, command) are now pinned by tests. Co-Authored-By: Claude Sonnet 5.5 --- hooks/no-pr-work-without-claim.py | 26 ++++++++++++++++-------- hooks/test-no-pr-work-without-claim.py | 28 ++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/hooks/no-pr-work-without-claim.py b/hooks/no-pr-work-without-claim.py index 8b96a92a4..f8ce3ee2c 100644 --- a/hooks/no-pr-work-without-claim.py +++ b/hooks/no-pr-work-without-claim.py @@ -21,7 +21,7 @@ PR's issue comments and DENIES unless one is a claim from THIS session: * a claim is a comment carrying the agent marker (`Posted by Claude Code (AI - agent)`) and the `hold off` (or legacy `paws off`) wording, per + agent)`) and the `hold off` (or legacy `paws off` / `back off`) wording, per `skills/claim-pr/SKILL.md`; * it is THIS session's when its body contains the payload's `session_id` or this worktree's path (`/c/x` and `C:/x` spellings are equal). The forge @@ -270,7 +270,13 @@ def run(argv, cwd=None): def git(cwd, *args): - r = run(["git", *args], cwd=cwd) + """stdout of a git call in `cwd`, or None (also when `cwd` does not exist: + git could not run the user's command there either, so there is nothing to + guard and no reason to warn).""" + try: + r = run(["git", *args], cwd=cwd) + except OSError: + return None return r.stdout.strip() if r.returncode == 0 else None @@ -313,16 +319,20 @@ def norm(text): def is_claim(body): """A claim comment: the agent marker plus hold-off wording. - Every emitter in skills/ carries one of the two wordings (claim-pr, ardi, - handoff, and the review-only form all say `hold off`; the older emitters - say `paws off`), so that is the invariant. A release comment ("unclaiming", + Every emitter in skills/ carries one of the three wordings claim-pr.md + tells claim readers to match (claim-pr, ardi, handoff and the review-only + form say `hold off`; the older emitters say `paws off` or `back off`), so + that is the invariant. A release comment ("unclaiming", "releasing my claim") and a negated mention ("no need to hold off") are not claims. """ - low = (body or "").lower() + # The session lines are data, not prose: a worktree named after an issue + # slug ("fix-unclaim-wording") must not read as a release term. + low = re.sub(r"(?m)^[ \t]*session (?:worktree|id):.*$", "", + (body or "").lower()) return (AGENT_MARKER in low - and re.search(r"(? str. `other` is a @@ -89,7 +89,8 @@ def run(name, command, expect, *, comments=None, prs="open", repo_kwargs=None, """ COUNT[0] += 1 with tempfile.TemporaryDirectory() as tmp: - repo, top = make_repo(tmp, branch=branch, **(repo_kwargs or {})) + repo, top = make_repo(tmp, branch=branch, name=repo_name, + **(repo_kwargs or {})) other, other_top = make_repo(tmp, branch="main", name="wt-other") if callable(command): command = command(top, other_top) @@ -467,6 +468,29 @@ def side_branch_sha(repo): comments=lambda top: [claim("Session worktree: `/somewhere/else/entirely`")], check_in_ctx="working this branch now") +# --- review round 6 ------------------------------------------------------- +# Worktrees are named after issue slugs, and the repo's vocabulary includes the +# release terms: a path or id carrying one must not make a good claim fail. +run("R6-1 worktree named after a release term still matches", COMMIT, None, + repo_name="fix-unclaim-wording", + comments=lambda top: [claim(f"Session worktree: `{top}`")]) +run("R6-2 worktree named 'pr-released' still matches", COMMIT, None, + repo_name="pr-released", + comments=lambda top: [claim(f"Session worktree: `{top}`")]) +run("R6-3 session id containing a release term still matches", COMMIT, None, + comments=[claim("Session id: `session_now-mergeable_1`")], + extra_payload={"session_id": "session_now-mergeable_1"}) +run("R6-4 'back off' wording is a claim", COMMIT, None, + comments=lambda top: [claim(f"Session worktree: `{top}`", + phrase="back off")]) +# Wrapped invocations resolve through strip_env (checked here, not assumed). +run("R6-5 /usr/bin/git commit", "/usr/bin/git commit -m x", "deny") +run("R6-6 timeout 60 git push", "timeout 60 git push origin feat/x", "deny") +run("R6-7 command git commit", "command git commit -m x", "deny") +# A directory that does not exist is a quiet pass, not a warning. +run("R6-8 cd into a missing directory", "cd /no/such/dir/at/all && " + COMMIT, + None) + # A broken install (no scripts/lib beside the hook) must be visible, not inert. COUNT[0] += 1 with tempfile.TemporaryDirectory() as tmp: From 82ee03a05c4a3da3c04bc6d66734ab224b010b8d Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Wed, 30 Sep 2026 21:26:00 -0700 Subject: [PATCH 9/9] fix: seventh review round on the claim-required hook (#4155) A bare 'released' is no longer a release term (a claim saying a fix 'can be released' was dropped and its session denied); only 'paws off released', 'claim released', 'released my claim', 'PR is free', 'now mergeable', 'unclaim' and 'releasing my claim' are. Name the reads a cloud session should run in claim-pr.md. 104 cases; three new mutants killed. Co-Authored-By: Claude Sonnet 5.5 --- hooks/no-pr-work-without-claim.py | 9 +++++++-- hooks/test-no-pr-work-without-claim.py | 10 ++++++++++ shared/workflow/claim-pr.md | 2 +- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/hooks/no-pr-work-without-claim.py b/hooks/no-pr-work-without-claim.py index f8ce3ee2c..719639729 100644 --- a/hooks/no-pr-work-without-claim.py +++ b/hooks/no-pr-work-without-claim.py @@ -150,8 +150,13 @@ # for ("unclaiming", the retired "paws off released", the "PR is free" and # "now mergeable" forms), plus "releasing my claim". A bare "will release the # claim when done" is a claim, not a release. -RELEASE_TERMS = (r"unclaim|released|pr is free|now mergeable" - r"|releasing (?:my |the |this )?claim") +# +# "released" alone is NOT a term: a claim can say "so it can be released" in its +# own prose, and treating that as a release denies a session that claimed +# correctly. The retired form is "paws off released". +RELEASE_TERMS = (r"unclaim|paws off released|claims? released" + r"|released (?:my |the |this )?claim|pr is free" + r"|now mergeable|releasing (?:my |the |this )?claim") # claim-pr.md: a claim is live for 2 hours from the PR's last push or comment. STALE_HOURS = 2 SKIP_BRANCHES = {"HEAD", "main", "master"} diff --git a/hooks/test-no-pr-work-without-claim.py b/hooks/test-no-pr-work-without-claim.py index 903c0f4a0..16e9165b2 100644 --- a/hooks/test-no-pr-work-without-claim.py +++ b/hooks/test-no-pr-work-without-claim.py @@ -487,6 +487,16 @@ def side_branch_sha(repo): run("R6-5 /usr/bin/git commit", "/usr/bin/git commit -m x", "deny") run("R6-6 timeout 60 git push", "timeout 60 git push origin feat/x", "deny") run("R6-7 command git commit", "command git commit -m x", "deny") +# --- review round 7 ------------------------------------------------------- +run("R7-1 a claim whose own prose says 'can be released' is still a claim", + COMMIT, None, + comments=lambda top: [claim(f"Session worktree: `{top}`\n\nDriving this " + f"PR so the fix can be released.")]) +run("R7-2 'claim released' is a release", COMMIT, "deny", + comments=[{"body": f"Claim released; hold off no more. Session id: " + f"`{SID}`\n\n{MARKER}", + "created_at": "2026-09-30T20:00:00Z", "html_url": "u"}]) + # A directory that does not exist is a quiet pass, not a warning. run("R6-8 cd into a missing directory", "cd /no/such/dir/at/all && " + COMMIT, None) diff --git a/shared/workflow/claim-pr.md b/shared/workflow/claim-pr.md index c8be6088d..7bc9a0d7a 100644 --- a/shared/workflow/claim-pr.md +++ b/shared/workflow/claim-pr.md @@ -128,7 +128,7 @@ It also denies when a claim that names a different session is newer than yours, The hook's docstring lists its known limits: an own claim's age and most release wordings are not evaluated (a peer claim on a PR idle over 2 hours only warns), a PR from a fork is not seen, and two sessions sharing one checkout cannot be told apart. A claim that names no session only warns, until every claim emitter carries the session line ([#4160](https://github.com/Morrison-Lab/ai-config/issues/4160)). The check reads `repos///activity?ref=refs/heads/`. -Hooks are inert in remote and web sessions, so a cloud session relies on its own instructions to run the same two reads. +Hooks are inert in remote and web sessions, so a cloud session relies on its own instructions to read the PR's claim comments and that activity endpoint before it commits or pushes. - **Do:** post the claim, with a `Session worktree:` or `Session id:` line, before the first commit on a PR branch you did not just create. - **Do:** read the activity endpoint (or `git ls-remote` plus `git log`) for pushes you did not make before every push, and treat one as a peer.