From b8dab99d56479a1f352d0af598f66bfad4083df9 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 08:26:53 +0000 Subject: [PATCH 1/6] Close self-approval, worktree reclaim, and scope enforcement gaps - Approvals: the agent that consumes an approval (pusher or takeover claimant) never counts toward its threshold, and a takeover's beneficiary cannot vote yes. Approved-but-unused instances still accept votes, so an independent approver can replace a self-vote and a deny revokes it before use. - Claims: a reclaim of an expired lease or an approved takeover on the incumbent's branch inherits its managed worktree instead of failing because Git will not check the branch out twice. - Scopes: globs are normalized (./, //, leading /, backslashes, ..) and overlap is compared case-insensitively; scopes escaping the repository are refused. - Hook: pushes to a claimed branch are rejected when the new commits touch paths outside the claim's scopes (merge commits count only paths the merge itself changed). - CLI: commit specs resolve to full OIDs (short SHAs, refs, uppercase); duplicate agent/protected-ref registration and malformed operation JSON are clean errors; task and agent creation take the write reservation. - handoff create honors an explicit --last-commit and no longer crashes when the claim's worktree has been removed. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Q8BpCkArA34hB1aSyVcnZp --- README.md | 17 +- src/quorumgit/cli.py | 98 ++++--- src/quorumgit/gate.py | 111 +++++++- src/quorumgit/handoff.py | 9 +- src/quorumgit/registry.py | 5 +- src/quorumgit/trees.py | 44 +++ src/quorumgit/work.py | 157 +++++++++-- tests/test_governance_fixes.py | 478 +++++++++++++++++++++++++++++++++ 8 files changed, 837 insertions(+), 82 deletions(-) create mode 100644 tests/test_governance_fixes.py diff --git a/README.md b/README.md index adfd0cf..fc61479 100644 --- a/README.md +++ b/README.md @@ -58,7 +58,7 @@ Then initialize the store once: ```bash $ quorumgit init store: /home/you/.quorumgit/quorumgit.db -migrations applied: ['001_core.sql'] +migrations applied: ['001_core.sql', '002_approval_identities.sql'] contract: ok ``` @@ -124,7 +124,7 @@ Every claim attempt is classified against all live claims before it is granted, | `CONFLICTING` | branch already claimed by another task | refused | | `BLOCKED` | task already held by an unexpired claim | refused; takeover requires approval | -Scope overlap uses a conservative literal-prefix test (the prefix of one glob up to its first wildcard against the other's) — it errs toward flagging. +Scopes are normalized when declared (`./src/**`, `/src/**`, `src//**` and `src\**` all become `src/**`; scopes that escape the repository root are refused). Scope overlap then uses a conservative literal-prefix test (the prefix of one glob up to its first wildcard against the other's), compared case-insensitively — it errs toward flagging. ## Two deployment models @@ -151,7 +151,9 @@ QUORUMGIT_AGENT=agent-two git push origin feat/x # rejected — branch is cla git push origin feat/x # rejected — unidentified ``` -Checkpoints in the hub model take an explicit `--commit `. Continuation points are **verified, not trusted**: the commit must exist in the registered repository, and when the claimed branch exists, be reachable from it. A typo'd or fabricated OID is rejected. +**Scopes are enforced at push time.** When a branch has a live claim, every commit the push introduces must only touch paths inside that claim's scopes (`**` spans directories, `*` stays within one; a scope without wildcards covers that file or directory). Merge commits count only the paths the merge itself changed, so merging the base branch in is not attributed to the claim holder. In the local model agents commit directly in their worktree, so no hook runs and scopes remain coordination metadata. + +Checkpoints in the hub model take an explicit `--commit ` (any commit spec Git resolves — a full or abbreviated OID or a ref — recorded as the full OID). Continuation points are **verified, not trusted**: the commit must exist in the registered repository, and when the claimed branch exists, be reachable from it. A typo'd or fabricated OID is rejected. ## Protected operations and approvals @@ -177,13 +179,14 @@ quorumgit approve vote 17 --agent operator Rules that hold no matter what: - **An approval never bypasses branch reservations.** A claimed branch still accepts pushes only from its claiming agent; a branch frozen by an open handoff accepts none at all. The hook checks reservations *before* approvals. -- **Denial has precedence and is terminal.** One `--deny` vote denies the approval; later yes votes are refused. +- **No agent authorizes itself.** The agent that uses an approval (the pusher, or the claimant of a takeover) never counts toward its threshold: its own yes vote is ignored at consumption, and a takeover's beneficiary cannot vote yes at all. If a self-vote was the only approval, another registered agent can still add theirs to the approved instance. +- **Denial has precedence and is terminal.** One `--deny` vote denies the approval — including an approved one that has not yet been used; later yes votes are refused. - **Consumption is single-use under concurrency.** Two simultaneous pushes racing for one approval produce exactly one accepted push — the loser is rejected, not silently allowed. - **Votes bind to one approval instance.** A delayed vote for an older denied or consumed instance cannot decide a newer request with the same operation hash. Requesters, voters, consumers, and pushers must name registered agents. - **A consumed approval is spent, not blacklisted.** Consumption moves the approval to a terminal `consumed` state (with `consumed_at`) rather than reusing `denied`, and only one *live* (`pending` or `approved`) approval may exist per operation hash. The same operation can therefore be requested and approved again later as a new approval instance — which matters for takeovers, whose payload is stable and legitimately repeatable. What is never possible is one approval authorizing twice. - The default threshold is 1 (a human operator); the vote schema supports higher thresholds. -Takeovers follow the same pattern: claiming a task someone else holds (`claim --takeover`) prints the takeover operation to approve. Its payload includes the incumbent claim ID, so an unused approval cannot displace a later claim by the same agent. The takeover is atomic — the incumbent is released, the replacement claim created, and the approval consumed in one transaction, or none of it happens. A refused takeover leaves the incumbent untouched and the approval unconsumed. +Takeovers follow the same pattern: claiming a task someone else holds (`claim --takeover`) prints the takeover operation to approve. Its payload includes the incumbent claim ID, so an unused approval cannot displace a later claim by the same agent. The takeover is atomic — the incumbent is released, the replacement claim created, and the approval consumed in one transaction, or none of it happens. A refused takeover leaves the incumbent untouched and the approval unconsumed. When the replacement claim uses the incumbent's branch, it inherits the incumbent's managed worktree (including uncommitted work), because Git will not check one branch out twice; reclaiming an expired lease on the same branch works the same way. ## Command reference @@ -198,9 +201,9 @@ Takeovers follow the same pattern: claiming a task someone else holds (`claim --title [--objective ]` | Create a task | | `quorumgit claim --branch --scope … [--no-worktree] [--takeover] [--override-overlap] [--lease-hours ]` | Claim a task | | `quorumgit renew ` | Extend a live, unexpired lease; expired claims must be acquired again | -| `quorumgit checkpoint [--commit ] [--note ]` | Record verified progress | +| `quorumgit checkpoint [--commit ] [--note ]` | Record verified progress | | `quorumgit release [--remove-worktree] [--reason ]` | Release a claim | -| `quorumgit handoff create --completed --remaining [--to ] [--last-commit ]` | Hand work off | +| `quorumgit handoff create --completed --remaining [--to ] [--last-commit ]` | Hand work off (from `--last-commit` when given, else the worktree's HEAD) | | `quorumgit handoff accept ` | Continue handed-off work (addressee, or anyone if unaddressed) | | `quorumgit handoff decline ` | Decline — addressee only; clean retained worktree is removed | | `quorumgit handoff cancel ` | Cancel — creator only; clean retained worktree is removed | diff --git a/src/quorumgit/cli.py b/src/quorumgit/cli.py index 4749d04..84ddb92 100644 --- a/src/quorumgit/cli.py +++ b/src/quorumgit/cli.py @@ -158,34 +158,35 @@ def cmd_claim(args, cfg) -> int: with store.connect(cfg) as conn: takeover_operation = None takeover_approval = None - if args.takeover: - # Bind the approval to a stable incumbent. claim_task() and every - # release take this same task lock, closing the stale-holder window. - work.lock_task(conn, args.task_id) - holder = work.active_claim_for_task(conn, args.task_id) - if holder and not holder["expired"]: - task = work.get_task(conn, args.task_id) - takeover_operation = { - "type": "lease_takeover", - "repository": task["repository"], - "task_id": args.task_id, - "from_claim_id": holder["id"], - "from_agent": holder["agent"], - "to_agent": agent, - } - takeover_approval = gate.approved_instance(conn, takeover_operation) - if takeover_approval is None: - print( - "[quorumgit] REFUSED: lease takeover requires approval.\n" - f"operation hash: " - f"{gate.operation_hash(takeover_operation)}\n" - "Request/approve it via `quorumgit approve request/vote` " - "with operation:\n" - f"{json.dumps(takeover_operation, sort_keys=True)}", - file=sys.stderr, - ) - conn.commit() # keep the conflict/audit trail - return 1 + # Pin the incumbent (live or expired) under the task lock: a takeover + # approval binds to it, and a successor on the same branch inherits + # its worktree. claim_task() and every release take this same lock. + work.lock_task(conn, args.task_id) + holder = work.active_claim_for_task(conn, args.task_id) + if args.takeover and holder and not holder["expired"]: + task = work.get_task(conn, args.task_id) + takeover_operation = { + "type": "lease_takeover", + "repository": task["repository"], + "task_id": args.task_id, + "from_claim_id": holder["id"], + "from_agent": holder["agent"], + "to_agent": agent, + } + takeover_approval = gate.approved_instance(conn, takeover_operation) + if takeover_approval is None: + print( + "[quorumgit] REFUSED: lease takeover requires approval.\n" + f"operation hash: " + f"{gate.operation_hash(takeover_operation)}\n" + "Request it via `quorumgit approve request`; another " + "registered agent must approve it with `quorumgit approve " + "vote`. Operation:\n" + f"{json.dumps(takeover_operation, sort_keys=True)}", + file=sys.stderr, + ) + conn.commit() # keep the conflict/audit trail + return 1 try: claim_id, classification, _ = work.claim_task( conn, @@ -218,12 +219,18 @@ def cmd_claim(args, cfg) -> int: print(f"[quorumgit] REFUSED: {exc}", file=sys.stderr) return 1 wt = None + inherited = False if not args.no_worktree: - wt = trees.create_worktree(conn, claim_id, cfg.worktrees_dir) + if holder is not None: + wt = trees.inherit_worktree(conn, holder["id"], claim_id, agent) + inherited = wt is not None + if wt is None: + wt = trees.create_worktree(conn, claim_id, cfg.worktrees_dir) conn.commit() print(f"claim {claim_id} acquired ({classification}).") if wt: - print(f"worktree: {wt['path']}") + suffix = f" (inherited from claim {holder['id']})" if inherited and holder else "" + print(f"worktree: {wt['path']}{suffix}") print(f"branch: {wt['branch']}") else: print(f"branch: {args.branch} (no worktree — work from your own clone " @@ -252,19 +259,25 @@ def cmd_release(args, cfg) -> int: return 0 +def _active_worktree(conn, claim_id: int) -> dict | None: + wt = trees.worktree_for_claim(conn, claim_id) + return wt if wt and wt["removed_at"] is None else None + + def cmd_checkpoint(args, cfg) -> int: agent = _agent(args, cfg) with store.connect(cfg) as conn: commit = args.commit if not commit: - wt = trees.worktree_for_claim(conn, args.claim_id) + wt = _active_worktree(conn, args.claim_id) if wt is None: - print("No worktree for this claim; pass --commit .", + print("No active worktree for this claim; pass --commit .", file=sys.stderr) return 1 commit = trees.head_commit(wt["path"]) cp_id = work.add_checkpoint(conn, args.claim_id, agent, commit, note=args.note) + commit = work.checkpoint_commit(conn, cp_id) conn.commit() print(f"checkpoint {cp_id} at {commit}.") return 0 @@ -276,11 +289,16 @@ def cmd_checkpoint(args, cfg) -> int: def cmd_handoff_create(args, cfg) -> int: agent = _agent(args, cfg) with store.connect(cfg) as conn: - wt = trees.worktree_for_claim(conn, args.claim_id) - last_commit = trees.head_commit(wt["path"]) if wt else args.last_commit + # An explicit --last-commit always wins; otherwise continue from the + # managed worktree's HEAD. + last_commit = args.last_commit if not last_commit: - print("Provide --last-commit (no worktree found).", file=sys.stderr) - return 1 + wt = _active_worktree(conn, args.claim_id) + if wt is None: + print("Provide --last-commit (no active worktree for this claim).", + file=sys.stderr) + return 1 + last_commit = trees.head_commit(wt["path"]) record = { "completed": args.completed, "remaining": args.remaining, @@ -295,6 +313,7 @@ def cmd_handoff_create(args, cfg) -> int: handoff_id = handoff.create_handoff( conn, args.claim_id, agent, record, to_agent=args.to ) + last_commit = handoff.get_handoff(conn, handoff_id)["record"]["last_commit"] conn.commit() print(f"handoff {handoff_id} created (last commit {last_commit}).") return 0 @@ -349,9 +368,12 @@ def cmd_handoff_cancel(args, cfg) -> int: def _operation_from_args(args) -> dict: - operation = json.loads(args.operation) + try: + operation = json.loads(args.operation) + except json.JSONDecodeError as exc: + raise gate.GateError(f"Operation is not valid JSON: {exc}") from exc if not isinstance(operation, dict): - raise SystemExit("Operation must be a JSON object.") + raise gate.GateError("Operation must be a JSON object.") return operation diff --git a/src/quorumgit/gate.py b/src/quorumgit/gate.py index a6a1d7b..d968757 100644 --- a/src/quorumgit/gate.py +++ b/src/quorumgit/gate.py @@ -23,7 +23,12 @@ get_repository, ) from .store import Connection, begin_immediate, json_dumps, json_loads -from .work import live_claim_for_branch, open_handoff_for_branch +from .work import ( + claim_scopes, + live_claim_for_branch, + open_handoff_for_branch, + path_in_scopes, +) DEFAULT_THRESHOLD = 1 HOOK_MARKER = "# quorumgit-managed-pre-receive v1" @@ -169,17 +174,45 @@ def get_approval_by_id(conn: Connection, approval_id: int) -> dict: return _approval_dict(row) +def _beneficiary(operation: dict[str, Any]) -> str | None: + """The agent an operation authorizes, when it is known before consumption.""" + if operation.get("type") == "lease_takeover": + return str(operation.get("to_agent") or "") or None + return None + + +def _independent_yes_votes( + conn: Connection, approval_id: int, excluding_agent_id: int +) -> int: + row = conn.execute( + "SELECT count(*) FROM votes " + "WHERE approval_id = ? AND vote = 1 AND voter_agent_id <> ?", + (approval_id, excluding_agent_id), + ).fetchone() + assert row is not None + return row[0] + + def vote(conn: Connection, approval_id: int, voter: str, approve: bool) -> dict: """Record a vote against one explicit approval instance atomically. BEGIN IMMEDIATE serializes competing voters before either reads the current - approval state. Denial has precedence and terminal states remain final. + approval state. Denial has precedence and denied/consumed are terminal. + An approved instance still accepts votes until it is consumed: further yes + votes let independent approvers replace a self-vote that consumption will + not count, and a no vote revokes it before use. An agent may never approve + an operation whose known beneficiary is itself. """ begin_immediate(conn) voter_row = get_agent(conn, voter) approval = get_approval_by_id(conn, approval_id) - if approval["status"] != "pending": + if approval["status"] not in ("pending", "approved"): raise GateError(f"Approval {approval_id} is already {approval['status']}.") + if approve and _beneficiary(approval["operation"]) == voter: + raise GateError( + f"Agent {voter} cannot approve operation {approval_id} that " + "authorizes itself; another registered agent must approve it." + ) threshold = approval["threshold"] conn.execute( """ @@ -214,11 +247,11 @@ def vote(conn: Connection, approval_id: int, voter: str, approve: bool) -> dict: new_status = "approved" else: new_status = "pending" - if new_status != "pending": + if new_status != approval["status"]: conn.execute( "UPDATE approvals SET status = ?, decided_at = unixepoch() " - "WHERE id = ? AND status = 'pending'", - (new_status, approval["id"]), + "WHERE id = ? AND status = ?", + (new_status, approval["id"], approval["status"]), ) audit.record( conn, @@ -266,6 +299,13 @@ def consume_approval( f"Approval {op_hash} is not consumable (status " f"{approval['status']}); it may already be used." ) + independent = _independent_yes_votes(conn, approval["id"], consumer["id"]) + if independent < approval["threshold"]: + raise GateError( + f"Approval {approval_id} needs {approval['threshold']} approving " + f"vote(s) from agents other than {agent}, who is using it; it has " + f"{independent}. An agent cannot authorize its own operation." + ) cur = conn.execute( "UPDATE approvals SET status = 'consumed', consumed_at = unixepoch(), " "consumed_by_agent_id = ? " @@ -315,6 +355,63 @@ def _is_fast_forward(git_dir: str, oldrev: str, newrev: str) -> bool: return result.returncode == 0 +MAX_REPORTED_PATHS = 10 + + +def _pushed_paths(git_dir: str, newrev: str) -> list[str]: + """Paths changed by the commits this push introduces to the repository. + + `newrev --not --all` is exactly the set of new commits: in pre-receive no + ref points at them yet. `--cc` lists only the paths a merge commit itself + authored (differing from every parent), so merging the base branch in does + not attribute the base's files to the pusher. Renames count as both paths. + """ + result = subprocess.run( + [ + "git", + "--git-dir", + git_dir, + "log", + "--format=", + "--name-only", + "--no-renames", + "--cc", + "-z", + newrev, + "--not", + "--all", + ], + capture_output=True, + ) + if result.returncode != 0: + raise PushRejected("Unable to determine the paths changed by this push.") + paths = ( + entry.lstrip("\n") + for entry in result.stdout.decode("utf-8", "surrogateescape").split("\0") + ) + return list(dict.fromkeys(path for path in paths if path)) + + +def enforce_claim_scopes( + conn: Connection, git_dir: str, claim: dict, branch: str, newrev: str +) -> None: + """Reject new commits that touch paths outside the claim's declared scopes.""" + scopes = claim_scopes(conn, claim["id"]) + outside = [ + path for path in _pushed_paths(git_dir, newrev) + if not path_in_scopes(path, scopes) + ] + if outside: + shown = ", ".join(outside[:MAX_REPORTED_PATHS]) + more = len(outside) - MAX_REPORTED_PATHS + if more > 0: + shown += f", and {more} more" + raise PushRejected( + f"Push to {branch!r} changes paths outside claim {claim['id']}'s " + f"scopes {scopes}: {shown}. Claim a scope that covers them first." + ) + + def _invoking_git_common_dir(git_dir: str) -> Path: result = subprocess.run( ["git", "--git-dir", git_dir, "rev-parse", "--git-common-dir"], @@ -377,6 +474,8 @@ def check_ref_update( f"(claim {claim['id']}); pusher is " f"{pusher or 'unidentified — set QUORUMGIT_AGENT'}." ) + if claim and not _is_zero(newrev): + enforce_claim_scopes(conn, git_dir, claim, branch, newrev) protected = refname in repo["protected_refs"] deletion = _is_zero(newrev) diff --git a/src/quorumgit/handoff.py b/src/quorumgit/handoff.py index 4d76f46..dba0125 100644 --- a/src/quorumgit/handoff.py +++ b/src/quorumgit/handoff.py @@ -37,9 +37,12 @@ def create_handoff( if not record.get(field): raise HandoffError(f"Handoff record requires {field!r}.") task = work.get_task(conn, claim["task_id"]) - work.verify_commit( - task["repository_path"], record["last_commit"], branch=claim["branch"] - ) + record = { + **record, + "last_commit": work.verify_commit( + task["repository_path"], record["last_commit"], branch=claim["branch"] + ), + } to_agent_id = get_agent(conn, to_agent)["id"] if to_agent else None row = conn.execute( diff --git a/src/quorumgit/registry.py b/src/quorumgit/registry.py index ff40e63..ba97bbd 100644 --- a/src/quorumgit/registry.py +++ b/src/quorumgit/registry.py @@ -131,7 +131,7 @@ def add_repository( ).fetchone() assert row is not None repo_id = row[0] - for refname in protected_refs or []: + for refname in dict.fromkeys(protected_refs or []): conn.execute( "INSERT INTO protected_refs (repository_id, refname) VALUES (?, ?)", (repo_id, refname), @@ -181,6 +181,9 @@ def list_repositories(conn: Connection) -> list[dict]: def add_agent(conn: Connection, name: str) -> int: + begin_immediate(conn) + if conn.execute("SELECT 1 FROM agents WHERE name = ?", (name,)).fetchone(): + raise RegistryError(f"Agent is already registered: {name}") row = conn.execute( "INSERT INTO agents (name) VALUES (?) RETURNING id", (name,) ).fetchone() diff --git a/src/quorumgit/trees.py b/src/quorumgit/trees.py index 904830b..5e5c008 100644 --- a/src/quorumgit/trees.py +++ b/src/quorumgit/trees.py @@ -129,6 +129,50 @@ def transfer_worktree(conn: Connection, worktree_id: int, new_claim_id: int) -> ) +def inherit_worktree( + conn: Connection, from_claim_id: int, to_claim_id: int, agent: str +) -> dict | None: + """Move a superseded claim's managed worktree to its successor claim. + + An expired or taken-over claim's checkout still has its branch checked + out, so Git refuses a second worktree for that branch. When the successor + claims the same branch, it inherits the checkout (and any uncommitted + work in it), exactly as a handoff does. Returns None when there is nothing + safe to inherit, so the caller creates a fresh worktree instead. + """ + wt = worktree_for_claim(conn, from_claim_id) + if wt is None or wt["removed_at"] is not None: + return None + previous = get_claim(conn, from_claim_id) + successor = get_claim(conn, to_claim_id) + if previous["released_at"] is None or successor["task_id"] != previous["task_id"]: + raise WorktreeError( + f"Claim {from_claim_id} does not hand its worktree to claim {to_claim_id}." + ) + if wt["branch"] != successor["branch"] or not Path(wt["path"]).exists(): + return None + try: + checked_out = _git(wt["path"], "rev-parse", "--symbolic-full-name", "HEAD") + except WorktreeError: + return None + if checked_out != f"refs/heads/{wt['branch']}": + return None + transfer_worktree(conn, wt["id"], to_claim_id) + audit.record( + conn, + "worktree.transferred", + "worktree", + wt["id"], + agent=agent, + detail={ + "path": wt["path"], + "from_claim_id": from_claim_id, + "to_claim_id": to_claim_id, + }, + ) + return {"id": wt["id"], "path": wt["path"], "branch": wt["branch"]} + + def _mark_removed( conn: Connection, wt: dict, diff --git a/src/quorumgit/work.py b/src/quorumgit/work.py index b79e416..565d24c 100644 --- a/src/quorumgit/work.py +++ b/src/quorumgit/work.py @@ -8,6 +8,8 @@ from __future__ import annotations import math +import posixpath +import re import subprocess from typing import Any @@ -43,6 +45,7 @@ def _lease_seconds(lease_hours: float) -> int: def create_task( conn: Connection, repository: str, title: str, objective: str = "" ) -> int: + begin_immediate(conn) repo = get_repository(conn, repository) row = conn.execute( """ @@ -191,13 +194,7 @@ def live_claims_in_repository( ).fetchall() result: list[dict] = [] for row in rows: - scopes = [ - r[0] - for r in conn.execute( - "SELECT path_glob FROM scopes WHERE claim_id = ? ORDER BY id", - (row[0],), - ).fetchall() - ] + scopes = claim_scopes(conn, row[0]) result.append( { "id": row[0], @@ -210,6 +207,16 @@ def live_claims_in_repository( return result +def claim_scopes(conn: Connection, claim_id: int) -> list[str]: + return [ + r[0] + for r in conn.execute( + "SELECT path_glob FROM scopes WHERE claim_id = ? ORDER BY id", + (claim_id,), + ).fetchall() + ] + + def open_handoff_for_task(conn: Connection, task_id: int) -> dict | None: """An open handoff reserving this task, if any.""" row = conn.execute( @@ -304,6 +311,30 @@ def live_claim_for_branch( # --------------------------------------------------------- scope comparison +def normalize_scope(glob: str) -> str: + """Canonical repository-relative spelling of a scope glob. + + `./src/**`, `src//**`, `/src/**` and `src\\**` all name `src/**`; a scope + naming the whole repository (`.`, `/`) becomes `**`. Never raises, so it is + safe on historical rows; validate_scope() rejects unusable input. + """ + text = glob.strip().replace("\\", "/") + if not text: + return "" + text = posixpath.normpath(text).lstrip("/") + return "**" if text in ("", ".") else text + + +def validate_scope(glob: str) -> str: + """Normalize a newly declared scope, refusing ones outside the repository.""" + scope = normalize_scope(glob) + if not scope: + raise ClaimRefused("Scopes must be non-empty path globs.") + if scope == ".." or scope.startswith("../"): + raise ClaimRefused(f"Scope {glob!r} escapes the repository root.") + return scope + + def _glob_prefix(glob: str) -> str: """Literal path prefix of a glob (up to the first wildcard character).""" for i, ch in enumerate(glob): @@ -313,30 +344,89 @@ def _glob_prefix(glob: str) -> str: def scopes_overlap(a: str, b: str) -> bool: - """Conservative overlap test that errs toward flagging.""" - pa, pb = _glob_prefix(a), _glob_prefix(b) + """Conservative overlap test that errs toward flagging. + + Both sides are normalized and compared case-insensitively, because a + case-insensitive checkout (macOS, Windows) treats `Src/` and `src/` as one + directory; on case-sensitive systems this only adds conservative flags. + """ + pa = _glob_prefix(normalize_scope(a)).casefold() + pb = _glob_prefix(normalize_scope(b)).casefold() return pa.startswith(pb) or pb.startswith(pa) +def _glob_regex(scope: str) -> re.Pattern[str]: + """Compile a normalized scope into a full-path matcher. + + `**` spans directories, `*` and `?` stay within one path segment, and + `[...]` is a character class. A scope without wildcards names a file or a + directory, so it also matches everything beneath it. + """ + out: list[str] = [] + i = 0 + while i < len(scope): + ch = scope[i] + if scope.startswith("**/", i): + out.append("(?:.*/)?") + i += 3 + elif scope.startswith("**", i): + out.append(".*") + i += 2 + elif ch == "*": + out.append("[^/]*") + i += 1 + elif ch == "?": + out.append("[^/]") + i += 1 + elif ch == "[" and (end := scope.find("]", i + 2)) != -1 and ( + scope[i + 1 : end] != "!" + ): + body = scope[i + 1 : end] + negate = body.startswith("!") + body = re.sub(r"([\\^\[\]])", r"\\\1", body[1:] if negate else body) + out.append(f"[{'^' if negate else ''}{body}]") + i = end + 1 + else: + out.append(re.escape(ch)) + i += 1 + pattern = "".join(out) + if not any(c in scope for c in "*?["): + pattern += "(?:/.*)?" + return re.compile(pattern + r"\Z", re.DOTALL) + + +def path_in_scopes(path: str, scopes: list[str]) -> bool: + """True when a repository path is covered by any of the declared scopes.""" + return any(_glob_regex(normalize_scope(s)).match(path) for s in scopes) + + def verify_commit( repo_path: str, commit_oid: str, branch: str | None = None -) -> None: - """Require an existing commit, reachable from the branch when it exists.""" - exists = ( - subprocess.run( - [ - "git", - "-C", - str(repo_path), - "cat-file", - "-e", - f"{commit_oid}^{{commit}}", - ], - capture_output=True, - ).returncode - == 0 +) -> str: + """Resolve a commit to its full object ID and verify it as a continuation. + + Accepts anything Git resolves to a commit (full or abbreviated OID, a ref) + and returns the canonical lowercase full OID. The commit must exist in the + registered repository and be reachable from the branch when it exists. + """ + spec = commit_oid.strip() + if not spec or spec.startswith("-"): + raise WorkError(f"Invalid commit: {commit_oid!r}") + resolved = subprocess.run( + [ + "git", + "-C", + str(repo_path), + "rev-parse", + "--verify", + "--quiet", + f"{spec}^{{commit}}", + ], + capture_output=True, + text=True, ) - if not exists: + oid = resolved.stdout.strip().lower() + if resolved.returncode != 0 or not re.fullmatch(r"[0-9a-f]{40}|[0-9a-f]{64}", oid): raise WorkError( f"Commit {commit_oid} does not exist in the registered repository." ) @@ -365,7 +455,7 @@ def verify_commit( str(repo_path), "merge-base", "--is-ancestor", - commit_oid, + oid, f"refs/heads/{branch}", ], capture_output=True, @@ -377,6 +467,7 @@ def verify_commit( f"Commit {commit_oid} is not reachable from branch " f"{branch!r} in the registered repository." ) + return oid def classify( @@ -455,6 +546,7 @@ def claim_task( agent_row = get_agent(conn, agent) if not scope_globs: raise ClaimRefused("At least one --scope is required to claim a task.") + scope_globs = list(dict.fromkeys(validate_scope(g) for g in scope_globs)) if not via_handoff: pending = open_handoff_for_task(conn, task_id) @@ -729,7 +821,9 @@ def add_checkpoint( f"Claim {claim_id} belongs to {claim['agent']}, not {agent}." ) task = get_task(conn, claim["task_id"]) - verify_commit(task["repository_path"], commit_oid, branch=claim["branch"]) + commit_oid = verify_commit( + task["repository_path"], commit_oid, branch=claim["branch"] + ) row = conn.execute( """ INSERT INTO checkpoints (claim_id, commit_oid, note, detail) @@ -747,3 +841,12 @@ def add_checkpoint( detail={"commit": commit_oid, "note": note}, ) return row[0] + + +def checkpoint_commit(conn: Connection, checkpoint_id: int) -> str: + row = conn.execute( + "SELECT commit_oid FROM checkpoints WHERE id = ?", (checkpoint_id,) + ).fetchone() + if row is None: + raise WorkError(f"No such checkpoint: {checkpoint_id}") + return row[0] diff --git a/tests/test_governance_fixes.py b/tests/test_governance_fixes.py new file mode 100644 index 0000000..163820a --- /dev/null +++ b/tests/test_governance_fixes.py @@ -0,0 +1,478 @@ +"""Regression tests for the second review round. + +1. An agent cannot authorize its own protected operation or takeover. +2. A successor claim on the same branch inherits the superseded worktree. +3. Scope globs are normalized before overlap comparison. +4. Pushes to a claimed branch are held to the claim's declared scopes. +5. Commit specs resolve to full OIDs and user errors never print tracebacks. +6. `handoff create --last-commit` is honored when a worktree exists. +""" + +from __future__ import annotations + +import os +import re +import subprocess +import uuid +from pathlib import Path + +import pytest + +from quorumgit import gate, handoff, registry, trees, work +from tests.conftest import make_git_repo +from tests.test_cli_hub import _cli +from tests.test_gate import _commit, _push, _setup + +GIT_ENV = { + **os.environ, + "GIT_AUTHOR_NAME": "t", + "GIT_AUTHOR_EMAIL": "t@localhost", + "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@localhost", +} + + +def _git(path: Path, *args: str) -> str: + return subprocess.run( + ["git", "-C", str(path), *args], + check=True, + capture_output=True, + text=True, + env=GIT_ENV, + ).stdout.strip() + + +def _local_setup(conn, tmp_path, agents=("a", "b", "op")): + """A registered working repository (local model) plus named agents.""" + suffix = uuid.uuid4().hex[:8] + repo_path = make_git_repo(tmp_path / f"repo-{suffix}") + repo = f"local-{suffix}" + registry.add_repository(conn, repo, repo_path) + names = [f"{agent}-{suffix}" for agent in agents] + for name in names: + registry.add_agent(conn, name) + task = work.create_task(conn, repo, "work") + conn.commit() + return repo, repo_path, task, names + + +def _claim_id(output: str) -> int: + match = re.search(r"claim (\d+) acquired", output) + assert match is not None, output + return int(match.group(1)) + + +def _no_traceback(result) -> None: + assert "Traceback" not in result.stderr, result.stderr + assert result.returncode == 1, (result.stdout, result.stderr) + assert result.stderr.startswith("[quorumgit] ERROR:"), result.stderr + + +# ---------------------------------------------------------------- finding 1 + + +def _register(conn, *names): + for name in names: + conn.execute( + "INSERT INTO agents (name) VALUES (?) ON CONFLICT DO NOTHING", (name,) + ) + + +def _protected_op(): + return { + "type": "protected_ref_update", + "repository": "self-approval", + "refname": "refs/heads/main", + "n": uuid.uuid4().hex, + } + + +def test_consumer_cannot_use_its_own_approving_vote(conn): + _register(conn, "pusher", "operator") + op = _protected_op() + approval = gate.request_approval(conn, op, requested_by="pusher") + assert gate.vote(conn, approval["id"], "pusher", True)["status"] == "approved" + + with pytest.raises(gate.GateError, match="cannot authorize its own"): + gate.consume_approval(conn, approval["id"], op, agent="pusher") + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "approved" + + # An independent approver can still vote on the approved instance, after + # which the same approval becomes usable by the pusher. + gate.vote(conn, approval["id"], "operator", True) + gate.consume_approval(conn, approval["id"], op, agent="pusher") + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "consumed" + + +def test_threshold_counts_only_independent_votes(conn): + _register(conn, "pusher", "op-1", "op-2") + op = _protected_op() + approval = gate.request_approval(conn, op, requested_by="op-1", threshold=2) + gate.vote(conn, approval["id"], "pusher", True) + gate.vote(conn, approval["id"], "op-1", True) + with pytest.raises(gate.GateError, match="needs 2 approving"): + gate.consume_approval(conn, approval["id"], op, agent="pusher") + gate.vote(conn, approval["id"], "op-2", True) + gate.consume_approval(conn, approval["id"], op, agent="pusher") + + +def test_deny_revokes_an_approved_but_unused_approval(conn): + _register(conn, "pusher", "op-1", "op-2") + op = _protected_op() + approval = gate.request_approval(conn, op, requested_by="op-1") + gate.vote(conn, approval["id"], "op-1", True) + assert gate.vote(conn, approval["id"], "op-2", False)["status"] == "denied" + with pytest.raises(gate.GateError, match="not consumable"): + gate.consume_approval(conn, approval["id"], op, agent="pusher") + + +def test_takeover_beneficiary_cannot_approve_itself(conn, tmp_path): + repo, _path, task, (a, b, op) = _local_setup(conn, tmp_path) + claim_id, _, _ = work.claim_task(conn, task, a, "feat/self", ["src/**"]) + operation = { + "type": "lease_takeover", + "repository": repo, + "task_id": task, + "from_claim_id": claim_id, + "from_agent": a, + "to_agent": b, + } + approval = gate.request_approval(conn, operation, requested_by=b) + with pytest.raises(gate.GateError, match="authorizes itself"): + gate.vote(conn, approval["id"], b, True) + assert gate.vote(conn, approval["id"], op, True)["status"] == "approved" + + +def test_hook_rejects_self_approved_protected_push(committed_conn, tmp_path, cfg): + conn = committed_conn + repo_name, hub, clone, a, b = _setup(conn, tmp_path) + _commit(clone, "newfile.txt") + op = { + "type": "protected_ref_update", + "repository": repo_name, + "refname": "refs/heads/main", + "oldrev": _git(Path(hub), "rev-parse", "refs/heads/main"), + "newrev": _git(clone, "rev-parse", "HEAD"), + } + approval = gate.request_approval(conn, op, requested_by=a) + gate.vote(conn, approval["id"], a, True) + conn.commit() + + self_approved = _push(clone, a, "main", cfg=cfg) + assert self_approved.returncode != 0 + assert "cannot authorize its own" in self_approved.stderr + + gate.vote(conn, approval["id"], b, True) + conn.commit() + accepted = _push(clone, a, "main", cfg=cfg) + assert accepted.returncode == 0, accepted.stderr + + +# ---------------------------------------------------------------- finding 2 + + +def test_reclaim_of_expired_task_inherits_same_branch_worktree( + committed_conn, tmp_path, cfg +): + conn = committed_conn + _repo, _path, task, (a, b, _op) = _local_setup(conn, tmp_path) + first = _cli(cfg, "claim", str(task), "--branch", "feat/inherit", + "--scope", "src/**", agent=a) + assert first.returncode == 0, first.stderr + old_claim = _claim_id(first.stdout) + old_wt = trees.worktree_for_claim(conn, old_claim) + assert old_wt is not None + (Path(old_wt["path"]) / "src" / "wip.py").write_text("uncommitted\n") + + conn.execute( + "UPDATE claims SET lease_expires_at = unixepoch() - 10 WHERE id = ?", + (old_claim,), + ) + conn.commit() + + second = _cli(cfg, "claim", str(task), "--branch", "feat/inherit", + "--scope", "src/**", agent=b) + assert second.returncode == 0, second.stderr + assert f"inherited from claim {old_claim}" in second.stdout + new_claim = _claim_id(second.stdout) + + moved = trees.worktree_for_claim(conn, new_claim) + assert moved is not None and moved["path"] == old_wt["path"] + assert trees.worktree_for_claim(conn, old_claim) is None + assert (Path(moved["path"]) / "src" / "wip.py").read_text() == "uncommitted\n" + events = conn.execute( + "SELECT count(*) FROM audit_events " + "WHERE event_type = 'worktree.transferred' AND entity_id = ?", + (moved["id"],), + ).fetchone() + assert events is not None and events[0] == 1 + + +def test_approved_takeover_on_same_branch_inherits_worktree( + committed_conn, tmp_path, cfg +): + conn = committed_conn + repo, _path, task, (a, b, op) = _local_setup(conn, tmp_path) + first = _cli(cfg, "claim", str(task), "--branch", "feat/take", + "--scope", "src/**", agent=a) + assert first.returncode == 0, first.stderr + old_claim = _claim_id(first.stdout) + old_path = trees.worktree_for_claim(conn, old_claim)["path"] + + operation = { + "type": "lease_takeover", + "repository": repo, + "task_id": task, + "from_claim_id": old_claim, + "from_agent": a, + "to_agent": b, + } + approval = gate.request_approval(conn, operation, requested_by=op) + gate.vote(conn, approval["id"], op, True) + conn.commit() + + taken = _cli(cfg, "claim", str(task), "--branch", "feat/take", + "--scope", "src/**", "--takeover", agent=b) + assert taken.returncode == 0, taken.stderr + new_claim = _claim_id(taken.stdout) + assert trees.worktree_for_claim(conn, new_claim)["path"] == old_path + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "consumed" + + +def test_reclaim_on_a_different_branch_creates_a_fresh_worktree( + committed_conn, tmp_path, cfg +): + conn = committed_conn + _repo, _path, task, (a, b, _op) = _local_setup(conn, tmp_path) + first = _cli(cfg, "claim", str(task), "--branch", "feat/old", + "--scope", "src/**", agent=a) + assert first.returncode == 0, first.stderr + old_claim = _claim_id(first.stdout) + conn.execute( + "UPDATE claims SET lease_expires_at = unixepoch() - 10 WHERE id = ?", + (old_claim,), + ) + conn.commit() + + second = _cli(cfg, "claim", str(task), "--branch", "feat/new", + "--scope", "src/**", agent=b) + assert second.returncode == 0, second.stderr + assert "inherited" not in second.stdout + new_claim = _claim_id(second.stdout) + new_wt = trees.worktree_for_claim(conn, new_claim) + assert new_wt is not None and new_wt["branch"] == "feat/new" + assert trees.worktree_for_claim(conn, old_claim)["removed_at"] is None + + +# ---------------------------------------------------------------- finding 3 + + +@pytest.mark.parametrize( + ("raw", "normalized"), + [ + ("./src/**", "src/**"), + ("src//api/**", "src/api/**"), + ("/src/**", "src/**"), + ("src\\api\\**", "src/api/**"), + (".", "**"), + ("src/../lib/**", "lib/**"), + ], +) +def test_normalize_scope(raw, normalized): + assert work.normalize_scope(raw) == normalized + + +@pytest.mark.parametrize( + ("a", "b"), + [ + ("./src/**", "src/**"), + ("src\\api\\**", "src/api/handlers.py"), + ("Src/**", "src/app.py"), + ("src/../lib/**", "lib/x.py"), + ], +) +def test_differently_spelled_scopes_overlap(a, b): + assert work.scopes_overlap(a, b) + assert work.scopes_overlap(b, a) + + +def test_scopes_escaping_the_repository_are_refused(conn, tmp_path): + _repo, _path, task, (a, _b, _op) = _local_setup(conn, tmp_path) + with pytest.raises(work.ClaimRefused, match="escapes the repository"): + work.claim_task(conn, task, a, "feat/escape", ["../other/**"]) + + +def test_dot_slash_scope_is_classified_overlapping(conn, tmp_path): + repo, _path, task, (a, b, _op) = _local_setup(conn, tmp_path) + work.claim_task(conn, task, a, "feat/one", ["src/**"]) + other = work.create_task(conn, repo, "second") + with pytest.raises(work.ClaimRefused, match="overlap"): + work.claim_task(conn, other, b, "feat/two", ["./src/**"]) + + claim_id, classification, _ = work.claim_task( + conn, other, b, "feat/two", ["./src/**"], override_overlap=True + ) + assert classification == "OVERLAPPING" + assert work.claim_scopes(conn, claim_id) == ["src/**"] + + +# ---------------------------------------------------------------- finding 4 + + +@pytest.mark.parametrize( + ("scope", "path", "inside"), + [ + ("src/**", "src/a/b.py", True), + ("src/**", "srcx/a.py", False), + ("src/*.py", "src/a.py", True), + ("src/*.py", "src/a/b.py", False), + ("docs", "docs/guide.md", True), + ("docs", "docs", True), + ("docs", "docs2/x", False), + ("**/*.md", "README.md", True), + ("**/*.md", "a/b/c.md", True), + ("src/[ab].py", "src/a.py", True), + ("src/[!ab].py", "src/a.py", False), + ("**", "anything/at/all", True), + ], +) +def test_path_in_scopes(scope, path, inside): + assert work.path_in_scopes(path, [scope]) is inside + + +def test_hook_holds_claimed_branch_to_its_scopes(committed_conn, tmp_path, cfg): + conn = committed_conn + repo_name, _hub, clone, a, b = _setup(conn, tmp_path) + task = work.create_task(conn, repo_name, "scoped work") + work.claim_task(conn, task, a, branch="feat/scoped", scope_globs=["src/**"]) + conn.commit() + + _commit(clone, "src/inside.py", branch="feat/scoped") + inside = _push(clone, a, "feat/scoped", cfg=cfg) + assert inside.returncode == 0, inside.stderr + + _commit(clone, "README.md", branch="feat/scoped") + outside = _push(clone, a, "feat/scoped", cfg=cfg) + assert outside.returncode != 0 + assert "outside claim" in outside.stderr + assert "README.md" in outside.stderr + + # Undo the out-of-scope commit; merging another branch's accepted work in + # does not attribute that branch's files to the claim holder. + _git(clone, "reset", "--hard", "HEAD~1") + _commit(clone, "docs/elsewhere.md", branch="feat/base") + base = _push(clone, b, "feat/base", cfg=cfg) + assert base.returncode == 0, base.stderr + _git(clone, "checkout", "feat/scoped") + _git(clone, "merge", "--no-edit", "feat/base") + merged = _push(clone, a, "feat/scoped", cfg=cfg) + assert merged.returncode == 0, merged.stderr + + +def test_hook_scope_check_covers_new_claimed_branches(committed_conn, tmp_path, cfg): + conn = committed_conn + repo_name, _hub, clone, a, _b = _setup(conn, tmp_path) + task = work.create_task(conn, repo_name, "new branch") + work.claim_task(conn, task, a, branch="feat/fresh", scope_globs=["src/**"]) + conn.commit() + + _commit(clone, "docs/outside.md", branch="feat/fresh") + rejected = _push(clone, a, "feat/fresh", cfg=cfg) + assert rejected.returncode != 0 + assert "docs/outside.md" in rejected.stderr + + +# ---------------------------------------------------------------- finding 5 + + +def test_checkpoint_accepts_abbreviated_commit(committed_conn, tmp_path, cfg): + conn = committed_conn + _repo, repo_path, task, (a, _b, _op) = _local_setup(conn, tmp_path) + claimed = _cli(cfg, "claim", str(task), "--branch", "feat/short", + "--scope", "src/**", "--no-worktree", agent=a) + assert claimed.returncode == 0, claimed.stderr + claim_id = _claim_id(claimed.stdout) + full = _git(repo_path, "rev-parse", "main") + + for spec in (full[:9], full.upper(), "main"): + result = _cli(cfg, "checkpoint", str(claim_id), "--commit", spec, agent=a) + assert result.returncode == 0, result.stderr + assert full in result.stdout + + bogus = _cli(cfg, "checkpoint", str(claim_id), "--commit", "nope", agent=a) + _no_traceback(bogus) + assert "does not exist" in bogus.stderr + + +def test_duplicate_registration_is_a_clean_error(committed_conn, tmp_path, cfg): + conn = committed_conn + _repo, _path, _task, (a, _b, _op) = _local_setup(conn, tmp_path) + result = _cli(cfg, "agent", "add", a) + _no_traceback(result) + assert "already registered" in result.stderr + + +def test_duplicate_protected_ref_is_deduplicated(committed_conn, tmp_path): + conn = committed_conn + repo_path = make_git_repo(tmp_path / "dup-refs") + name = f"dup-{uuid.uuid4().hex[:8]}" + registry.add_repository( + conn, name, repo_path, + protected_refs=["refs/heads/main", "refs/heads/main"], + ) + assert registry.get_repository(conn, name)["protected_refs"] == ["refs/heads/main"] + + +@pytest.mark.parametrize("payload", ["{bad", "[1, 2]"]) +def test_invalid_operation_json_is_a_clean_error(cfg, initialized_store, payload): + result = _cli(cfg, "approve", "hash", payload) + _no_traceback(result) + assert "Operation" in result.stderr + + +# ---------------------------------------------------------------- finding 6 + + +def test_explicit_last_commit_wins_over_worktree_head(committed_conn, tmp_path, cfg): + conn = committed_conn + _repo, _path, task, (a, _b, _op) = _local_setup(conn, tmp_path) + claimed = _cli(cfg, "claim", str(task), "--branch", "feat/hand", + "--scope", "src/**", agent=a) + assert claimed.returncode == 0, claimed.stderr + claim_id = _claim_id(claimed.stdout) + wt_path = Path(trees.worktree_for_claim(conn, claim_id)["path"]) + base = _git(wt_path, "rev-parse", "HEAD") + (wt_path / "src" / "more.py").write_text("x\n") + _git(wt_path, "add", "-A") + _git(wt_path, "commit", "-m", "later work") + assert _git(wt_path, "rev-parse", "HEAD") != base + + created = _cli(cfg, "handoff", "create", str(claim_id), "--completed", "c", + "--remaining", "r", "--last-commit", base[:10], agent=a) + assert created.returncode == 0, created.stderr + assert base in created.stdout + hid = int(re.search(r"handoff (\d+) created", created.stdout).group(1)) + assert handoff.get_handoff(conn, hid)["record"]["last_commit"] == base + + +def test_handoff_after_worktree_removal_uses_last_commit( + committed_conn, tmp_path, cfg +): + conn = committed_conn + _repo, repo_path, task, (a, _b, _op) = _local_setup(conn, tmp_path) + claimed = _cli(cfg, "claim", str(task), "--branch", "feat/gone", + "--scope", "src/**", agent=a) + assert claimed.returncode == 0, claimed.stderr + claim_id = _claim_id(claimed.stdout) + trees.remove_worktree(conn, claim_id, a) + conn.commit() + + missing = _cli(cfg, "handoff", "create", str(claim_id), "--completed", "c", + "--remaining", "r", agent=a) + assert missing.returncode == 1 + assert "--last-commit" in missing.stderr + + head = _git(repo_path, "rev-parse", "feat/gone") + created = _cli(cfg, "handoff", "create", str(claim_id), "--completed", "c", + "--remaining", "r", "--last-commit", head, agent=a) + assert created.returncode == 0, created.stderr From 716c335467add9fd9ef778a2118e41ae37fe8a90 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 08:28:33 +0000 Subject: [PATCH 2/6] Narrow Optional worktree lookups in governance tests for pyright Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Q8BpCkArA34hB1aSyVcnZp --- tests/test_governance_fixes.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tests/test_governance_fixes.py b/tests/test_governance_fixes.py index 163820a..6e9ca42 100644 --- a/tests/test_governance_fixes.py +++ b/tests/test_governance_fixes.py @@ -62,6 +62,12 @@ def _claim_id(output: str) -> int: return int(match.group(1)) +def _worktree(conn, claim_id: int) -> dict: + wt = trees.worktree_for_claim(conn, claim_id) + assert wt is not None, f"claim {claim_id} has no worktree" + return wt + + def _no_traceback(result) -> None: assert "Traceback" not in result.stderr, result.stderr assert result.returncode == 1, (result.stdout, result.stderr) @@ -217,7 +223,7 @@ def test_approved_takeover_on_same_branch_inherits_worktree( "--scope", "src/**", agent=a) assert first.returncode == 0, first.stderr old_claim = _claim_id(first.stdout) - old_path = trees.worktree_for_claim(conn, old_claim)["path"] + old_path = _worktree(conn, old_claim)["path"] operation = { "type": "lease_takeover", @@ -235,7 +241,7 @@ def test_approved_takeover_on_same_branch_inherits_worktree( "--scope", "src/**", "--takeover", agent=b) assert taken.returncode == 0, taken.stderr new_claim = _claim_id(taken.stdout) - assert trees.worktree_for_claim(conn, new_claim)["path"] == old_path + assert _worktree(conn, new_claim)["path"] == old_path assert gate.get_approval_by_id(conn, approval["id"])["status"] == "consumed" @@ -261,7 +267,7 @@ def test_reclaim_on_a_different_branch_creates_a_fresh_worktree( new_claim = _claim_id(second.stdout) new_wt = trees.worktree_for_claim(conn, new_claim) assert new_wt is not None and new_wt["branch"] == "feat/new" - assert trees.worktree_for_claim(conn, old_claim)["removed_at"] is None + assert _worktree(conn, old_claim)["removed_at"] is None # ---------------------------------------------------------------- finding 3 @@ -440,7 +446,7 @@ def test_explicit_last_commit_wins_over_worktree_head(committed_conn, tmp_path, "--scope", "src/**", agent=a) assert claimed.returncode == 0, claimed.stderr claim_id = _claim_id(claimed.stdout) - wt_path = Path(trees.worktree_for_claim(conn, claim_id)["path"]) + wt_path = Path(_worktree(conn, claim_id)["path"]) base = _git(wt_path, "rev-parse", "HEAD") (wt_path / "src" / "more.py").write_text("x\n") _git(wt_path, "add", "-A") @@ -451,7 +457,9 @@ def test_explicit_last_commit_wins_over_worktree_head(committed_conn, tmp_path, "--remaining", "r", "--last-commit", base[:10], agent=a) assert created.returncode == 0, created.stderr assert base in created.stdout - hid = int(re.search(r"handoff (\d+) created", created.stdout).group(1)) + match = re.search(r"handoff (\d+) created", created.stdout) + assert match is not None, created.stdout + hid = int(match.group(1)) assert handoff.get_handoff(conn, hid)["record"]["last_commit"] == base From 44dc3202d11af70734d854aa843e2fe1f79baeb3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 08:45:17 +0000 Subject: [PATCH 3/6] Bind push authorization to Git's ref transaction; verify full schema Pre-receive used to consume approvals and commit before Git updated the ref, so an update Git later refused still spent its approval, and a claim release, takeover, or handoff between pre-receive and the ref update let a push land on authority that no longer held. - pre-receive now validates and records each exact ref update (ref_updates, migration 003) without consuming anything. - A managed reference-transaction hook re-validates every recorded update at the `prepared` stage, while Git holds the ref locks and can still abort, and consumes approvals there. Failure aborts the update. `committed`/`aborted` record the outcome, and an abort after `prepared` restores the consumed approval. - pre-receive refuses pushes when the transaction hook is missing or modified; hook install writes both hooks and inspects both before writing either. Ref transactions no push validated pass through. - Repository identity lookups no longer inherit the hook's GIT_DIR, which resolved every registered path to the invoking repository. verify_contract() now replays the bundled migrations in memory and requires every table, index, and trigger to match, rejecting missing or altered objects (append-only audit triggers, single-owner and single live approval unique indexes, identity triggers), unexpected indexes or triggers, and stores migrated by a newer version. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Q8BpCkArA34hB1aSyVcnZp --- README.md | 18 +- src/quorumgit/cli.py | 22 + src/quorumgit/gate.py | 522 ++++++++++++++++--- src/quorumgit/migrations/003_ref_updates.sql | 37 ++ src/quorumgit/registry.py | 24 + src/quorumgit/store.py | 94 +++- tests/test_authorization_binding.py | 25 +- tests/test_transaction_boundary.py | 343 ++++++++++++ 8 files changed, 978 insertions(+), 107 deletions(-) create mode 100644 src/quorumgit/migrations/003_ref_updates.sql create mode 100644 tests/test_transaction_boundary.py diff --git a/README.md b/README.md index fc61479..40e5f94 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ QuorumGit prevents coding agents (or humans) working the same codebase from step agent-one ──┐ agent-two ──┼──► quorumgit CLI ──► local libSQL file (claims, leases, approvals, audit) reviewer ──┘ │ - └──► git pre-receive hook (push enforcement) + └──► git pre-receive + reference-transaction hooks (push enforcement) ``` ## The problem it solves @@ -41,7 +41,7 @@ Everything an agent does — claim, renew, checkpoint, hand off, release — wri ## Installation -Requirements: **Python 3.11–3.14**, **git**. There is no database to install, no server to run, and no extension to provision: the store is a single [libSQL](https://github.com/tursodatabase/libsql-python) database file under `QUORUMGIT_DATA_DIR`. +Requirements: **Python 3.11–3.14**, **git 2.28+** (for the reference-transaction hook). There is no database to install, no server to run, and no extension to provision: the store is a single [libSQL](https://github.com/tursodatabase/libsql-python) database file under `QUORUMGIT_DATA_DIR`. > **Platform note.** Linux, macOS, and Windows are supported. `libsql` 0.1.11 publishes a prebuilt wheel for CPython 3.11–3.13 on all three, and for CPython 3.14 on Linux only — on macOS 3.14 pip builds the extension from source (needs a Rust toolchain), and on Windows 3.14 that build does not currently succeed. Use 3.11–3.13 on Windows until upstream ships a 3.14 wheel. @@ -58,11 +58,11 @@ Then initialize the store once: ```bash $ quorumgit init store: /home/you/.quorumgit/quorumgit.db -migrations applied: ['001_core.sql', '002_approval_identities.sql'] +migrations applied: ['001_core.sql', '002_approval_identities.sql', '003_ref_updates.sql'] contract: ok ``` -`init` creates the state directory, applies migrations, and verifies the runtime contract (required tables, foreign keys on, WAL journal mode). It is idempotent — re-run it any time. +`init` creates the state directory, applies migrations, and verifies the runtime contract: required tables and migrations, foreign keys on, WAL journal mode, and **every schema object the governance rules depend on**. The expected schema is derived by replaying the bundled migrations in memory, so a store whose append-only audit triggers, single-owner/single-live-approval unique indexes, or identity triggers are missing or altered — or that carries extra indexes or triggers — fails the contract instead of silently losing an invariant. Every command runs this check. It is idempotent — re-run it any time. `quorumgit status` shows the store URI, contract state, and row counts. If the store is down or incomplete, every command exits non-zero: there is **one storage backend and no fallback**, by design. @@ -132,7 +132,7 @@ Where the registered repository sits determines how enforcement works. Pick one **Local model** — the registered repo is a working checkout on the same machine. Claims create isolated worktrees; agents commit directly in them. Coordination is entirely claim-based; no pushes, no hook needed. This is the default and the simplest setup — the quick start above is the local model. -**Hub model** — the registered repo is a **bare** hub that agents push to from their own clones. Enforcement moves to a pre-receive hook: +**Hub model** — the registered repo is a **bare** hub that agents push to from their own clones. Enforcement moves to a pair of Git hooks: ```bash git clone --bare /path/to/source myproject.git @@ -151,6 +151,8 @@ QUORUMGIT_AGENT=agent-two git push origin feat/x # rejected — branch is cla git push origin feat/x # rejected — unidentified ``` +**Authorization is bound to the ref update itself.** `pre-receive` checks the push and records each exact ref update it accepted, but spends nothing. Git's `reference-transaction` hook then runs at the `prepared` stage — after Git has locked the refs and while it can still abort — re-checks every rule against the store's current state, and only then consumes any approval. If ownership changed after `pre-receive` (a release, takeover, or handoff), the update is aborted. If Git refuses the update after `pre-receive` (another hook, a lost ref lock), no approval is spent; if Git aborts after `prepared`, the consumed approval is restored. The `committed`/`aborted` outcome of every governed update is recorded. `pre-receive` refuses pushes when the transaction hook is missing or modified, and ref transactions that no push validated (local maintenance in the hub) pass through. + **Scopes are enforced at push time.** When a branch has a live claim, every commit the push introduces must only touch paths inside that claim's scopes (`**` spans directories, `*` stays within one; a scope without wildcards covers that file or directory). Merge commits count only the paths the merge itself changed, so merging the base branch in is not attributed to the claim holder. In the local model agents commit directly in their worktree, so no hook runs and scopes remain coordination metadata. Checkpoints in the hub model take an explicit `--commit ` (any commit spec Git resolves — a full or abbreviated OID or a ref — recorded as the full OID). Continuation points are **verified, not trusted**: the commit must exist in the registered repository, and when the claimed branch exists, be reachable from it. A typo'd or fabricated OID is rejected. @@ -173,7 +175,7 @@ quorumgit approve request '{"type":"protected_ref_update","repository":"myprojec quorumgit approve vote 17 --agent operator # 3. The same push now lands. Pushing it again — or replaying the approval — is rejected: -# the approval was consumed atomically when the operation was accepted. +# the approval was consumed while Git held the ref lock, as the update committed. ``` Rules that hold no matter what: @@ -211,7 +213,7 @@ Takeovers follow the same pattern: claiming a task someone else holds (`claim [--threshold ]` | Open an approval for an exact operation | | `quorumgit approve vote [--deny]` | Vote on one approval instance | | `quorumgit approve hash ` | Compute an operation's hash | -| `quorumgit hook install --repo ` | Install the pre-receive hook (hub model) | +| `quorumgit hook install --repo ` | Install the pre-receive and reference-transaction hooks (hub model) | | `quorumgit audit [--entity ] [--entity-id ] [--limit ]` | Read the audit trail | `repo list`, `agent list`, and `task list [--repo ]` enumerate what's registered. Exit codes: `0` success, `1` refused/violation/error, `2` usage error. @@ -241,7 +243,7 @@ QuorumGit v1 coordinates **cooperating agents**; the adversary is *accident, not - Agent identity is asserted (`QUORUMGIT_AGENT`), not cryptographically authenticated. Any local process can claim to be any agent. - The trust root is write access to the database and the filesystem. An actor with either can bypass governance. -- The pre-receive hook governs `git push` only. Direct ref manipulation inside a repository bypasses it. +- The hooks govern `git push` only. Direct ref manipulation inside a repository bypasses them. - Approval hashes provide exact-payload binding and tamper-evidence, not approver authentication. These are the correct trade-offs for preventing well-intentioned agents from colliding on one machine. They are not Byzantine fault tolerance, and this document will not pretend otherwise. diff --git a/src/quorumgit/cli.py b/src/quorumgit/cli.py index 84ddb92..c2c2b85 100644 --- a/src/quorumgit/cli.py +++ b/src/quorumgit/cli.py @@ -414,8 +414,11 @@ def cmd_approve_hash(args, cfg) -> int: def cmd_hook_install(args, cfg) -> int: with store.connect(cfg) as conn: path = gate.install_hook(conn, args.repo) + repo_path = registry.get_repository(conn, args.repo)["path"] conn.commit() print(f"pre-receive hook installed: {path}") + print("reference-transaction hook installed: " + f"{gate._effective_hook(repo_path, 'reference-transaction')}") return 0 @@ -424,6 +427,21 @@ def cmd_hook_pre_receive(args, cfg) -> int: return gate.run_pre_receive(conn, args.repo, sys.stdin) +def cmd_hook_reference_transaction(args, cfg) -> int: + if not store.database_path(cfg).exists(): + # A governed push cannot reach here without a store: its pre-receive + # needed one. Without one this is local ref maintenance. + return 0 + try: + conn = store.connect(cfg) + except store.StoreError as exc: + label = "REJECTED" if args.state == "prepared" else f"WARNING ({args.state})" + print(f"[quorumgit] {label}: {exc}", file=sys.stderr) + return 1 + with conn: + return gate.run_reference_transaction(conn, args.repo, args.state, sys.stdin) + + # -------------------------------------------------------------------- audit @@ -563,6 +581,10 @@ def add(name, fn, configure=None, parent=None): lambda sp: sp.add_argument("--repo", required=True), parent=hook) add("pre-receive", cmd_hook_pre_receive, lambda sp: sp.add_argument("--repo", required=True), parent=hook) + add("reference-transaction", cmd_hook_reference_transaction, lambda sp: ( + sp.add_argument("--repo", required=True), + sp.add_argument("state"), + ), parent=hook) add("audit", cmd_audit, lambda sp: ( sp.add_argument("--entity"), diff --git a/src/quorumgit/gate.py b/src/quorumgit/gate.py index d968757..21784c7 100644 --- a/src/quorumgit/gate.py +++ b/src/quorumgit/gate.py @@ -7,6 +7,7 @@ from __future__ import annotations import os +import re import shlex import subprocess import sys @@ -447,7 +448,29 @@ def _verify_repository_binding( return repo -def check_ref_update( +def _required_approval( + conn: Connection, operation: dict[str, Any], pusher: str +) -> dict: + """The approval that authorizes `operation` for `pusher`, or PushRejected.""" + approval = approved_instance(conn, operation) + if approval is None: + raise PushRejected( + f"{operation['type']} on {operation['refname']} requires an approval " + f"bound to this exact update (hash {operation_hash(operation)})." + ) + independent = _independent_yes_votes( + conn, approval["id"], get_agent(conn, pusher)["id"] + ) + if independent < approval["threshold"]: + raise PushRejected( + f"Approval {approval['id']} needs {approval['threshold']} approving " + f"vote(s) from agents other than {pusher}, who is pushing; it has " + f"{independent}. An agent cannot authorize its own operation." + ) + return approval + + +def evaluate_ref_update( conn: Connection, repository: str, git_dir: str, @@ -455,8 +478,14 @@ def check_ref_update( oldrev: str, newrev: str, refname: str, -) -> None: - """Enforce governance for one ref update. Raises PushRejected.""" +) -> tuple[dict, dict[str, Any] | None, dict | None]: + """Apply every push rule to one ref update without changing any state. + + Returns (repository, protected operation or None, its approval or None). + Raises PushRejected. Runs twice per governed update: in pre-receive, and + again at the reference transaction's `prepared` stage against the state + current while Git holds the ref locks. + """ repo = _verify_repository_binding(conn, repository, git_dir) branch = refname.removeprefix("refs/heads/") @@ -484,42 +513,72 @@ def check_ref_update( and not _is_zero(oldrev) and not _is_fast_forward(git_dir, oldrev, newrev) ) + if not (protected or deletion or forced): + return repo, None, None + + operation = { + "type": "protected_ref_update" + if protected + else ("ref_delete" if deletion else "force_update"), + "repository": repository, + "refname": refname, + "oldrev": oldrev, + "newrev": newrev, + } + if pusher is None: + raise PushRejected("Pusher identity required for a protected update.") + return repo, operation, _required_approval(conn, operation, pusher) - if protected or deletion or forced: - operation = { - "type": "protected_ref_update" - if protected - else ("ref_delete" if deletion else "force_update"), - "repository": repository, - "refname": refname, - "oldrev": oldrev, - "newrev": newrev, - } - approval = approved_instance(conn, operation) - if approval is None: - raise PushRejected( - f"{operation['type']} on {refname} requires an approval " - f"bound to this exact update (hash {operation_hash(operation)})." - ) - assert pusher is not None - consume_approval(conn, approval["id"], operation, agent=pusher) - audit.record( - conn, - "gate.protected_update_allowed", - "repository", - repo["id"], - agent=pusher, - detail=operation, - ) - return +def check_ref_update( + conn: Connection, + repository: str, + git_dir: str, + pusher: str, + oldrev: str, + newrev: str, + refname: str, +) -> None: + """pre-receive: validate one ref update and record it for its transaction. + + Nothing is consumed here. Git may still refuse the update after + pre-receive (another hook, a lost ref lock), so the approval is only spent + once Git has locked the ref, in the reference-transaction hook. + """ + repo, operation, approval = evaluate_ref_update( + conn, repository, git_dir, pusher, oldrev, newrev, refname + ) + row = conn.execute( + """ + INSERT INTO ref_updates ( + repository_id, refname, oldrev, newrev, pusher_agent_id, operation + ) + VALUES (?, ?, ?, ?, ?, ?) + RETURNING id + """, + ( + repo["id"], + refname, + oldrev, + newrev, + get_agent(conn, pusher)["id"], + json_dumps(operation) if operation else None, + ), + ).fetchone() + assert row is not None audit.record( conn, - "gate.update_allowed", - "repository", - repo["id"], + "gate.update_validated", + "ref_update", + row[0], agent=pusher, - detail={"refname": refname, "oldrev": oldrev, "newrev": newrev}, + detail={ + "refname": refname, + "oldrev": oldrev, + "newrev": newrev, + "operation": operation, + "approval_id": approval["id"] if approval else None, + }, ) @@ -553,6 +612,9 @@ def run_pre_receive( check_ref_update(conn, repository, git_dir, pusher, *parts) if not saw_update: raise PushRejected("No ref updates supplied on stdin.") + # Without the transaction hook nothing would re-validate at update + # time or consume approvals, so a missing one must reject the push. + _require_reference_transaction_hook(conn, repository) except Exception as exc: # fail closed on anything conn.rollback() print(f"[quorumgit] REJECTED: {exc}", file=sys.stderr) @@ -562,10 +624,248 @@ def run_pre_receive( return 0 -def _effective_pre_receive_hook(repository_path: str | Path) -> Path: +# ------------------------------------------------------ reference transaction + +TRANSACTION_STATES = ("prepared", "committed", "aborted") +# pre-receive and the reference transaction run seconds apart. A validation +# older than this is stale (Git refused the update after pre-receive) and is +# never matched against a later transaction. +VALIDATION_TTL_SECONDS = 300 + + +def _parse_updates(stdin_lines: Iterable[str]) -> list[tuple[str, str, str]]: + updates = [] + for line in stdin_lines: + parts = line.split() + if not parts: + continue + if len(parts) != 3: + raise PushRejected(f"Malformed reference-transaction input: {line!r}") + updates.append((parts[0], parts[1], parts[2])) + return updates + + +def _find_update( + conn: Connection, + repository_id: int, + status: str, + oldrev: str, + newrev: str, + refname: str, +) -> tuple | None: + """The newest recorded update matching one transaction line. + + Git reports the zero OID as the old value when a transaction does not + check it, so a zero old value matches any recorded one. + """ + return conn.execute( + """ + SELECT u.id, u.oldrev, a.name, u.approval_id + FROM ref_updates u JOIN agents a ON a.id = u.pusher_agent_id + WHERE u.repository_id = ? AND u.status = ? AND u.refname = ? + AND u.newrev = ? AND (u.oldrev = ? OR ?) + AND u.created_at >= unixepoch() - ? + ORDER BY u.id DESC + LIMIT 1 + """, + ( + repository_id, + status, + refname, + newrev, + oldrev, + 1 if _is_zero(oldrev) else 0, + VALIDATION_TTL_SECONDS, + ), + ).fetchone() + + +def _prepare_updates( + conn: Connection, + repository: str, + git_dir: str, + pusher: str | None, + updates: list[tuple[str, str, str]], +) -> None: + """Re-validate and authorize validated updates while Git holds the locks.""" + begin_immediate(conn) + repo = get_repository(conn, repository) + for oldrev, newrev, refname in updates: + found = _find_update(conn, repo["id"], "validated", oldrev, newrev, refname) + if found is None: + stray = conn.execute( + "SELECT id FROM ref_updates WHERE repository_id = ? " + "AND refname = ? AND status = 'validated' " + "AND created_at >= unixepoch() - ? LIMIT 1", + (repo["id"], refname, VALIDATION_TTL_SECONDS), + ).fetchone() + if stray is not None: + raise PushRejected( + f"Update of {refname} to {newrev} does not match the update " + "pre-receive validated." + ) + # Not a governed push (pre-receive records every update it + # accepts): local ref maintenance in the hub passes through. + continue + update_id, validated_old, validated_pusher, _ = found + if pusher != validated_pusher: + raise PushRejected( + f"Update of {refname} was validated for {validated_pusher}, " + f"not {pusher or 'an unidentified pusher'}." + ) + _, operation, approval = evaluate_ref_update( + conn, repository, git_dir, pusher, validated_old, newrev, refname + ) + if operation is not None: + assert approval is not None and pusher is not None + consume_approval(conn, approval["id"], operation, agent=pusher) + cur = conn.execute( + "UPDATE ref_updates SET status = 'prepared', operation = ?, " + "approval_id = ? WHERE id = ? AND status = 'validated'", + ( + json_dumps(operation) if operation else None, + approval["id"] if approval else None, + update_id, + ), + ) + if cur.rowcount != 1: + raise PushRejected(f"Update of {refname} changed while being prepared.") + audit.record( + conn, + "gate.update_prepared", + "ref_update", + update_id, + agent=pusher, + detail={ + "refname": refname, + "oldrev": validated_old, + "newrev": newrev, + "approval_id": approval["id"] if approval else None, + }, + ) + + +def _restore_approval(conn: Connection, update_id: int, approval_id: int) -> None: + """Return an approval consumed for an update Git then aborted.""" + try: + cur = conn.execute( + "UPDATE approvals SET status = 'approved', consumed_at = NULL, " + "consumed_by_agent_id = NULL WHERE id = ? AND status = 'consumed'", + (approval_id,), + ) + except ValueError as exc: # libSQL constraint failure + # A newer live instance for the same operation now exists; the old + # one stays consumed rather than creating two live approvals. + audit.record( + conn, + "approval.restore_skipped", + "approval", + approval_id, + detail={"ref_update_id": update_id, "reason": str(exc)}, + ) + return + if cur.rowcount == 1: + audit.record( + conn, + "approval.restored", + "approval", + approval_id, + detail={"ref_update_id": update_id}, + ) + + +def _resolve_updates( + conn: Connection, + repository: str, + state: str, + updates: list[tuple[str, str, str]], +) -> None: + begin_immediate(conn) + repo = get_repository(conn, repository) + for oldrev, newrev, refname in updates: + statuses = ("prepared",) if state == "committed" else ("prepared", "validated") + for status in statuses: + found = _find_update(conn, repo["id"], status, oldrev, newrev, refname) + if found is None: + continue + update_id, _, pusher, approval_id = found + conn.execute( + "UPDATE ref_updates SET status = ?, resolved_at = unixepoch() " + "WHERE id = ? AND status = ?", + (state, update_id, status), + ) + if state == "aborted" and approval_id is not None: + _restore_approval(conn, update_id, approval_id) + audit.record( + conn, + f"gate.update_{state}", + "ref_update", + update_id, + agent=pusher, + detail={"refname": refname, "newrev": newrev}, + ) + break + + +def run_reference_transaction( + conn: Connection, repository: str, state: str, stdin_lines: Iterable[str] +) -> int: + """Hook entry point for Git's reference-transaction hook. + + `prepared` is the authoritative gate: its failure makes Git abort the + transaction, so it fails closed. Git ignores the exit status of + `committed` and `aborted`; they only record the outcome. + """ + if state not in TRANSACTION_STATES: + return 0 + git_dir = os.environ.get("GIT_DIR") or "." + pusher = os.environ.get("QUORUMGIT_AGENT") or None + try: + updates = _parse_updates(stdin_lines) + if state == "prepared": + _prepare_updates(conn, repository, git_dir, pusher, updates) + else: + _resolve_updates(conn, repository, state, updates) + except Exception as exc: # noqa: BLE001 — fail closed on anything + conn.rollback() + label = "REJECTED" if state == "prepared" else f"WARNING ({state})" + print(f"[quorumgit] {label}: {exc}", file=sys.stderr) + return 1 + conn.commit() + return 0 + + +# ------------------------------------------------------------ hook install + +HOOK_NAMES = ("pre-receive", "reference-transaction") +REFERENCE_TRANSACTION_MARKER = "# quorumgit-managed-reference-transaction v1" +# The reference-transaction hook (with its prepared/committed/aborted states). +MIN_GIT_VERSION = (2, 28) + + +def _git_version() -> tuple[int, ...]: + result = subprocess.run( + ["git", "version"], capture_output=True, text=True, check=False + ) + match = re.search(r"(\d+)\.(\d+)", result.stdout) + if result.returncode != 0 or match is None: + raise GateError("Unable to determine the Git version.") + return (int(match.group(1)), int(match.group(2))) + + +def _require_git_version() -> None: + version = _git_version() + if version < MIN_GIT_VERSION: + raise GateError( + f"Git {'.'.join(map(str, version))} lacks the reference-transaction " + f"hook; QuorumGit needs Git {'.'.join(map(str, MIN_GIT_VERSION))} or newer." + ) + + +def _effective_hook(repository_path: str | Path, name: str) -> Path: repo_path = Path(repository_path).resolve() result = subprocess.run( - ["git", "-C", str(repo_path), "rev-parse", "--git-path", "hooks/pre-receive"], + ["git", "-C", str(repo_path), "rev-parse", "--git-path", f"hooks/{name}"], capture_output=True, text=True, ) @@ -575,13 +875,17 @@ def _effective_pre_receive_hook(repository_path: str | Path) -> Path: raise GateError(f"Unable to resolve Git hook path for {repo_path}{suffix}") raw = result.stdout.strip() if not raw: - raise GateError(f"Git returned no pre-receive hook path for {repo_path}") + raise GateError(f"Git returned no {name} hook path for {repo_path}") hook_path = Path(raw) if not hook_path.is_absolute(): hook_path = repo_path / hook_path return hook_path.resolve() +def _effective_pre_receive_hook(repository_path: str | Path) -> Path: + return _effective_hook(repository_path, "pre-receive") + + def _hook_script(repository: str, executable: str | None = None) -> str: python = executable or sys.executable return ( @@ -592,6 +896,22 @@ def _hook_script(repository: str, executable: str | None = None) -> str: ) +def _reference_transaction_script( + repository: str, executable: str | None = None +) -> str: + python = executable or sys.executable + return ( + "#!/bin/sh\n" + f"{REFERENCE_TRANSACTION_MARKER}\n" + 'case "$1" in\n' + "prepared|committed|aborted) ;;\n" + "*) cat >/dev/null; exit 0 ;;\n" + "esac\n" + f"exec {shlex.quote(python)} -m quorumgit hook reference-transaction " + f'--repo {shlex.quote(repository)} "$1"\n' + ) + + def _legacy_hook_script(repository: str) -> str: """Exact pre-PR6 hook shape, recognized only for safe in-place upgrade.""" return ( @@ -601,6 +921,29 @@ def _legacy_hook_script(repository: str) -> str: ) +def _hook_plan(repository: str, name: str) -> tuple[str, str, tuple[str, ...]]: + """(expected content, ownership marker, upgradeable legacy contents).""" + if name == "pre-receive": + return _hook_script(repository), HOOK_MARKER, (_legacy_hook_script(repository),) + return _reference_transaction_script(repository), REFERENCE_TRANSACTION_MARKER, () + + +def _require_reference_transaction_hook(conn: Connection, repository: str) -> None: + _require_git_version() + repo = get_repository(conn, repository) + hook_path = _effective_hook(repo["path"], "reference-transaction") + expected, _, _ = _hook_plan(repository, "reference-transaction") + try: + installed = hook_path.read_text(encoding="utf-8") + except (OSError, UnicodeError): + installed = None + if installed != expected: + raise PushRejected( + f"The QuorumGit reference-transaction hook is missing or modified at " + f"{hook_path}; run `quorumgit hook install --repo {repository}`." + ) + + def _write_hook_atomically(hook_path: Path, content: str) -> None: hook_path.parent.mkdir(parents=True, exist_ok=True) fd, tmp_name = tempfile.mkstemp( @@ -623,63 +966,70 @@ def _write_hook_atomically(hook_path: Path, content: str) -> None: pass +def _inspect_hook(hook_path: Path, expected: str, marker: str, legacy: tuple[str, ...]) -> str: + """Decide what installing over `hook_path` means; refuse foreign content.""" + if not hook_path.exists(): + return "installed" + try: + existing = hook_path.read_text(encoding="utf-8") + except (OSError, UnicodeError) as exc: + raise GateError(f"Cannot inspect existing hook {hook_path}: {exc}") from exc + if existing == expected: + return "verified" + if existing in legacy: + return "upgraded" + if marker in existing.splitlines()[:3]: + raise GateError( + f"Existing QuorumGit-managed hook differs at {hook_path}; " + "refusing to overwrite it." + ) + raise GateError( + f"Existing hook at {hook_path} is not owned by QuorumGit; refusing to " + "overwrite or silently chain it." + ) + + def install_hook(conn: Connection, repository: str) -> Path: - """Safely install QuorumGit at Git's effective pre-receive hook path. + """Safely install QuorumGit's pre-receive and reference-transaction hooks. - Existing unrelated hooks are never overwritten or silently chained. An + Both go at Git's effective hook paths. Existing unrelated hooks are never + overwritten or silently chained, and every existing hook is inspected + before anything is written, so a refusal leaves both paths untouched. An exact current QuorumGit hook is idempotent; an exact legacy QuorumGit hook - for the same repository is upgraded in place. Any other existing content - is refused so operators must make coexistence explicit. + for the same repository is upgraded in place. Returns the pre-receive path. """ begin_immediate(conn) + _require_git_version() repo = get_repository(conn, repository) common_dir = assert_repository_identity_unique(conn, repo) - hook_path = _effective_pre_receive_hook(repo["path"]) - expected = _hook_script(repository) - action = "installed" - if hook_path.exists(): - try: - existing = hook_path.read_text(encoding="utf-8") - except (OSError, UnicodeError) as exc: - raise GateError( - f"Cannot inspect existing pre-receive hook {hook_path}: {exc}" - ) from exc - if existing == expected: + plans = [] + for name in HOOK_NAMES: + expected, marker, legacy = _hook_plan(repository, name) + hook_path = _effective_hook(repo["path"], name) + plans.append( + (name, hook_path, expected, _inspect_hook(hook_path, expected, marker, legacy)) + ) + + for name, hook_path, expected, action in plans: + if action == "verified": hook_path.chmod(0o755) - action = "verified" - elif existing == _legacy_hook_script(repository): - _write_hook_atomically(hook_path, expected) - action = "upgraded" - elif HOOK_MARKER in existing.splitlines()[:3]: - raise GateError( - f"Existing QuorumGit-managed pre-receive hook differs at " - f"{hook_path}; refusing to overwrite it." - ) else: - raise GateError( - f"Existing pre-receive hook at {hook_path} is not owned by " - "QuorumGit; refusing to overwrite or silently chain it." - ) - else: - _write_hook_atomically(hook_path, expected) - - effective = _effective_pre_receive_hook(repo["path"]) - if effective != hook_path or not hook_path.exists(): - raise GateError( - f"Installed hook is not Git's effective pre-receive hook: {hook_path}" + _write_hook_atomically(hook_path, expected) + effective = _effective_hook(repo["path"], name) + if effective != hook_path or not hook_path.exists(): + raise GateError(f"Installed hook is not Git's effective {name} hook: {hook_path}") + if hook_path.read_text(encoding="utf-8") != expected: + raise GateError(f"Installed {name} hook failed verification: {hook_path}") + audit.record( + conn, + f"hook.{action}", + "repository", + repo["id"], + detail={ + "hook": name, + "path": str(hook_path), + "git_common_dir": str(common_dir), + }, ) - if hook_path.read_text(encoding="utf-8") != expected: - raise GateError(f"Installed pre-receive hook failed verification: {hook_path}") - - audit.record( - conn, - f"hook.{action}", - "repository", - repo["id"], - detail={ - "path": str(hook_path), - "git_common_dir": str(common_dir), - }, - ) - return hook_path + return plans[0][1] diff --git a/src/quorumgit/migrations/003_ref_updates.sql b/src/quorumgit/migrations/003_ref_updates.sql new file mode 100644 index 0000000..dfc0c93 --- /dev/null +++ b/src/quorumgit/migrations/003_ref_updates.sql @@ -0,0 +1,37 @@ +-- Bind a push's pre-receive validation to Git's own reference transaction. +-- pre-receive records each exact ref update it validated. The +-- reference-transaction hook re-validates it and consumes any approval while +-- Git holds the ref locks ("prepared"), then records whether Git committed or +-- aborted the update. An approval is therefore never spent on an update that +-- did not happen, and ownership is re-checked at the last abortable moment. + +CREATE TABLE ref_updates ( + id INTEGER PRIMARY KEY, + repository_id INTEGER NOT NULL REFERENCES repositories(id), + refname TEXT NOT NULL CHECK (refname <> ''), + oldrev TEXT NOT NULL CHECK ( + length(oldrev) BETWEEN 40 AND 64 + AND oldrev NOT GLOB '*[^0-9a-f]*' + ), + newrev TEXT NOT NULL CHECK ( + length(newrev) BETWEEN 40 AND 64 + AND newrev NOT GLOB '*[^0-9a-f]*' + ), + pusher_agent_id INTEGER NOT NULL REFERENCES agents(id), + operation TEXT CHECK ( + operation IS NULL + OR (json_valid(operation) AND json_type(operation) = 'object') + ), + approval_id INTEGER REFERENCES approvals(id), + status TEXT NOT NULL DEFAULT 'validated' + CHECK (status IN ('validated', 'prepared', 'committed', 'aborted')), + created_at INTEGER NOT NULL DEFAULT (unixepoch()), + resolved_at INTEGER, + CHECK (status IN ('validated', 'aborted') OR operation IS NULL OR approval_id IS NOT NULL), + CHECK (status IN ('validated', 'prepared') OR resolved_at IS NOT NULL) +); + +-- quorumgit-statement +CREATE INDEX ref_updates_in_flight + ON ref_updates(repository_id, refname, newrev) + WHERE status IN ('validated', 'prepared'); diff --git a/src/quorumgit/registry.py b/src/quorumgit/registry.py index ba97bbd..22e9137 100644 --- a/src/quorumgit/registry.py +++ b/src/quorumgit/registry.py @@ -2,6 +2,7 @@ from __future__ import annotations +import os import subprocess from pathlib import Path @@ -23,6 +24,28 @@ def _protected_refs(conn: Connection, repository_id: int) -> list[str]: ] +# Variables Git exports to hooks that select a repository. A lookup of a +# specific path must not inherit them, or every path resolves to the hook's +# repository. +_REPOSITORY_SELECTING_ENV = ( + "GIT_DIR", + "GIT_WORK_TREE", + "GIT_COMMON_DIR", + "GIT_INDEX_FILE", + "GIT_OBJECT_DIRECTORY", + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + "GIT_QUARANTINE_PATH", +) + + +def path_scoped_git_env() -> dict[str, str]: + return { + key: value + for key, value in os.environ.items() + if key not in _REPOSITORY_SELECTING_ENV + } + + def git_common_dir(path: str | Path) -> Path: """Return the canonical Git common directory for a repository path. @@ -35,6 +58,7 @@ def git_common_dir(path: str | Path) -> Path: ["git", "-C", str(repo_path), "rev-parse", "--git-common-dir"], capture_output=True, text=True, + env=path_scoped_git_env(), ) if result.returncode != 0: detail = (result.stderr or result.stdout).strip() diff --git a/src/quorumgit/store.py b/src/quorumgit/store.py index bca20f9..83313af 100644 --- a/src/quorumgit/store.py +++ b/src/quorumgit/store.py @@ -7,7 +7,9 @@ from __future__ import annotations +import functools import json +import re import time from importlib import import_module, resources from pathlib import Path @@ -52,6 +54,14 @@ "audit_events", ) +# Kept byte-identical to the statement existing stores were created with. +SCHEMA_MIGRATIONS_DDL = """ + CREATE TABLE IF NOT EXISTS schema_migrations ( + version TEXT PRIMARY KEY, + applied_at INTEGER NOT NULL DEFAULT (unixepoch()) + ) + """ + class StoreError(RuntimeError): pass @@ -248,14 +258,7 @@ def migrate(target: Config | str | Path) -> list[str]: applied: list[str] = [] try: begin_immediate(conn) - conn.execute( - """ - CREATE TABLE IF NOT EXISTS schema_migrations ( - version TEXT PRIMARY KEY, - applied_at INTEGER NOT NULL DEFAULT (unixepoch()) - ) - """ - ) + conn.execute(SCHEMA_MIGRATIONS_DDL) conn.commit() done = { @@ -294,6 +297,74 @@ def migrate(target: Config | str | Path) -> list[str]: # ------------------------------------------------------------ contract check +def _normalized_sql(sql: str | None) -> str | None: + """Schema SQL with formatting-only whitespace removed.""" + if sql is None: + return None + return re.sub(r" ?([(),;]) ?", r"\1", " ".join(sql.split())) + + +def _schema_objects(conn: Connection) -> dict[tuple[str, str], tuple[str, str | None]]: + """Every user schema object as {(type, name): (table, normalized SQL)}.""" + rows = conn.execute( + "SELECT type, name, tbl_name, sql FROM sqlite_master " + "WHERE name NOT LIKE 'sqlite_%'" + ).fetchall() + return {(row[0], row[1]): (row[2], _normalized_sql(row[3])) for row in rows} + + +@functools.lru_cache(maxsize=1) +def _reference_schema() -> dict[tuple[str, str], tuple[str, str | None]]: + """The schema this QuorumGit build expects: every migration replayed in memory. + + Deriving the fingerprint from the migrations themselves keeps it exact + without a hand-maintained list: each table, index (including the partial + unique indexes that enforce single ownership and single live approvals) + and trigger (including the append-only audit guards) must be present with + the same definition. + """ + conn = libsql.connect(":memory:") + try: + conn.execute(SCHEMA_MIGRATIONS_DDL) + for mig in _migration_files(): + for statement in _migration_statements(mig.read_text(encoding="utf-8")): + conn.execute(statement) + return _schema_objects(conn) + finally: + conn.close() + + +def _describe(keys: set[tuple[str, str]]) -> str: + return ", ".join(f"{kind} {name}" for kind, name in sorted(keys)) + + +def _verify_schema_objects(conn: Connection) -> None: + expected = _reference_schema() + actual = _schema_objects(conn) + missing = set(expected) - set(actual) + changed = { + key for key in set(expected) & set(actual) if expected[key] != actual[key] + } + # Extra tables are inert data; an extra index or trigger can change what + # governance writes succeed or what they do, so it is a violation. + unexpected = { + key for key in set(actual) - set(expected) if key[0] != "table" + } + problems = [] + if missing: + problems.append(f"missing {_describe(missing)}") + if changed: + problems.append(f"altered {_describe(changed)}") + if unexpected: + problems.append(f"unexpected {_describe(unexpected)}") + if problems: + raise ContractViolation( + "Store schema does not match this QuorumGit version: " + + "; ".join(problems) + + ". Governance invariants cannot be guaranteed." + ) + + def verify_contract(target: Config | Connection | str | Path) -> None: """Fail loudly unless the local store satisfies the runtime contract.""" owned = isinstance(target, (Config, str, Path)) @@ -332,6 +403,13 @@ def verify_contract(target: Config | Connection | str | Path) -> None: f"Missing required migrations: {sorted(missing_migrations)}. " "Run `quorumgit init` to apply migrations." ) + unknown_migrations = applied_migrations - required_migrations + if unknown_migrations: + raise ContractViolation( + f"Store has migrations this QuorumGit does not know: " + f"{sorted(unknown_migrations)}. It was written by a newer version." + ) + _verify_schema_objects(conn) foreign_keys = conn.execute("PRAGMA foreign_keys").fetchone()[0] journal_mode = str(conn.execute("PRAGMA journal_mode").fetchone()[0]).lower() diff --git a/tests/test_authorization_binding.py b/tests/test_authorization_binding.py index 4212f72..97b6153 100644 --- a/tests/test_authorization_binding.py +++ b/tests/test_authorization_binding.py @@ -185,13 +185,19 @@ def test_contract_rejects_001_only_store_until_all_migrations_apply(tmp_path): conn.commit() conn.close() - message = "Missing required migrations: ['002_approval_identities.sql']" + message = ( + "Missing required migrations: " + "['002_approval_identities.sql', '003_ref_updates.sql']" + ) with pytest.raises(store.ContractViolation, match=re.escape(message)): store.verify_contract(cfg) with pytest.raises(store.ContractViolation, match=re.escape(message)): store.connect(cfg) - assert store.migrate(cfg) == ["002_approval_identities.sql"] + assert store.migrate(cfg) == [ + "002_approval_identities.sql", + "003_ref_updates.sql", + ] store.verify_contract(cfg) upgraded = store.connect(cfg) upgraded.close() @@ -208,7 +214,10 @@ def test_identity_migration_preserves_and_backfills_history(tmp_path): conn.commit() conn.close() - assert store.migrate(cfg) == ["002_approval_identities.sql"] + assert store.migrate(cfg) == [ + "002_approval_identities.sql", + "003_ref_updates.sql", + ] migrated = store.connect(cfg) try: requester = migrated.execute( @@ -248,7 +257,10 @@ def test_migration_does_not_backfill_requester_registered_after_request(tmp_path conn.commit() conn.close() - assert store.migrate(cfg) == ["002_approval_identities.sql"] + assert store.migrate(cfg) == [ + "002_approval_identities.sql", + "003_ref_updates.sql", + ] migrated = store.connect(cfg) try: row = migrated.execute( @@ -280,7 +292,10 @@ def test_migration_does_not_backfill_voter_registered_after_vote(tmp_path): conn.commit() conn.close() - assert store.migrate(cfg) == ["002_approval_identities.sql"] + assert store.migrate(cfg) == [ + "002_approval_identities.sql", + "003_ref_updates.sql", + ] migrated = store.connect(cfg) try: approval = migrated.execute( diff --git a/tests/test_transaction_boundary.py b/tests/test_transaction_boundary.py new file mode 100644 index 0000000..822d8f9 --- /dev/null +++ b/tests/test_transaction_boundary.py @@ -0,0 +1,343 @@ +"""The push authorization boundary and the store's schema contract. + +pre-receive validates and records each ref update but spends nothing. The +reference-transaction hook re-validates it and consumes any approval at the +`prepared` stage, while Git holds the ref locks and can still abort, then +records whether Git committed or aborted. verify_contract() checks every +schema object the governance rules depend on, not only table names. +""" + +from __future__ import annotations + +import os +import shlex +import subprocess +import sys +from pathlib import Path + +import pytest + +from quorumgit import gate, registry, store, work +from quorumgit.config import Config +from tests.test_gate import _commit, _push, _setup + + +def _rev(git_args: list[str], ref: str) -> str: + return subprocess.run( + ["git", *git_args, "rev-parse", ref], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + +def _approve_main(conn, repo_name: str, hub: Path, clone: Path) -> tuple[dict, dict]: + op = { + "type": "protected_ref_update", + "repository": repo_name, + "refname": "refs/heads/main", + "oldrev": _rev(["--git-dir", str(hub)], "refs/heads/main"), + "newrev": _rev(["-C", str(clone)], "HEAD"), + } + conn.execute("INSERT INTO agents (name) VALUES ('operator') ON CONFLICT DO NOTHING") + approval = gate.request_approval(conn, op, requested_by="operator") + gate.vote(conn, approval["id"], "operator", True) + conn.commit() + return op, approval + + +def _install_update_hook(hub: Path, python_body: str) -> Path: + """An extra `update` hook: runs after pre-receive, before the ref moves.""" + script = hub / "hooks" / "extra_update.py" + script.write_text(python_body, encoding="utf-8") + hook = hub / "hooks" / "update" + hook.write_text( + "#!/bin/sh\n" + f"exec {shlex.quote(sys.executable)} {shlex.quote(str(script))} \"$@\"\n", + encoding="utf-8", + ) + hook.chmod(0o755) + return hook + + +def _ref_update_statuses(conn, refname: str, newrev: str) -> list[str]: + conn.commit() + return [ + row[0] + for row in conn.execute( + "SELECT status FROM ref_updates WHERE refname = ? AND newrev = ? " + "ORDER BY id", + (refname, newrev), + ).fetchall() + ] + + +# ----------------------------------------------------- transaction boundary + + +def test_approval_survives_an_update_git_refuses_after_pre_receive( + committed_conn, tmp_path, cfg +): + conn = committed_conn + repo_name, hub, clone, a, _b = _setup(conn, tmp_path) + _commit(clone, "src/approved.py") + op, approval = _approve_main(conn, repo_name, hub, clone) + + refusing = _install_update_hook(hub, "import sys\nsys.exit(1)\n") + refused = _push(clone, a, "main", cfg=cfg) + assert refused.returncode != 0 + assert _rev(["--git-dir", str(hub)], "refs/heads/main") == op["oldrev"] + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "approved" + assert _ref_update_statuses(conn, "refs/heads/main", op["newrev"]) == ["validated"] + + refusing.unlink() + accepted = _push(clone, a, "main", cfg=cfg) + assert accepted.returncode == 0, accepted.stderr + assert _rev(["--git-dir", str(hub)], "refs/heads/main") == op["newrev"] + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "consumed" + assert _ref_update_statuses(conn, "refs/heads/main", op["newrev"]) == [ + "validated", + "committed", + ] + + +def test_ownership_change_after_pre_receive_aborts_the_update( + committed_conn, tmp_path, cfg +): + conn = committed_conn + repo_name, hub, clone, a, b = _setup(conn, tmp_path) + task = work.create_task(conn, repo_name, "contested") + claim, _, _ = work.claim_task(conn, task, a, branch="feat/race", scope_globs=["src/**"]) + conn.commit() + _commit(clone, "src/first.py", branch="feat/race") + first = _push(clone, a, "feat/race", cfg=cfg) + assert first.returncode == 0, first.stderr + before = _rev(["--git-dir", str(hub)], "refs/heads/feat/race") + + # Between pre-receive and the ref update, a's claim is released and b + # claims the branch. pre-receive saw a as owner; the update must not land. + _install_update_hook( + hub, + "from quorumgit import config, store, work\n" + "cfg = config.load()\n" + "conn = store.connect(cfg)\n" + f"work.release_claim(conn, {claim}, agent={a!r})\n" + f"work.claim_task(conn, {task}, {b!r}, branch='feat/race', scope_globs=['src/**'])\n" + "conn.commit()\n" + "conn.close()\n", + ) + _commit(clone, "src/second.py", branch="feat/race") + raced = _push(clone, a, "feat/race", cfg=cfg) + assert raced.returncode != 0 + assert _rev(["--git-dir", str(hub)], "refs/heads/feat/race") == before + newrev = _rev(["-C", str(clone)], "HEAD") + assert _ref_update_statuses(conn, "refs/heads/feat/race", newrev) == ["aborted"] + rejected = conn.execute( + "SELECT count(*) FROM audit_events WHERE event_type = 'gate.update_prepared' " + "AND json_extract(detail, '$.newrev') = ?", + (newrev,), + ).fetchone() + assert rejected is not None and rejected[0] == 0 + + +def test_aborted_transaction_restores_the_consumed_approval( + committed_conn, tmp_path, cfg, monkeypatch +): + conn = committed_conn + repo_name, hub, clone, a, _b = _setup(conn, tmp_path) + _commit(clone, "src/restore.py") + # Make the new commit's objects available in the hub without moving main. + staged = _push(clone, a, "HEAD:refs/heads/staging", cfg=cfg) + assert staged.returncode == 0, staged.stderr + op, approval = _approve_main(conn, repo_name, hub, clone) + + # receive-pack runs hooks from inside the repository with GIT_DIR=".". + monkeypatch.chdir(hub) + monkeypatch.setenv("GIT_DIR", ".") + monkeypatch.setenv("QUORUMGIT_AGENT", a) + line = [f"{op['oldrev']} {op['newrev']} refs/heads/main\n"] + assert gate.run_pre_receive(conn, repo_name, line) == 0 + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "approved" + + assert gate.run_reference_transaction(conn, repo_name, "prepared", line) == 0 + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "consumed" + + assert gate.run_reference_transaction(conn, repo_name, "aborted", line) == 0 + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "approved" + assert _ref_update_statuses(conn, "refs/heads/main", op["newrev"]) == ["aborted"] + restored = conn.execute( + "SELECT count(*) FROM audit_events WHERE event_type = 'approval.restored' " + "AND entity_id = ?", + (approval["id"],), + ).fetchone() + assert restored is not None and restored[0] == 1 + + +def test_prepared_rejects_an_update_pre_receive_did_not_validate( + committed_conn, tmp_path, monkeypatch +): + conn = committed_conn + repo_name, hub, clone, a, _b = _setup(conn, tmp_path) + _commit(clone, "docs/one.md", branch="feat/swap") + _commit(clone, "docs/two.md", branch="feat/swap") + validated = _rev(["-C", str(clone)], "HEAD") + swapped = _rev(["-C", str(clone)], "HEAD~1") + + monkeypatch.chdir(hub) + monkeypatch.setenv("GIT_DIR", ".") + monkeypatch.setenv("QUORUMGIT_AGENT", a) + zero = "0" * 40 + conn.execute( + "INSERT INTO ref_updates (repository_id, refname, oldrev, newrev, " + "pusher_agent_id) VALUES (?, 'refs/heads/feat/swap', ?, ?, ?)", + ( + registry.get_repository(conn, repo_name)["id"], + zero, + validated, + registry.get_agent(conn, a)["id"], + ), + ) + conn.commit() + line = [f"{zero} {swapped} refs/heads/feat/swap\n"] + assert gate.run_reference_transaction(conn, repo_name, "prepared", line) == 1 + + +def test_push_requires_the_reference_transaction_hook(committed_conn, tmp_path, cfg): + conn = committed_conn + _repo_name, hub, clone, a, _b = _setup(conn, tmp_path) + (hub / "hooks" / "reference-transaction").unlink() + _commit(clone, "docs/free.md", branch="feat/nohook") + rejected = _push(clone, a, "feat/nohook", cfg=cfg) + assert rejected.returncode != 0 + assert "reference-transaction hook is missing" in rejected.stderr + + +def test_local_ref_maintenance_in_the_hub_passes_through( + committed_conn, tmp_path, cfg +): + _repo_name, hub, _clone, _a, _b = _setup(committed_conn, tmp_path) + head = _rev(["--git-dir", str(hub)], "refs/heads/main") + for data_dir in (cfg.data_dir, tmp_path / "no-store-here"): + env = {**os.environ, "QUORUMGIT_DATA_DIR": str(data_dir)} + env.pop("QUORUMGIT_AGENT", None) + env["PATH"] = str(Path(sys.executable).parent) + os.pathsep + env["PATH"] + result = subprocess.run( + ["git", "--git-dir", str(hub), "update-ref", + f"refs/heads/local-{data_dir.name}", head], + capture_output=True, + text=True, + env=env, + check=False, + ) + assert result.returncode == 0, result.stderr + + +def test_install_refuses_foreign_reference_transaction_hook_before_writing( + conn, tmp_path +): + from tests.test_hook_integrity import _hub_with_clone + + hub, _clone = _hub_with_clone(tmp_path, "foreign-rt") + repo_name = f"foreign-rt-{tmp_path.name[-8:]}" + registry.add_repository(conn, repo_name, hub) + foreign = hub / "hooks" / "reference-transaction" + foreign.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + foreign.chmod(0o755) + + with pytest.raises(gate.GateError, match="not owned by QuorumGit"): + gate.install_hook(conn, repo_name) + assert not (hub / "hooks" / "pre-receive").exists() + assert foreign.read_text(encoding="utf-8") == "#!/bin/sh\nexit 0\n" + + +def test_install_writes_both_hooks(conn, tmp_path): + from tests.test_hook_integrity import _hub_with_clone + + hub, _clone = _hub_with_clone(tmp_path, "both-hooks") + repo_name = f"both-{tmp_path.name[-8:]}" + registry.add_repository(conn, repo_name, hub) + gate.install_hook(conn, repo_name) + rt = (hub / "hooks" / "reference-transaction").read_text(encoding="utf-8") + assert rt == gate._reference_transaction_script(repo_name) + assert gate.REFERENCE_TRANSACTION_MARKER in rt + + +# ------------------------------------------------------------ schema contract + + +@pytest.fixture() +def fresh_store(tmp_path) -> Config: + cfg = Config(data_dir=tmp_path / "contract", agent=None) + store.migrate(cfg) + store.verify_contract(cfg) + return cfg + + +def _tamper(cfg: Config, *statements: str) -> None: + conn = store.open_connection(cfg) + try: + for statement in statements: + conn.execute(statement) + conn.commit() + finally: + conn.close() + + +@pytest.mark.parametrize( + "trigger", + ["audit_events_no_delete", "audit_events_no_update", "votes_require_registered_voter"], +) +def test_contract_rejects_a_missing_governance_trigger(fresh_store, trigger): + _tamper(fresh_store, f"DROP TRIGGER {trigger}") + with pytest.raises(store.ContractViolation, match=f"missing trigger {trigger}"): + store.verify_contract(fresh_store) + with pytest.raises(store.ContractViolation): + store.connect(fresh_store) + + +def test_contract_rejects_a_weakened_unique_index(fresh_store): + _tamper( + fresh_store, + "DROP INDEX claims_one_active_per_task", + "CREATE INDEX claims_one_active_per_task ON claims(task_id) " + "WHERE released_at IS NULL", + ) + with pytest.raises( + store.ContractViolation, match="altered index claims_one_active_per_task" + ): + store.verify_contract(fresh_store) + + +def test_contract_rejects_an_unexpected_trigger(fresh_store): + _tamper( + fresh_store, + "CREATE TRIGGER auto_approve AFTER INSERT ON approvals BEGIN " + "UPDATE approvals SET status = 'approved' WHERE id = NEW.id; END", + ) + with pytest.raises(store.ContractViolation, match="unexpected trigger auto_approve"): + store.verify_contract(fresh_store) + + +def test_contract_rejects_a_store_from_a_newer_version(fresh_store): + _tamper(fresh_store, "INSERT INTO schema_migrations (version) VALUES ('999_future.sql')") + with pytest.raises(store.ContractViolation, match="newer version"): + store.verify_contract(fresh_store) + + +def test_contract_tolerates_formatting_only_schema_differences(fresh_store): + _tamper( + fresh_store, + "DROP INDEX ref_updates_in_flight", + "CREATE INDEX ref_updates_in_flight ON ref_updates ( repository_id,refname , " + "newrev ) WHERE status IN ('validated','prepared')", + ) + store.verify_contract(fresh_store) + + +def test_identity_lookup_ignores_the_hooks_git_dir(tmp_path, monkeypatch): + from tests.conftest import make_git_repo + + hub = make_git_repo(tmp_path / "hook-repo") + other = make_git_repo(tmp_path / "other-repo") + monkeypatch.setenv("GIT_DIR", str(hub / ".git")) + assert registry.git_common_dir(other) == (other / ".git").resolve() From cc88d1485162fb31971391be15f04e506a46446f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 08:56:34 +0000 Subject: [PATCH 4/6] Reconcile ref updates stuck in prepared from doctor A governed ref update whose committed/aborted hook never ran (killed process, unreachable store) stayed `prepared` forever, with its approval consumed whether or not Git applied it. `quorumgit doctor` now reports updates prepared for longer than the validation window and reads the outcome from the ref: at the new value (or gone, for a deletion) is committed; still at the old value is aborted, and --repair restores the consumed approval. A ref that has moved elsewhere or is still locked is reported, never guessed. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Q8BpCkArA34hB1aSyVcnZp --- README.md | 4 +- src/quorumgit/cli.py | 39 +++++-- src/quorumgit/gate.py | 115 +++++++++++++++++++ tests/test_transaction_boundary.py | 171 +++++++++++++++++++++++++++++ 4 files changed, 316 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 40e5f94..07e56c3 100644 --- a/README.md +++ b/README.md @@ -196,7 +196,7 @@ Takeovers follow the same pattern: claiming a task someone else holds (`claim [--protected-ref ]…` | Register a repository | | `quorumgit agent add ` | Register an agent identity | @@ -220,6 +220,8 @@ Takeovers follow the same pattern: claiming a task someone else holds (`claim int: def cmd_doctor(args, cfg) -> int: - """Inspect and optionally reconcile recorded managed-worktree drift.""" + """Inspect and optionally reconcile worktree drift and stuck ref updates.""" with store.connect(cfg) as conn: findings = trees.doctor_worktrees(conn, repair=args.repair) + ref_findings = gate.doctor_ref_updates(conn, repair=args.repair) if args.repair: conn.commit() - if not findings: - print("worktrees: ok") - return 0 unresolved = False - for finding in findings: - state = "repaired" if finding.get("repaired") else "detected" + + def state_of(finding: dict) -> str: + nonlocal unresolved if finding.get("error"): - state = f"repair failed: {finding['error']}" - unresolved = True - elif not finding.get("repaired"): unresolved = True + prefix = "repair failed" if args.repair else "unresolvable" + return f"{prefix}: {finding['error']}" + if finding.get("repaired"): + return "repaired" + unresolved = True + return "detected" + + for finding in findings: print( f"worktree {finding['worktree_id']} claim {finding['claim_id']}: " - f"{finding['issue']} — {state} — {finding['path']}" + f"{finding['issue']} — {state_of(finding)} — {finding['path']}" + ) + for finding in ref_findings: + outcome = f" (git {finding['outcome']})" if finding.get("outcome") else "" + print( + f"ref update {finding['ref_update_id']} {finding['repository']} " + f"{finding['refname']}: {finding['issue']}{outcome} — {state_of(finding)}" ) - if args.repair and not unresolved: - print("worktrees: reconciled") + if not findings: + print("worktrees: ok") + if not ref_findings: + print("ref updates: ok") + if not unresolved: + if args.repair and (findings or ref_findings): + print("reconciled.") return 0 if not args.repair: print("re-run with `quorumgit doctor --repair` to reconcile safe cases.") diff --git a/src/quorumgit/gate.py b/src/quorumgit/gate.py index 21784c7..d6d9a42 100644 --- a/src/quorumgit/gate.py +++ b/src/quorumgit/gate.py @@ -22,6 +22,7 @@ assert_repository_identity_unique, get_agent, get_repository, + path_scoped_git_env, ) from .store import Connection, begin_immediate, json_dumps, json_loads from .work import ( @@ -807,6 +808,120 @@ def _resolve_updates( break +# A prepared update normally resolves within milliseconds, when Git commits or +# aborts the locked transaction. One older than this lost its outcome hook +# (killed process, unreachable store) and is reconciled from the ref itself. +PREPARED_STUCK_AFTER_SECONDS = VALIDATION_TTL_SECONDS + + +def _current_ref(repository_path: str, refname: str) -> str | None: + """The ref's object ID in the repository, or None when it does not exist.""" + result = subprocess.run( + ["git", "-C", repository_path, "rev-parse", "--verify", "--quiet", refname], + capture_output=True, + text=True, + env=path_scoped_git_env(), + check=False, + ) + if result.returncode == 0: + return result.stdout.strip().lower() + if result.returncode == 1: + return None + detail = (result.stderr or result.stdout).strip() + raise GateError(f"Unable to read {refname} in {repository_path}: {detail}") + + +def _ref_is_locked(repository_path: str, refname: str) -> bool: + """True while Git holds the files-backend lock for the ref.""" + result = subprocess.run( + ["git", "-C", repository_path, "rev-parse", "--git-path", f"{refname}.lock"], + capture_output=True, + text=True, + env=path_scoped_git_env(), + check=False, + ) + if result.returncode != 0: + return False + lock = Path(result.stdout.strip()) + if not lock.is_absolute(): + lock = Path(repository_path) / lock + return lock.exists() + + +def doctor_ref_updates(conn: Connection, repair: bool = False) -> list[dict]: + """Report and conservatively reconcile ref updates stuck in `prepared`. + + The outcome is read from the ref itself: at the new value (or gone, for + a deletion) means Git committed; still at the old value (or absent, for a + creation) means Git aborted, so the consumed approval is restored. A ref + that has since moved elsewhere, or is still locked, is reported for + manual inspection and never guessed at. + """ + if repair: + begin_immediate(conn) + rows = conn.execute( + """ + SELECT u.id, u.refname, u.oldrev, u.newrev, u.approval_id, r.name, r.path + FROM ref_updates u JOIN repositories r ON r.id = u.repository_id + WHERE u.status = 'prepared' AND u.created_at < unixepoch() - ? + ORDER BY u.id + """, + (PREPARED_STUCK_AFTER_SECONDS,), + ).fetchall() + findings: list[dict] = [] + for update_id, refname, oldrev, newrev, approval_id, repo_name, repo_path in rows: + finding: dict[str, Any] = { + "ref_update_id": update_id, + "repository": repo_name, + "refname": refname, + "issue": "stuck_prepared", + "outcome": None, + "repaired": False, + } + findings.append(finding) + try: + if _ref_is_locked(repo_path, refname): + finding["error"] = "ref is still locked by Git; retry once it is released" + continue + current = _current_ref(repo_path, refname) + except GateError as exc: + finding["error"] = str(exc) + continue + new_value = None if _is_zero(newrev) else newrev + old_value = None if _is_zero(oldrev) else oldrev + if current == new_value: + finding["outcome"] = "committed" + elif current == old_value: + finding["outcome"] = "aborted" + else: + finding["error"] = ( + f"{refname} has since moved to {current or 'deletion'}; the outcome " + "cannot be proven from the ref. Inspect it manually." + ) + continue + if not repair: + continue + cur = conn.execute( + "UPDATE ref_updates SET status = ?, resolved_at = unixepoch() " + "WHERE id = ? AND status = 'prepared'", + (finding["outcome"], update_id), + ) + if cur.rowcount != 1: + finding["error"] = "ref update changed while being reconciled" + continue + if finding["outcome"] == "aborted" and approval_id is not None: + _restore_approval(conn, update_id, approval_id) + audit.record( + conn, + f"gate.update_reconciled_{finding['outcome']}", + "ref_update", + update_id, + detail={"refname": refname, "current": current}, + ) + finding["repaired"] = True + return findings + + def run_reference_transaction( conn: Connection, repository: str, state: str, stdin_lines: Iterable[str] ) -> int: diff --git a/tests/test_transaction_boundary.py b/tests/test_transaction_boundary.py index 822d8f9..8fd85bb 100644 --- a/tests/test_transaction_boundary.py +++ b/tests/test_transaction_boundary.py @@ -341,3 +341,174 @@ def test_identity_lookup_ignores_the_hooks_git_dir(tmp_path, monkeypatch): other = make_git_repo(tmp_path / "other-repo") monkeypatch.setenv("GIT_DIR", str(hub / ".git")) assert registry.git_common_dir(other) == (other / ".git").resolve() + + +# ----------------------------------------------------- doctor: stuck updates + + +def _stuck_prepared_update(conn, tmp_path, cfg, monkeypatch): + """A protected update that reached `prepared` but never heard its outcome.""" + repo_name, hub, clone, a, _b = _setup(conn, tmp_path) + _commit(clone, "src/stuck.py") + staged = _push(clone, a, "HEAD:refs/heads/staging", cfg=cfg) + assert staged.returncode == 0, staged.stderr + op, approval = _approve_main(conn, repo_name, hub, clone) + + with monkeypatch.context() as scoped: + scoped.chdir(hub) + scoped.setenv("GIT_DIR", ".") + scoped.setenv("QUORUMGIT_AGENT", a) + line = [f"{op['oldrev']} {op['newrev']} refs/heads/main\n"] + assert gate.run_pre_receive(conn, repo_name, line) == 0 + assert gate.run_reference_transaction(conn, repo_name, "prepared", line) == 0 + update_id = conn.execute( + "SELECT id FROM ref_updates WHERE status = 'prepared' AND newrev = ?", + (op["newrev"],), + ).fetchone()[0] + conn.execute( + "UPDATE ref_updates SET created_at = created_at - ? WHERE id = ?", + (gate.PREPARED_STUCK_AFTER_SECONDS + 60, update_id), + ) + conn.commit() + return hub, op, approval, update_id + + +def _finding(conn, update_id: int, repair: bool = False) -> dict: + findings = [ + f for f in gate.doctor_ref_updates(conn, repair=repair) + if f["ref_update_id"] == update_id + ] + assert len(findings) == 1, findings + return findings[0] + + +def _status(conn, update_id: int) -> str: + return conn.execute( + "SELECT status FROM ref_updates WHERE id = ?", (update_id,) + ).fetchone()[0] + + +def _move_ref(hub: Path, tmp_path: Path, refname: str, value: str) -> None: + # No store at this data dir, so QuorumGit's hook treats this as local + # maintenance and records nothing. + env = {**os.environ, "QUORUMGIT_DATA_DIR": str(tmp_path / "no-store")} + env.pop("QUORUMGIT_AGENT", None) + env["PATH"] = str(Path(sys.executable).parent) + os.pathsep + env["PATH"] + subprocess.run( + ["git", "--git-dir", str(hub), "update-ref", refname, value], + check=True, + capture_output=True, + env=env, + ) + + +def test_doctor_reconciles_a_stuck_update_git_committed( + committed_conn, tmp_path, cfg, monkeypatch +): + conn = committed_conn + hub, op, approval, update_id = _stuck_prepared_update(conn, tmp_path, cfg, monkeypatch) + _move_ref(hub, tmp_path, "refs/heads/main", op["newrev"]) + + detected = _finding(conn, update_id) + assert detected["outcome"] == "committed" and not detected["repaired"] + assert _status(conn, update_id) == "prepared" + + repaired = _finding(conn, update_id, repair=True) + conn.commit() + assert repaired["repaired"] and "error" not in repaired + assert _status(conn, update_id) == "committed" + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "consumed" + + +def test_doctor_reconciles_a_stuck_update_git_aborted( + committed_conn, tmp_path, cfg, monkeypatch +): + conn = committed_conn + _hub, _op, approval, update_id = _stuck_prepared_update( + conn, tmp_path, cfg, monkeypatch + ) + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "consumed" + + repaired = _finding(conn, update_id, repair=True) + conn.commit() + assert repaired["outcome"] == "aborted" and repaired["repaired"] + assert _status(conn, update_id) == "aborted" + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "approved" + events = conn.execute( + "SELECT count(*) FROM audit_events WHERE entity = 'ref_update' " + "AND entity_id = ? AND event_type = 'gate.update_reconciled_aborted'", + (update_id,), + ).fetchone() + assert events is not None and events[0] == 1 + + +def test_doctor_refuses_to_guess_when_the_ref_has_moved_on( + committed_conn, tmp_path, cfg, monkeypatch +): + conn = committed_conn + hub, op, approval, update_id = _stuck_prepared_update( + conn, tmp_path, cfg, monkeypatch + ) + # A later commit that is neither the update's old nor its new value. + pusher = conn.execute( + "SELECT a.name FROM ref_updates u JOIN agents a ON a.id = u.pusher_agent_id " + "WHERE u.id = ?", + (update_id,), + ).fetchone()[0] + clone = tmp_path / "clone" + _commit(clone, "src/later.py") + later = _push(clone, pusher, "HEAD:refs/heads/later", cfg=cfg) + assert later.returncode == 0, later.stderr + elsewhere = _rev(["-C", str(clone)], "HEAD") + assert elsewhere not in (op["oldrev"], op["newrev"]) + _move_ref(hub, tmp_path, "refs/heads/main", elsewhere) + + finding = _finding(conn, update_id, repair=True) + conn.commit() + assert not finding["repaired"] and "moved" in finding["error"] + assert _status(conn, update_id) == "prepared" + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "consumed" + + +def test_doctor_leaves_a_locked_ref_alone(committed_conn, tmp_path, cfg, monkeypatch): + conn = committed_conn + hub, _op, _approval, update_id = _stuck_prepared_update( + conn, tmp_path, cfg, monkeypatch + ) + lock = hub / "refs" / "heads" / "main.lock" + lock.parent.mkdir(parents=True, exist_ok=True) + lock.write_text("", encoding="utf-8") + + finding = _finding(conn, update_id, repair=True) + conn.commit() + assert not finding["repaired"] and "locked" in finding["error"] + assert _status(conn, update_id) == "prepared" + + +def test_doctor_ignores_a_recent_prepared_update( + committed_conn, tmp_path, cfg, monkeypatch +): + conn = committed_conn + _hub, _op, _approval, update_id = _stuck_prepared_update( + conn, tmp_path, cfg, monkeypatch + ) + conn.execute( + "UPDATE ref_updates SET created_at = unixepoch() WHERE id = ?", (update_id,) + ) + conn.commit() + assert all( + f["ref_update_id"] != update_id for f in gate.doctor_ref_updates(conn) + ) + + +def test_doctor_cli_reports_stuck_updates(committed_conn, tmp_path, cfg, monkeypatch): + from tests.test_cli_hub import _cli + + conn = committed_conn + _hub, _op, _approval, update_id = _stuck_prepared_update( + conn, tmp_path, cfg, monkeypatch + ) + result = _cli(cfg, "doctor") + assert result.returncode == 1 + assert f"ref update {update_id} " in result.stdout + assert "stuck_prepared (git aborted) — detected" in result.stdout From 9e2fb240121d1d31a1c8647e7610e915077df111 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 13:11:05 +0000 Subject: [PATCH 5/6] Fail closed for identified ref transactions without fresh validation Addresses review findings on the reference-transaction boundary: - pre-receive also requires the transaction hook to be an executable regular file; Git silently skips a non-executable hook, which would have skipped re-validation and approval consumption. - An identified transaction (every push carries QUORUMGIT_AGENT) whose pre-receive validation expired or is missing is rejected instead of passing through as local maintenance. Only unidentified transactions pass through. Classification happens before taking the write reservation, and symbolic refs such as HEAD, which Git lists beside the ref they point to, are left to that ref's own line. - With the store missing, an identified `prepared` transaction fails closed instead of exiting 0. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Q8BpCkArA34hB1aSyVcnZp --- README.md | 2 +- src/quorumgit/cli.py | 12 +++- src/quorumgit/gate.py | 63 +++++++++++++++------ tests/test_transaction_boundary.py | 91 ++++++++++++++++++++++++++++++ 4 files changed, 149 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 21478a0..a6624ed 100644 --- a/README.md +++ b/README.md @@ -159,7 +159,7 @@ QUORUMGIT_AGENT=agent-two git push origin feat/x # rejected — branch is cla git push origin feat/x # rejected — unidentified ``` -**Authorization is bound to the ref update itself.** `pre-receive` checks the push and records each exact ref update it accepted, but spends nothing. Git's `reference-transaction` hook then runs at the `prepared` stage — after Git has locked the refs and while it can still abort — re-checks every rule against the store's current state, and only then consumes any approval. If ownership changed after `pre-receive` (a release, takeover, or handoff), the update is aborted. If Git refuses the update after `pre-receive` (another hook, a lost ref lock), no approval is spent; if Git aborts after `prepared`, the consumed approval is restored. The `committed`/`aborted` outcome of every governed update is recorded. `pre-receive` refuses pushes when the transaction hook is missing or modified, and ref transactions that no push validated (local maintenance in the hub) pass through. +**Authorization is bound to the ref update itself.** `pre-receive` checks the push and records each exact ref update it accepted, but spends nothing. Git's `reference-transaction` hook then runs at the `prepared` stage — after Git has locked the refs and while it can still abort — re-checks every rule against the store's current state, and only then consumes any approval. If ownership changed after `pre-receive` (a release, takeover, or handoff), the update is aborted. If Git refuses the update after `pre-receive` (another hook, a lost ref lock), no approval is spent; if Git aborts after `prepared`, the consumed approval is restored. The `committed`/`aborted` outcome of every governed update is recorded. `pre-receive` refuses pushes when the transaction hook is missing, modified, or not executable. A transaction that carries an agent identity must match a fresh `pre-receive` validation (and a store that has vanished fails it closed); only unidentified ref transactions — local maintenance in the hub — pass through. **Scopes are enforced at push time.** When a branch has a live claim, every commit the push introduces must only touch paths inside that claim's scopes (`**` spans directories, `*` stays within one; a scope without wildcards covers that file or directory). Merge commits count only the paths the merge itself changed, so merging the base branch in is not attributed to the claim holder. In the local model agents commit directly in their worktree, so no hook runs and scopes remain coordination metadata. diff --git a/src/quorumgit/cli.py b/src/quorumgit/cli.py index bfac363..5cf8a13 100644 --- a/src/quorumgit/cli.py +++ b/src/quorumgit/cli.py @@ -444,8 +444,16 @@ def cmd_hook_pre_receive(args, cfg) -> int: def cmd_hook_reference_transaction(args, cfg) -> int: if not store.database_path(cfg).exists(): - # A governed push cannot reach here without a store: its pre-receive - # needed one. Without one this is local ref maintenance. + # Without a store, only unidentified local ref maintenance may pass. + # Every push is identified (pre-receive requires it), so a store that + # vanished after pre-receive must not wave its update through. + if args.state == "prepared" and cfg.agent: + print( + "[quorumgit] REJECTED: the store is missing; cannot re-validate " + f"this update by {cfg.agent}.", + file=sys.stderr, + ) + return 1 return 0 try: conn = store.connect(cfg) diff --git a/src/quorumgit/gate.py b/src/quorumgit/gate.py index d6d9a42..4f49c77 100644 --- a/src/quorumgit/gate.py +++ b/src/quorumgit/gate.py @@ -688,26 +688,50 @@ def _prepare_updates( pusher: str | None, updates: list[tuple[str, str, str]], ) -> None: - """Re-validate and authorize validated updates while Git holds the locks.""" - begin_immediate(conn) + """Re-validate and authorize validated updates while Git holds the locks. + + Only an unidentified transaction with no matching validation is local ref + maintenance. An identified one (every push carries QUORUMGIT_AGENT, which + pre-receive requires) must match a fresh validation, or it is rejected: + an expired or missing record never downgrades a push to pass-through. + """ repo = get_repository(conn, repository) + governed = [] + # Classify without the write reservation, so pass-through maintenance + # never waits on (or deadlocks with) a command holding the store. for oldrev, newrev, refname in updates: + if not refname.startswith("refs/"): + # A symbolic ref such as HEAD is listed alongside the ref it + # points to; that ref appears as its own line and is governed + # there. Pushes only ever name refs under refs/. + continue found = _find_update(conn, repo["id"], "validated", oldrev, newrev, refname) - if found is None: - stray = conn.execute( - "SELECT id FROM ref_updates WHERE repository_id = ? " - "AND refname = ? AND status = 'validated' " - "AND created_at >= unixepoch() - ? LIMIT 1", - (repo["id"], refname, VALIDATION_TTL_SECONDS), - ).fetchone() - if stray is not None: - raise PushRejected( - f"Update of {refname} to {newrev} does not match the update " - "pre-receive validated." - ) - # Not a governed push (pre-receive records every update it - # accepts): local ref maintenance in the hub passes through. + if found is not None: + governed.append((oldrev, newrev, refname)) continue + if pusher is not None: + raise PushRejected( + f"Update of {refname} to {newrev} by {pusher} has no current " + "pre-receive validation; push it again." + ) + stray = conn.execute( + "SELECT id FROM ref_updates WHERE repository_id = ? " + "AND refname = ? AND status = 'validated' " + "AND created_at >= unixepoch() - ? LIMIT 1", + (repo["id"], refname, VALIDATION_TTL_SECONDS), + ).fetchone() + if stray is not None: + raise PushRejected( + f"Update of {refname} to {newrev} does not match the update " + "pre-receive validated." + ) + if not governed: + return + begin_immediate(conn) + for oldrev, newrev, refname in governed: + found = _find_update(conn, repo["id"], "validated", oldrev, newrev, refname) + if found is None: + raise PushRejected(f"Update of {refname} changed while being prepared.") update_id, validated_old, validated_pusher, _ = found if pusher != validated_pusher: raise PushRejected( @@ -1057,6 +1081,13 @@ def _require_reference_transaction_hook(conn: Connection, repository: str) -> No f"The QuorumGit reference-transaction hook is missing or modified at " f"{hook_path}; run `quorumgit hook install --repo {repository}`." ) + # Git silently ignores a hook that is not an executable regular file. + if not hook_path.is_file() or not os.access(hook_path, os.X_OK): + raise PushRejected( + f"The QuorumGit reference-transaction hook at {hook_path} is not " + f"executable, so Git would skip it; run `quorumgit hook install " + f"--repo {repository}`." + ) def _write_hook_atomically(hook_path: Path, content: str) -> None: diff --git a/tests/test_transaction_boundary.py b/tests/test_transaction_boundary.py index 8fd85bb..0b645e6 100644 --- a/tests/test_transaction_boundary.py +++ b/tests/test_transaction_boundary.py @@ -512,3 +512,94 @@ def test_doctor_cli_reports_stuck_updates(committed_conn, tmp_path, cfg, monkeyp assert result.returncode == 1 assert f"ref update {update_id} " in result.stdout assert "stuck_prepared (git aborted) — detected" in result.stdout + + +# ------------------------------------------- review: fail-closed transactions + + +@pytest.mark.skipif(os.name == "nt", reason="Windows has no executable bit") +def test_push_requires_an_executable_transaction_hook(committed_conn, tmp_path, cfg): + conn = committed_conn + _repo_name, hub, clone, a, _b = _setup(conn, tmp_path) + (hub / "hooks" / "reference-transaction").chmod(0o644) + _commit(clone, "docs/noexec.md", branch="feat/noexec") + rejected = _push(clone, a, "feat/noexec", cfg=cfg) + assert rejected.returncode != 0 + assert "not executable" in rejected.stderr + + +def test_identified_transaction_needs_a_fresh_validation( + committed_conn, tmp_path, monkeypatch +): + conn = committed_conn + repo_name, hub, clone, a, _b = _setup(conn, tmp_path) + _commit(clone, "docs/slow.md", branch="feat/slow") + newrev = _rev(["-C", str(clone)], "HEAD") + zero = "0" * 40 + conn.execute( + "INSERT INTO ref_updates (repository_id, refname, oldrev, newrev, " + "pusher_agent_id, created_at) VALUES (?, 'refs/heads/feat/slow', ?, ?, ?, " + "unixepoch() - ?)", + ( + registry.get_repository(conn, repo_name)["id"], + zero, + newrev, + registry.get_agent(conn, a)["id"], + gate.VALIDATION_TTL_SECONDS + 60, + ), + ) + conn.commit() + monkeypatch.chdir(hub) + monkeypatch.setenv("GIT_DIR", ".") + line = [f"{zero} {newrev} refs/heads/feat/slow\n"] + + # A push delayed past the validation window is rejected, not waved through. + monkeypatch.setenv("QUORUMGIT_AGENT", a) + assert gate.run_reference_transaction(conn, repo_name, "prepared", line) == 1 + other = [f"{zero} {newrev} refs/heads/feat/never-validated\n"] + assert gate.run_reference_transaction(conn, repo_name, "prepared", other) == 1 + + # Only unidentified transactions are local maintenance. + monkeypatch.delenv("QUORUMGIT_AGENT") + assert gate.run_reference_transaction(conn, repo_name, "prepared", other) == 0 + + +def test_identified_local_ref_change_in_the_hub_is_rejected( + committed_conn, tmp_path, cfg +): + _repo_name, hub, _clone, a, _b = _setup(committed_conn, tmp_path) + head = _rev(["--git-dir", str(hub)], "refs/heads/main") + env = {**os.environ, "QUORUMGIT_DATA_DIR": str(cfg.data_dir), "QUORUMGIT_AGENT": a} + env["PATH"] = str(Path(sys.executable).parent) + os.pathsep + env["PATH"] + result = subprocess.run( + ["git", "--git-dir", str(hub), "update-ref", "refs/heads/sneaky", head], + capture_output=True, + text=True, + env=env, + check=False, + ) + assert result.returncode != 0 + assert "no current pre-receive validation" in result.stderr + + +@pytest.mark.parametrize( + ("agent", "state", "code"), + [("someone", "prepared", 1), (None, "prepared", 0), ("someone", "committed", 0)], +) +def test_missing_store_fails_closed_only_for_identified_prepare( + tmp_path, agent, state, code +): + env = {**os.environ, "QUORUMGIT_DATA_DIR": str(tmp_path / "vanished")} + env.pop("QUORUMGIT_AGENT", None) + if agent: + env["QUORUMGIT_AGENT"] = agent + result = subprocess.run( + [sys.executable, "-m", "quorumgit", "hook", "reference-transaction", + "--repo", "any", state], + input=f"{'0' * 40} {'1' * 40} refs/heads/x\n", + capture_output=True, + text=True, + env=env, + check=False, + ) + assert result.returncode == code, result.stderr From 2f2d6c6bfe6bd6cdc87154efcabeae12bdc97d5a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 13:18:04 +0000 Subject: [PATCH 6/6] Test that an abort never restores over a newer live approval When Git aborts a transaction after `prepared`, the consumed approval is restored unless the same operation was re-requested meanwhile. Restoring then would leave two live approvals for one operation, so the old one stays consumed and `approval.restore_skipped` is recorded. The test covers that path, which depends on catching sqlite3.IntegrityError: with the libSQL-era ValueError catch the constraint error escapes and the abort is never recorded. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Q8BpCkArA34hB1aSyVcnZp --- tests/test_transaction_boundary.py | 47 ++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/test_transaction_boundary.py b/tests/test_transaction_boundary.py index 0b645e6..be0ed5e 100644 --- a/tests/test_transaction_boundary.py +++ b/tests/test_transaction_boundary.py @@ -173,6 +173,53 @@ def test_aborted_transaction_restores_the_consumed_approval( assert restored is not None and restored[0] == 1 +def test_abort_does_not_restore_over_a_newer_live_approval( + committed_conn, tmp_path, cfg, monkeypatch +): + """Restoring must never leave two live approvals for one operation.""" + conn = committed_conn + repo_name, hub, clone, a, _b = _setup(conn, tmp_path) + _commit(clone, "src/conflict.py") + staged = _push(clone, a, "HEAD:refs/heads/staging", cfg=cfg) + assert staged.returncode == 0, staged.stderr + op, first = _approve_main(conn, repo_name, hub, clone) + + monkeypatch.chdir(hub) + monkeypatch.setenv("GIT_DIR", ".") + monkeypatch.setenv("QUORUMGIT_AGENT", a) + line = [f"{op['oldrev']} {op['newrev']} refs/heads/main\n"] + assert gate.run_pre_receive(conn, repo_name, line) == 0 + assert gate.run_reference_transaction(conn, repo_name, "prepared", line) == 0 + assert gate.get_approval_by_id(conn, first["id"])["status"] == "consumed" + + # While the transaction is in flight the same operation is requested + # again; the consumed instance is not live, so this opens a new one. + second = gate.request_approval(conn, op, requested_by="operator") + conn.commit() + assert second["id"] != first["id"] and second["status"] == "pending" + + assert gate.run_reference_transaction(conn, repo_name, "aborted", line) == 0 + assert gate.get_approval_by_id(conn, first["id"])["status"] == "consumed" + assert gate.get_approval_by_id(conn, second["id"])["status"] == "pending" + live = conn.execute( + "SELECT count(*) FROM approvals WHERE operation_hash = ? " + "AND status IN ('pending', 'approved')", + (gate.operation_hash(op),), + ).fetchone() + assert live is not None and live[0] == 1 + assert _ref_update_statuses(conn, "refs/heads/main", op["newrev"]) == ["aborted"] + events = { + row[0] + for row in conn.execute( + "SELECT event_type FROM audit_events WHERE entity = 'approval' " + "AND entity_id = ?", + (first["id"],), + ).fetchall() + } + assert "approval.restore_skipped" in events + assert "approval.restored" not in events + + def test_prepared_rejects_an_update_pre_receive_did_not_validate( committed_conn, tmp_path, monkeypatch ):