From 1ad1f7ec8fb0ec3002559c0b867ab1aa04243ee6 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 08:37:07 +0000 Subject: [PATCH 1/7] Move approval authority to the repository and continue retained worktrees Approvals previously proved only that the voter was a registered agent, and the requester chose the threshold (default 1), so a worker could request and approve its own protected push or takeover. Authority: - agents carry a role (worker, reviewer, operator; existing agents upgrade as workers) - each repository owns its approval policy: threshold, approving roles, and whether the requester may vote; `approve request --threshold` is removed - only approving roles may vote, the requester may not by default, and a takeover's beneficiary may never vote on it - an agent that approved an operation cannot consume it (pusher or taker), enforced in the gate and by a database trigger - eligible votes are re-derived from current roles and policy at use time, so a tightened policy or a demoted approver invalidates old approvals - role and policy changes require an operator after bootstrap, the last operator cannot be demoted, and both are audited - the store contract now requires the governance triggers by name Worktrees: - a claim that supersedes an earlier claim on the same task and branch (lease expiry, approved takeover, release that kept its checkout) verifies and transfers the retained worktree instead of failing on Git's one-checkout-per-branch rule; drifted or other-task checkouts are refused - managed worktree paths are built from IDs plus a random suffix, so agent and repository names can no longer place a checkout outside the managed directory Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SrukvuPz7kpbore3kkKpQ3 --- README.md | 43 +- src/quorumgit/cli.py | 98 ++++- src/quorumgit/gate.py | 212 ++++++++-- .../migrations/004_approval_authority.sql | 116 ++++++ src/quorumgit/registry.py | 174 +++++++- src/quorumgit/store.py | 41 +- src/quorumgit/trees.py | 183 +++++++-- tests/conftest.py | 40 +- tests/test_acceptance.py | 10 +- tests/test_approval_authority.py | 388 ++++++++++++++++++ tests/test_authorization_binding.py | 79 ++-- tests/test_gate.py | 64 +-- tests/test_review_fixes.py | 39 +- tests/test_worktree_continuation.py | 202 +++++++++ 14 files changed, 1505 insertions(+), 184 deletions(-) create mode 100644 src/quorumgit/migrations/004_approval_authority.sql create mode 100644 tests/test_approval_authority.py create mode 100644 tests/test_worktree_continuation.py diff --git a/README.md b/README.md index adfd0cf..67d2913 100644 --- a/README.md +++ b/README.md @@ -25,17 +25,18 @@ QuorumGit makes each of these either impossible or explicitly governed, using me ## The mental model -Seven concepts, in the order you meet them: +Eight concepts, in the order you meet them: | Concept | What it is | |---|---| | **Repository** | A registered Git repo that QuorumGit governs. | -| **Agent** | A registered identity. Set via `QUORUMGIT_AGENT` or `--agent`. | +| **Agent** | A registered identity with one role — `worker` (the default), `reviewer`, or `operator`. Set via `QUORUMGIT_AGENT` or `--agent`. | | **Task** | A unit of work against one repository. | | **Claim** | An agent's exclusive lease on a task: names a branch, declares write **scopes** (path globs), and expires at a timestamp. Expired leases make the task reclaimable and cannot be renewed — evaluated at read time, no timers. | -| **Worktree** | An isolated `git worktree` created per claim. Agents never share a mutable checkout; Git itself refuses to check one branch out twice. | +| **Worktree** | An isolated `git worktree` created per claim. Agents never share a mutable checkout; Git itself refuses to check one branch out twice. A claim that supersedes an earlier claim on the same task and branch continues its retained checkout instead of creating a second one. | | **Handoff** | A structured continuation record (done / remaining / exact commit / blockers) that transfers work to a successor instead of abandoning it. | -| **Approval** | An operator sign-off, hash-bound to one exact operation (a specific push, takeover, or deletion), consumed on use. | +| **Approval** | A sign-off, hash-bound to one exact operation (a specific push, takeover, or deletion), consumed on use. | +| **Approval policy** | Owned by each repository: how many votes an approval needs, which roles may cast them, and whether the requester may vote. The agent asking for permission never chooses its own quorum. | Everything an agent does — claim, renew, checkpoint, hand off, release — writes an audit event in the same database transaction. The audit table is append-only, enforced by a trigger. @@ -72,6 +73,7 @@ Register a repository and two agents, then run one full work cycle: ```bash quorumgit repo add myproject /path/to/working-repo +quorumgit agent add lead --role operator # the first operator needs no sponsor quorumgit agent add agent-one quorumgit agent add agent-two export QUORUMGIT_AGENT=agent-one @@ -81,10 +83,12 @@ quorumgit task add --repo myproject --title "implement feature X" quorumgit claim 1 --branch feat/x --scope 'src/**' # claim 1 acquired (CLEAR). -# worktree: ~/.quorumgit/worktrees/myproject/task-1-agent-one-claim-1 +# worktree: ~/.quorumgit/worktrees/repo-1/claim-1-3f9a0c1e # branch: feat/x ``` +Managed worktree paths are built from database IDs plus a random suffix. Repository and agent names are display identities and never become path components, so no registered name can place a checkout outside `QUORUMGIT_DATA_DIR/worktrees`. + The agent now works **inside that worktree** — edits, commits, tests — without touching anyone else's checkout. Along the way: ```bash @@ -164,11 +168,11 @@ The flow, driven by the rejection messages themselves: # protected_ref_update on refs/heads/main requires an approval # bound to this exact update (hash sha256:ab12…). -# 2. A registered operator requests that exact operation, then votes on the -# returned approval instance ID: -quorumgit approve request '{"type":"protected_ref_update","repository":"myproject","refname":"refs/heads/main","oldrev":"","newrev":""}' -# approval 17 hash=sha256:ab12… status=pending -quorumgit approve vote 17 --agent operator +# 2. The agent requests that exact operation; an operator votes on the returned +# approval instance ID. The threshold comes from the repository's policy: +QUORUMGIT_AGENT=agent-one quorumgit approve request '{"type":"protected_ref_update","repository":"myproject","refname":"refs/heads/main","oldrev":"","newrev":""}' +# approval 17 hash=sha256:ab12… status=pending threshold=1 +quorumgit approve vote 17 --agent lead # 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. @@ -181,10 +185,20 @@ Rules that hold no matter what: - **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. +- **Nobody authorizes themselves.** Only agents whose role the repository's policy names may vote, in either direction. The requester may not vote unless the policy says so, the agent a takeover would benefit may not vote on it, and an agent that voted to approve an operation can never be the one that carries it out — pushes and takeovers by an approver are rejected, and a database trigger refuses the consumption even for writers that bypass the CLI. +- **Authority is checked when it is used, not only when it is granted.** Consumption re-derives eligible votes from current roles and policy, so raising the threshold or demoting an approver invalidates approvals that no longer meet it. +- **The repository owns the policy.** New repositories require one vote from an `operator`, and the requester may not vote. Change it with `quorumgit repo policy [--threshold ] [--role ]… [--requester-may-vote | --no-requester-may-vote]`. `approve request` no longer accepts `--threshold`. + +### Roles and administration + +Registering a `reviewer` or `operator`, changing a role (`quorumgit agent role `), and changing repository policy are themselves authority decisions, so each requires an operator acting through `--agent` or `QUORUMGIT_AGENT`, and each is audited. The single exception is bootstrap: while no operator exists, the first one can be designated by anyone. The last remaining operator cannot be demoted, so a store never falls back into bootstrap mode by accident. + +> **Upgrading.** Migration `004_approval_authority.sql` makes every existing agent a `worker` and gives every existing repository the default policy. Approvals granted under the old rules (including self-approvals) stop authorizing anything until an eligible operator votes on a fresh request. After running `quorumgit init`, designate an operator with `quorumgit agent role operator`. 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. +The incumbent's checkout is not duplicated. When a claim supersedes an earlier claim on the same task and branch — an approved takeover, a reclaim after lease expiry, or a re-claim after `release` without `--remove-worktree` — the retained worktree, including any uncommitted work, is verified against its recorded repository and branch and transferred to the new claim (audited as `worktree.continued`). A retained checkout that has drifted to another branch or belongs to a different task is never adopted; the claim is refused with an explanation and rolls back entirely. A recorded checkout whose directory is gone has nothing to continue, so a fresh worktree is created and `quorumgit doctor` reports the stale record. + ## Command reference | Command | Purpose | @@ -194,7 +208,9 @@ Takeovers follow the same pattern: claiming a task someone else holds (`claim [--protected-ref ]…` | Register a repository | -| `quorumgit agent add ` | Register an agent identity | +| `quorumgit agent add [--role worker\|reviewer\|operator]` | Register an agent identity (non-workers need an operator once one exists) | +| `quorumgit agent role ` | Change an agent's role (operator only, after bootstrap) | +| `quorumgit repo policy [--threshold ] [--role ]… [--[no-]requester-may-vote]` | Show or change a repository's approval policy (changes are operator only) | | `quorumgit task add --repo --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 | @@ -205,7 +221,7 @@ Takeovers follow the same pattern: claiming a task someone else holds (`claim ` | Decline — addressee only; clean retained worktree is removed | | `quorumgit handoff cancel ` | Cancel — creator only; clean retained worktree is removed | | `quorumgit handoff list / show ` | Inspect handoffs | -| `quorumgit approve request [--threshold ]` | Open an approval for an exact operation | +| `quorumgit approve request ` | Open an approval for an exact operation, at the repository's threshold | | `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) | @@ -240,6 +256,7 @@ QuorumGit v1 coordinates **cooperating agents**; the adversary is *accident, not - 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. - Approval hashes provide exact-payload binding and tamper-evidence, not approver authentication. +- Roles separate duties between cooperating agents. Because identity is asserted, a process willing to impersonate an operator can still do so; role separation stops an agent from authorizing its own work by accident or by default, not a determined impersonator. 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 4749d04..1ce4992 100644 --- a/src/quorumgit/cli.py +++ b/src/quorumgit/cli.py @@ -119,18 +119,63 @@ def cmd_repo_list(args, cfg) -> int: return 0 +def cmd_repo_policy(args, cfg) -> int: + with store.connect(cfg) as conn: + changing = ( + args.threshold is not None + or args.role + or args.requester_may_vote is not None + ) + if changing: + policy = registry.set_approval_policy( + conn, + args.name, + actor=getattr(args, "agent", None) or cfg.agent, + threshold=args.threshold, + roles=args.role or None, + requester_may_vote=args.requester_may_vote, + ) + conn.commit() + else: + repo = registry.get_repository(conn, args.name) + policy = registry.approval_policy(conn, repo["id"]) + print(f"repository: {args.name}") + print(f"approval threshold: {policy['threshold']}") + print(f"approving roles: {', '.join(policy['roles'])}") + print(f"requester may vote: {'yes' if policy['requester_may_vote'] else 'no'}") + return 0 + + def cmd_agent_add(args, cfg) -> int: with store.connect(cfg) as conn: - agent_id = registry.add_agent(conn, args.name) + agent_id = registry.add_agent( + conn, + args.name, + role=args.role, + actor=getattr(args, "agent", None) or cfg.agent, + ) conn.commit() - print(f"agent {args.name} registered (id {agent_id}).") + print(f"agent {args.name} registered as {args.role} (id {agent_id}).") + return 0 + + +def cmd_agent_role(args, cfg) -> int: + with store.connect(cfg) as conn: + registry.set_agent_role( + conn, + args.name, + args.role, + actor=getattr(args, "agent", None) or cfg.agent, + ) + conn.commit() + print(f"agent {args.name} is now a {args.role}.") return 0 def cmd_agent_list(args, cfg) -> int: with store.connect(cfg) as conn: for agent in registry.list_agents(conn): - print(f"{agent['id']}\t{agent['name']}") + print(f"{agent['id']}\t{agent['name']}\t{agent['role']}") return 0 @@ -173,11 +218,17 @@ def cmd_claim(args, cfg) -> int: "from_agent": holder["agent"], "to_agent": agent, } - takeover_approval = gate.approved_instance(conn, takeover_operation) + takeover_approval = gate.approved_instance( + conn, takeover_operation, consumer=agent + ) if takeover_approval is None: + refusal = gate.approval_refusal( + conn, takeover_operation, consumer=agent + ) print( "[quorumgit] REFUSED: lease takeover requires approval.\n" - f"operation hash: " + + (f"{refusal}\n" if refusal else "") + + "operation hash: " f"{gate.operation_hash(takeover_operation)}\n" "Request/approve it via `quorumgit approve request/vote` " "with operation:\n" @@ -219,11 +270,16 @@ def cmd_claim(args, cfg) -> int: return 1 wt = None if not args.no_worktree: - wt = trees.create_worktree(conn, claim_id, cfg.worktrees_dir) + wt = trees.continue_or_create_worktree(conn, claim_id, cfg.worktrees_dir) conn.commit() print(f"claim {claim_id} acquired ({classification}).") if wt: print(f"worktree: {wt['path']}") + if wt.get("continued_from_claim_id"): + print( + "continued retained worktree of claim " + f"{wt['continued_from_claim_id']} (uncommitted work preserved)" + ) print(f"branch: {wt['branch']}") else: print(f"branch: {args.branch} (no worktree — work from your own clone " @@ -358,13 +414,12 @@ def _operation_from_args(args) -> dict: def cmd_approve_request(args, cfg) -> int: with store.connect(cfg) as conn: approval = gate.request_approval( - conn, _operation_from_args(args), requested_by=_agent(args, cfg), - threshold=args.threshold, + conn, _operation_from_args(args), requested_by=_agent(args, cfg) ) conn.commit() print( f"approval {approval['id']} hash={approval['operation_hash']} " - f"status={approval['status']}" + f"status={approval['status']} threshold={approval['threshold']}" ) return 0 @@ -444,9 +499,31 @@ def add(name, fn, configure=None, parent=None): sp.add_argument("--protected-ref", action="append", default=[]), ), parent=repo) add("list", cmd_repo_list, parent=repo) + add("policy", cmd_repo_policy, lambda sp: ( + sp.add_argument("name"), + sp.add_argument("--agent", help="operator making the change"), + sp.add_argument("--threshold", type=int, + help="eligible yes votes required"), + sp.add_argument("--role", action="append", default=[], + choices=registry.ROLES, + help="role whose votes count (repeatable; replaces " + "the current set)"), + sp.add_argument("--requester-may-vote", dest="requester_may_vote", + action=argparse.BooleanOptionalAction, default=None), + ), parent=repo) agent = sub.add_parser("agent").add_subparsers(dest="sub", required=True) - add("add", cmd_agent_add, lambda sp: sp.add_argument("name"), parent=agent) + add("add", cmd_agent_add, lambda sp: ( + sp.add_argument("name"), + sp.add_argument("--role", choices=registry.ROLES, + default=registry.DEFAULT_ROLE), + sp.add_argument("--agent", help="operator registering a non-worker"), + ), parent=agent) + add("role", cmd_agent_role, lambda sp: ( + sp.add_argument("name"), + sp.add_argument("role", choices=registry.ROLES), + sp.add_argument("--agent", help="operator making the change"), + ), parent=agent) add("list", cmd_agent_list, parent=agent) task = sub.add_parser("task").add_subparsers(dest="sub", required=True) @@ -526,7 +603,6 @@ def add(name, fn, configure=None, parent=None): add("request", cmd_approve_request, lambda sp: ( sp.add_argument("operation", help="operation JSON object"), sp.add_argument("--agent"), - sp.add_argument("--threshold", type=int, default=1), ), parent=ap) add("vote", cmd_approve_vote, lambda sp: ( sp.add_argument("approval_id", type=int), diff --git a/src/quorumgit/gate.py b/src/quorumgit/gate.py index a6a1d7b..9b40e7f 100644 --- a/src/quorumgit/gate.py +++ b/src/quorumgit/gate.py @@ -1,7 +1,11 @@ """Approval gate and pre-receive enforcement. Protected operations require an approval whose hash binds to the exact -operation payload. Enforcement is fail-closed: any hook error rejects the push. +operation payload. Who may authorize is owned by the operation's repository: +its approval policy names the threshold, the roles whose votes count, and +whether the requester may vote. An agent that approved an operation may never +be the one that carries it out. Enforcement is fail-closed: any hook error +rejects the push. """ from __future__ import annotations @@ -18,6 +22,7 @@ from .canonical import stable_hash from .registry import ( RegistryError, + approval_policy, assert_repository_identity_unique, get_agent, get_repository, @@ -25,7 +30,6 @@ from .store import Connection, begin_immediate, json_dumps, json_loads from .work import live_claim_for_branch, open_handoff_for_branch -DEFAULT_THRESHOLD = 1 HOOK_MARKER = "# quorumgit-managed-pre-receive v1" @@ -77,6 +81,12 @@ def _validate_takeover_operation(conn: Connection, operation: dict[str, Any]) -> ) +_APPROVAL_COLUMNS = ( + "id, operation_hash, operation, threshold, status, consumed_at, " + "repository_id, requested_by_agent_id" +) + + def _approval_dict(row) -> dict: return { "id": row[0], @@ -85,47 +95,147 @@ def _approval_dict(row) -> dict: "threshold": row[3], "status": row[4], "consumed_at": row[5], + "repository_id": row[6], + "requested_by_agent_id": row[7], } +def _operation_repository(conn: Connection, operation: dict[str, Any]) -> dict: + try: + return get_repository(conn, str(operation["repository"])) + except RegistryError as exc: + raise GateError( + f"Approval operations must name a registered repository: {exc}" + ) from exc + + +def _policy_for(conn: Connection, approval: dict) -> dict: + if approval["repository_id"] is None: + raise GateError( + f"Approval {approval['id']} predates repository approval policy " + "and names no registered repository; request it again." + ) + return approval_policy(conn, approval["repository_id"]) + + +def _vote_refusal(approval: dict, policy: dict, voter: dict) -> str | None: + """Why this agent may not vote on this approval, or None if it may.""" + if voter["role"] not in policy["roles"]: + return ( + f"{voter['name']} is a {voter['role']}; this repository accepts " + f"votes only from: {', '.join(policy['roles']) or 'nobody'}." + ) + if ( + not policy["requester_may_vote"] + and approval["requested_by_agent_id"] == voter["id"] + ): + return f"{voter['name']} requested this approval and may not vote on it." + operation = approval["operation"] + if ( + operation.get("type") == "lease_takeover" + and operation.get("to_agent") == voter["name"] + ): + return f"{voter['name']} would receive this takeover and may not vote on it." + return None + + +def _eligible_approvals( + conn: Connection, + approval: dict, + policy: dict, + exclude_agent_id: int | None = None, +) -> int: + """Count yes votes that still satisfy the repository's current policy. + + Roles and policy can change after a vote is cast, so authority is + re-derived from current state rather than trusted from the vote row. + """ + rows = conn.execute( + """ + SELECT a.id, a.name, a.role + FROM votes v JOIN agents a ON a.id = v.voter_agent_id + WHERE v.approval_id = ? AND v.vote = 1 + """, + (approval["id"],), + ).fetchall() + return sum( + 1 + for agent_id, name, role in rows + if agent_id != exclude_agent_id + and _vote_refusal(approval, policy, {"id": agent_id, "name": name, "role": role}) + is None + ) + + +def _authorization_refusal( + conn: Connection, approval: dict, consumer: dict | None +) -> str | None: + """Why an approved instance cannot authorize this consumer now, or None.""" + policy = _policy_for(conn, approval) + if consumer is not None: + approved_by_consumer = conn.execute( + "SELECT 1 FROM votes WHERE approval_id = ? AND voter_agent_id = ? " + "AND vote = 1", + (approval["id"], consumer["id"]), + ).fetchone() + if approved_by_consumer is not None: + return ( + f"{consumer['name']} approved this operation and may not also " + "carry it out." + ) + eligible = _eligible_approvals(conn, approval, policy) + if eligible < policy["threshold"]: + return ( + f"Approval {approval['id']} has {eligible} eligible approval(s); the " + f"repository policy now requires {policy['threshold']}." + ) + return None + + def request_approval( conn: Connection, operation: dict[str, Any], requested_by: str, - threshold: int = DEFAULT_THRESHOLD, ) -> dict: """Create or return the live approval instance for an exact operation. Pending/approved instances are reused. Denied/consumed instances are terminal history, so the same exact operation may be requested again as a fresh approval instance. BEGIN IMMEDIATE makes that lifecycle race-free. + + The requester does not choose the quorum: the threshold is the policy of + the repository the operation names, recorded on the instance for display + and re-evaluated against current policy whenever it is used. """ begin_immediate(conn) requester = get_agent(conn, requested_by) - _validate_takeover_operation(conn, operation) op_hash = operation_hash(operation) + repo = _operation_repository(conn, operation) + _validate_takeover_operation(conn, operation) existing = conn.execute( - """ - SELECT id, operation_hash, operation, threshold, status, consumed_at + f""" + SELECT {_APPROVAL_COLUMNS} FROM approvals WHERE operation_hash = ? AND status IN ('pending', 'approved') ORDER BY id DESC LIMIT 1 - """, + """, # noqa: S608 -- fixed column list (op_hash,), ).fetchone() if existing is not None: return _approval_dict(existing) + threshold = approval_policy(conn, repo["id"])["threshold"] row = conn.execute( """ INSERT INTO approvals ( - operation_hash, operation, threshold, requested_by_agent_id + operation_hash, operation, threshold, requested_by_agent_id, + repository_id ) - VALUES (?, ?, ?, ?) + VALUES (?, ?, ?, ?, ?) RETURNING id """, - (op_hash, json_dumps(operation), threshold, requester["id"]), + (op_hash, json_dumps(operation), threshold, requester["id"], repo["id"]), ).fetchone() assert row is not None audit.record( @@ -142,13 +252,13 @@ def request_approval( def get_approval(conn: Connection, op_hash: str) -> dict: """Return the newest approval instance for an operation hash.""" row = conn.execute( - """ - SELECT id, operation_hash, operation, threshold, status, consumed_at + f""" + SELECT {_APPROVAL_COLUMNS} FROM approvals WHERE operation_hash = ? ORDER BY id DESC LIMIT 1 - """, + """, # noqa: S608 -- fixed column list (op_hash,), ).fetchone() if row is None: @@ -158,10 +268,7 @@ def get_approval(conn: Connection, op_hash: str) -> dict: def get_approval_by_id(conn: Connection, approval_id: int) -> dict: row = conn.execute( - """ - SELECT id, operation_hash, operation, threshold, status, consumed_at - FROM approvals WHERE id = ? - """, + f"SELECT {_APPROVAL_COLUMNS} FROM approvals WHERE id = ?", # noqa: S608 -- fixed column list (approval_id,), ).fetchone() if row is None: @@ -174,13 +281,18 @@ def vote(conn: Connection, approval_id: int, voter: str, approve: bool) -> dict: BEGIN IMMEDIATE serializes competing voters before either reads the current approval state. Denial has precedence and terminal states remain final. + Only agents the repository's policy recognizes may vote, in either + direction; the requester may not vote unless the policy allows it. """ begin_immediate(conn) voter_row = get_agent(conn, voter) approval = get_approval_by_id(conn, approval_id) if approval["status"] != "pending": raise GateError(f"Approval {approval_id} is already {approval['status']}.") - threshold = approval["threshold"] + policy = _policy_for(conn, approval) + refusal = _vote_refusal(approval, policy, voter_row) + if refusal is not None: + raise GateError(f"Vote refused: {refusal}") conn.execute( """ INSERT INTO votes (approval_id, voter, vote, voter_agent_id) @@ -200,17 +312,14 @@ def vote(conn: Connection, approval_id: int, voter: str, approve: bool) -> dict: detail={"vote": approve, "hash": approval["operation_hash"]}, ) - counts = conn.execute( - "SELECT count(*) FILTER (WHERE vote = 1), " - "count(*) FILTER (WHERE vote = 0) " - "FROM votes WHERE approval_id = ?", + denials = conn.execute( + "SELECT count(*) FROM votes WHERE approval_id = ? AND vote = 0", (approval["id"],), ).fetchone() - assert counts is not None - yes, no = counts - if no > 0: + assert denials is not None + if denials[0] > 0: new_status = "denied" - elif yes >= threshold: + elif _eligible_approvals(conn, approval, policy) >= policy["threshold"]: new_status = "approved" else: new_status = "pending" @@ -230,23 +339,51 @@ def vote(conn: Connection, approval_id: int, voter: str, approve: bool) -> dict: return get_approval_by_id(conn, approval_id) -def is_approved(conn: Connection, operation: dict[str, Any]) -> bool: - """True only if the newest instance for this exact operation is approved.""" +def approved_instance( + conn: Connection, + operation: dict[str, Any], + consumer: str | None = None, +) -> dict | None: + """The approved instance for this exact operation, if it can be used now. + + With a consumer, the instance must also be usable by that agent: an agent + that approved an operation cannot be the one that carries it out. + """ try: approval = get_approval(conn, operation_hash(operation)) except GateError: - return False - return approval["status"] == "approved" and approval["operation"] == operation + return None + if approval["status"] != "approved" or approval["operation"] != operation: + return None + try: + consumer_row = get_agent(conn, consumer) if consumer else None + if _authorization_refusal(conn, approval, consumer_row) is not None: + return None + except (GateError, RegistryError): + return None + return approval -def approved_instance(conn: Connection, operation: dict[str, Any]) -> dict | None: +def approval_refusal( + conn: Connection, operation: dict[str, Any], consumer: str | None = None +) -> str | None: + """Why an existing approved instance cannot be used, for error messages.""" try: approval = get_approval(conn, operation_hash(operation)) except GateError: return None - if approval["status"] == "approved" and approval["operation"] == operation: - return approval - return None + if approval["status"] != "approved": + return None + try: + consumer_row = get_agent(conn, consumer) if consumer else None + return _authorization_refusal(conn, approval, consumer_row) + except (GateError, RegistryError) as exc: + return str(exc) + + +def is_approved(conn: Connection, operation: dict[str, Any]) -> bool: + """True only if the newest instance for this exact operation is usable.""" + return approved_instance(conn, operation) is not None def consume_approval( @@ -266,6 +403,9 @@ def consume_approval( f"Approval {op_hash} is not consumable (status " f"{approval['status']}); it may already be used." ) + refusal = _authorization_refusal(conn, approval, consumer) + if refusal is not None: + raise GateError(f"Approval {approval_id} cannot authorize {agent}: {refusal}") cur = conn.execute( "UPDATE approvals SET status = 'consumed', consumed_at = unixepoch(), " "consumed_by_agent_id = ? " @@ -396,11 +536,13 @@ def check_ref_update( "oldrev": oldrev, "newrev": newrev, } - approval = approved_instance(conn, operation) + approval = approved_instance(conn, operation, consumer=pusher) if approval is None: + refusal = approval_refusal(conn, operation, consumer=pusher) raise PushRejected( f"{operation['type']} on {refname} requires an approval " f"bound to this exact update (hash {operation_hash(operation)})." + + (f" {refusal}" if refusal else "") ) assert pusher is not None consume_approval(conn, approval["id"], operation, agent=pusher) diff --git a/src/quorumgit/migrations/004_approval_authority.sql b/src/quorumgit/migrations/004_approval_authority.sql new file mode 100644 index 0000000..81df7a4 --- /dev/null +++ b/src/quorumgit/migrations/004_approval_authority.sql @@ -0,0 +1,116 @@ +-- Approval authority: who may authorize is decided by the repository, not by +-- the agent asking for permission. +-- +-- Agents carry one role. Every repository owns its approval policy: the +-- threshold, the roles whose votes count, and whether the requester may vote. +-- Existing agents become workers, so no historical identity silently gains +-- authority; an operator must be designated explicitly after upgrading. + +ALTER TABLE agents + ADD COLUMN role TEXT NOT NULL DEFAULT 'worker' + CHECK (role IN ('worker', 'reviewer', 'operator')); + +-- quorumgit-statement +ALTER TABLE repositories + ADD COLUMN approval_threshold INTEGER NOT NULL DEFAULT 1 + CHECK (approval_threshold >= 1); + +-- quorumgit-statement +ALTER TABLE repositories + ADD COLUMN requester_may_vote INTEGER NOT NULL DEFAULT 0 + CHECK (requester_may_vote IN (0, 1)); + +-- quorumgit-statement +CREATE TABLE repository_approval_roles ( + id INTEGER PRIMARY KEY, + repository_id INTEGER NOT NULL REFERENCES repositories(id) ON DELETE CASCADE, + role TEXT NOT NULL CHECK (role IN ('worker', 'reviewer', 'operator')), + UNIQUE (repository_id, role) +); + +-- quorumgit-statement +INSERT INTO repository_approval_roles (repository_id, role) +SELECT id, 'operator' FROM repositories; + +-- quorumgit-statement +CREATE TRIGGER repositories_default_approval_roles +AFTER INSERT ON repositories +BEGIN + INSERT INTO repository_approval_roles (repository_id, role) + VALUES (NEW.id, 'operator'); +END; + +-- quorumgit-statement +ALTER TABLE approvals + ADD COLUMN repository_id INTEGER REFERENCES repositories(id); + +-- quorumgit-statement +UPDATE approvals +SET repository_id = ( + SELECT r.id FROM repositories r + WHERE r.name = json_extract(approvals.operation, '$.repository') +) +WHERE repository_id IS NULL; + +-- quorumgit-statement +CREATE TRIGGER approvals_require_registered_repository +BEFORE INSERT ON approvals +WHEN NEW.repository_id IS NULL + OR NOT EXISTS ( + SELECT 1 FROM repositories + WHERE id = NEW.repository_id + AND name = json_extract(NEW.operation, '$.repository') + ) +BEGIN + SELECT RAISE(ABORT, 'approval must name a registered repository'); +END; + +-- quorumgit-statement +CREATE TRIGGER votes_require_eligible_voter +BEFORE INSERT ON votes +WHEN NOT EXISTS ( + SELECT 1 + FROM approvals ap + JOIN repositories r ON r.id = ap.repository_id + JOIN agents v ON v.id = NEW.voter_agent_id + JOIN repository_approval_roles rr + ON rr.repository_id = r.id AND rr.role = v.role + WHERE ap.id = NEW.approval_id + AND (r.requester_may_vote = 1 + OR ap.requested_by_agent_id IS NOT NEW.voter_agent_id) +) +BEGIN + SELECT RAISE(ABORT, 'approval voter is not eligible under repository policy'); +END; + +-- quorumgit-statement +CREATE TRIGGER vote_updates_require_eligible_voter +BEFORE UPDATE OF vote, voter, voter_agent_id ON votes +WHEN NOT EXISTS ( + SELECT 1 + FROM approvals ap + JOIN repositories r ON r.id = ap.repository_id + JOIN agents v ON v.id = NEW.voter_agent_id + JOIN repository_approval_roles rr + ON rr.repository_id = r.id AND rr.role = v.role + WHERE ap.id = NEW.approval_id + AND (r.requester_may_vote = 1 + OR ap.requested_by_agent_id IS NOT NEW.voter_agent_id) +) +BEGIN + SELECT RAISE(ABORT, 'approval voter is not eligible under repository policy'); +END; + +-- quorumgit-statement +CREATE TRIGGER approvals_consumer_is_not_approver +BEFORE UPDATE OF status ON approvals +WHEN NEW.status = 'consumed' + AND EXISTS ( + SELECT 1 FROM votes + WHERE approval_id = NEW.id + AND voter_agent_id = NEW.consumed_by_agent_id + AND vote = 1 + ) +BEGIN + SELECT RAISE(ABORT, 'an approver cannot consume its own approval'); +END; diff --git a/src/quorumgit/registry.py b/src/quorumgit/registry.py index ff40e63..6248b8a 100644 --- a/src/quorumgit/registry.py +++ b/src/quorumgit/registry.py @@ -9,10 +9,51 @@ from .store import Connection, begin_immediate +ROLES = ("worker", "reviewer", "operator") +DEFAULT_ROLE = "worker" + + class RegistryError(RuntimeError): pass +def _validate_role(role: str) -> str: + if role not in ROLES: + raise RegistryError( + f"Unknown role {role!r}; expected one of: {', '.join(ROLES)}." + ) + return role + + +def _operator_count(conn: Connection) -> int: + row = conn.execute( + "SELECT count(*) FROM agents WHERE role = 'operator'" + ).fetchone() + assert row is not None + return row[0] + + +def require_operator(conn: Connection, actor: str | None, action: str) -> None: + """Refuse an administrative change unless an operator performs it. + + Roles and approval policy decide who may authorize protected operations, + so changing them is itself an authority decision. Until the first operator + exists there is nobody to ask, so the store is in bootstrap mode and the + change is allowed (and audited with whatever actor was given). + """ + if _operator_count(conn) == 0: + return + if not actor: + raise RegistryError( + f"{action} requires an operator; pass --agent or set QUORUMGIT_AGENT." + ) + row = get_agent(conn, actor) + if row["role"] != "operator": + raise RegistryError( + f"{action} requires an operator; {actor} is a {row['role']}." + ) + + def _protected_refs(conn: Connection, repository_id: int) -> list[str]: return [ row[0] @@ -165,6 +206,82 @@ def get_repository(conn: Connection, name: str) -> dict: } +def approval_policy(conn: Connection, repository_id: int) -> dict: + """The repository-owned rule for who may authorize its operations.""" + row = conn.execute( + "SELECT approval_threshold, requester_may_vote FROM repositories " + "WHERE id = ?", + (repository_id,), + ).fetchone() + if row is None: + raise RegistryError(f"No such repository id: {repository_id}") + roles = [ + r[0] + for r in conn.execute( + "SELECT role FROM repository_approval_roles " + "WHERE repository_id = ? ORDER BY role", + (repository_id,), + ).fetchall() + ] + return { + "threshold": row[0], + "requester_may_vote": bool(row[1]), + "roles": roles, + } + + +def set_approval_policy( + conn: Connection, + repository: str, + *, + actor: str | None, + threshold: int | None = None, + roles: list[str] | None = None, + requester_may_vote: bool | None = None, +) -> dict: + begin_immediate(conn) + repo = get_repository(conn, repository) + require_operator(conn, actor, "Changing approval policy") + before = approval_policy(conn, repo["id"]) + if threshold is not None: + if threshold < 1: + raise RegistryError("Approval threshold must be at least 1.") + conn.execute( + "UPDATE repositories SET approval_threshold = ? WHERE id = ?", + (threshold, repo["id"]), + ) + if roles is not None: + wanted = sorted({_validate_role(role) for role in roles}) + if not wanted: + raise RegistryError("At least one approving role is required.") + conn.execute( + "DELETE FROM repository_approval_roles WHERE repository_id = ?", + (repo["id"],), + ) + for role in wanted: + conn.execute( + "INSERT INTO repository_approval_roles (repository_id, role) " + "VALUES (?, ?)", + (repo["id"], role), + ) + if requester_may_vote is not None: + conn.execute( + "UPDATE repositories SET requester_may_vote = ? WHERE id = ?", + (1 if requester_may_vote else 0, repo["id"]), + ) + after = approval_policy(conn, repo["id"]) + if after != before: + audit.record( + conn, + "repository.policy_changed", + "repository", + repo["id"], + agent=actor, + detail={"before": before, "after": after}, + ) + return after + + def list_repositories(conn: Connection) -> list[dict]: rows = conn.execute( "SELECT id, name, path FROM repositories ORDER BY name" @@ -180,25 +297,68 @@ def list_repositories(conn: Connection) -> list[dict]: ] -def add_agent(conn: Connection, name: str) -> int: +def add_agent( + conn: Connection, + name: str, + role: str = DEFAULT_ROLE, + *, + actor: str | None = None, +) -> int: + _validate_role(role) + begin_immediate(conn) + if role != DEFAULT_ROLE: + require_operator(conn, actor, f"Registering a {role}") row = conn.execute( - "INSERT INTO agents (name) VALUES (?) RETURNING id", (name,) + "INSERT INTO agents (name, role) VALUES (?, ?) RETURNING id", (name, role) ).fetchone() assert row is not None agent_id = row[0] - audit.record(conn, "agent.registered", "agent", agent_id, agent=name) + detail = {"role": role} + if actor: + detail["registered_by"] = actor + audit.record( + conn, "agent.registered", "agent", agent_id, agent=name, detail=detail + ) return agent_id +def set_agent_role( + conn: Connection, name: str, role: str, *, actor: str | None +) -> None: + _validate_role(role) + begin_immediate(conn) + target = get_agent(conn, name) + require_operator(conn, actor, "Changing an agent role") + if target["role"] == role: + return + if target["role"] == "operator" and _operator_count(conn) == 1: + # Demoting the last operator would drop the store back into bootstrap + # mode, where any agent could then promote itself. + raise RegistryError( + f"{name} is the last operator; designate another operator first." + ) + conn.execute("UPDATE agents SET role = ? WHERE id = ?", (role, target["id"])) + audit.record( + conn, + "agent.role_changed", + "agent", + target["id"], + agent=actor, + detail={"agent": name, "from": target["role"], "to": role}, + ) + + def get_agent(conn: Connection, name: str) -> dict: row = conn.execute( - "SELECT id, name FROM agents WHERE name = ?", (name,) + "SELECT id, name, role FROM agents WHERE name = ?", (name,) ).fetchone() if row is None: raise RegistryError(f"Agent is not registered: {name}") - return {"id": row[0], "name": row[1]} + return {"id": row[0], "name": row[1], "role": row[2]} def list_agents(conn: Connection) -> list[dict]: - rows = conn.execute("SELECT id, name FROM agents ORDER BY name").fetchall() - return [{"id": r[0], "name": r[1]} for r in rows] + rows = conn.execute( + "SELECT id, name, role FROM agents ORDER BY name" + ).fetchall() + return [{"id": r[0], "name": r[1], "role": r[2]} for r in rows] diff --git a/src/quorumgit/store.py b/src/quorumgit/store.py index bca20f9..9acf079 100644 --- a/src/quorumgit/store.py +++ b/src/quorumgit/store.py @@ -50,6 +50,23 @@ "votes", "conflict_events", "audit_events", + "repository_approval_roles", +) + +# Engine-level governance rules. A store missing any of these would still +# have every table, so the contract checks them by name as well. +REQUIRED_TRIGGERS = ( + "audit_events_no_update", + "audit_events_no_delete", + "approvals_require_registered_requester", + "approvals_require_registered_consumer", + "votes_require_registered_voter", + "vote_updates_require_registered_voter", + "repositories_default_approval_roles", + "approvals_require_registered_repository", + "votes_require_eligible_voter", + "vote_updates_require_eligible_voter", + "approvals_consumer_is_not_approver", ) @@ -312,10 +329,9 @@ def verify_contract(target: Config | Connection | str | Path) -> None: "SELECT name FROM sqlite_master WHERE type = 'table'" ).fetchall() } - missing = set(REQUIRED_TABLES) - tables - if missing: + if "schema_migrations" not in tables: raise ContractViolation( - f"Missing required tables: {sorted(missing)}. " + "Missing required tables: ['schema_migrations']. " "Run `quorumgit init` to apply migrations." ) @@ -333,6 +349,25 @@ def verify_contract(target: Config | Connection | str | Path) -> None: "Run `quorumgit init` to apply migrations." ) + missing = set(REQUIRED_TABLES) - tables + if missing: + raise ContractViolation( + f"Missing required tables: {sorted(missing)}. " + "Run `quorumgit init` to apply migrations." + ) + + triggers = { + row[0] + for row in conn.execute( + "SELECT name FROM sqlite_master WHERE type = 'trigger'" + ).fetchall() + } + missing_triggers = set(REQUIRED_TRIGGERS) - triggers + if missing_triggers: + raise ContractViolation( + f"Missing required governance triggers: {sorted(missing_triggers)}." + ) + foreign_keys = conn.execute("PRAGMA foreign_keys").fetchone()[0] journal_mode = str(conn.execute("PRAGMA journal_mode").fetchone()[0]).lower() if foreign_keys != 1: diff --git a/src/quorumgit/trees.py b/src/quorumgit/trees.py index 904830b..928f3a4 100644 --- a/src/quorumgit/trees.py +++ b/src/quorumgit/trees.py @@ -8,6 +8,7 @@ from __future__ import annotations +import secrets import subprocess from pathlib import Path @@ -42,6 +43,25 @@ def _git_common_dir(repo_path: str | Path) -> Path: return common.resolve() +def managed_worktree_path( + worktrees_dir: Path, repository_id: int, claim_id: int +) -> Path: + """Filesystem location for a new managed worktree. + + Built only from database-generated integer IDs and a random suffix: + repository and agent names are display identities and never become path + components, so no registered name can steer a checkout outside the + directory QuorumGit owns. The suffix keeps the path fresh even when SQLite + reuses the rowid of a rolled-back claim whose checkout was left behind. + """ + root = Path(worktrees_dir).resolve() + leaf = f"claim-{int(claim_id)}-{secrets.token_hex(4)}" + path = (root / f"repo-{int(repository_id)}" / leaf).resolve() + if not path.is_relative_to(root): + raise WorktreeError(f"Managed worktree path escapes {root}: {path}") + return path + + def create_worktree( conn: Connection, claim_id: int, worktrees_dir: Path, base_ref: str = "HEAD" ) -> dict: @@ -52,14 +72,7 @@ def create_worktree( repo_path = task["repository_path"] branch = claim["branch"] - # Claim identity is part of the managed path. Re-claiming the same task by - # the same agent therefore creates a fresh path without colliding with the - # historical UNIQUE(worktrees.path) row from an earlier claim. - wt_path = ( - worktrees_dir - / task["repository"] - / f"task-{task['id']}-{claim['agent']}-claim-{claim_id}" - ) + wt_path = managed_worktree_path(worktrees_dir, task["repository_id"], claim_id) wt_path.parent.mkdir(parents=True, exist_ok=True) if wt_path.exists(): raise WorktreeError(f"Worktree path already exists: {wt_path}") @@ -121,6 +134,112 @@ def worktree_for_claim(conn: Connection, claim_id: int) -> dict | None: return {"id": row[0], "path": row[1], "branch": row[2], "removed_at": row[3]} +def _retained_worktree_for_branch( + conn: Connection, repository_id: int, branch: str, exclude_claim_id: int +) -> dict | None: + """A live recorded worktree with this branch checked out, if any.""" + row = conn.execute( + """ + SELECT w.id, w.path, w.branch, c.id, c.task_id, c.released_at, + c.release_reason, + EXISTS ( + SELECT 1 FROM handoffs h + WHERE h.from_claim_id = c.id AND h.status = 'open' + ) + FROM worktrees w + JOIN claims c ON c.id = w.claim_id + JOIN tasks t ON t.id = c.task_id + WHERE t.repository_id = ? AND w.branch = ? AND w.removed_at IS NULL + AND c.id <> ? + ORDER BY w.id DESC + LIMIT 1 + """, + (repository_id, branch, exclude_claim_id), + ).fetchone() + if row is None: + return None + return { + "id": row[0], + "path": row[1], + "branch": row[2], + "claim_id": row[3], + "task_id": row[4], + "released_at": row[5], + "release_reason": row[6], + "open_handoff": bool(row[7]), + } + + +def continue_or_create_worktree( + conn: Connection, claim_id: int, worktrees_dir: Path, base_ref: str = "HEAD" +) -> dict: + """Give a new claim its worktree, continuing a retained one when it exists. + + A claim that supersedes an earlier claim on the same task and branch — + after lease expiry, an approved takeover, or a release that kept its + checkout — cannot get a second checkout: Git refuses to check one branch + out twice. Instead the retained checkout, with any unfinished work in it, + is verified and transferred to the new claim, and the transfer is audited. + A retained checkout belonging to a different task is never adopted. + """ + claim = get_claim(conn, claim_id) + if claim["released_at"] is not None: + raise WorktreeError(f"Claim {claim_id} is released.") + task = get_task(conn, claim["task_id"]) + retained = _retained_worktree_for_branch( + conn, task["repository_id"], claim["branch"], exclude_claim_id=claim_id + ) + if retained is None or not Path(retained["path"]).exists(): + # A recorded checkout whose directory is gone holds no work to + # continue; doctor reports and reconciles that record separately. + return create_worktree(conn, claim_id, worktrees_dir, base_ref) + + where = ( + f"Branch {claim['branch']!r} is still checked out in retained worktree " + f"{retained['path']} (claim {retained['claim_id']}, task " + f"{retained['task_id']})" + ) + if retained["task_id"] != task["id"]: + raise WorktreeError( + f"{where}. It belongs to another task and will not be adopted; " + "its owner should release it with --remove-worktree, or claim with " + "--no-worktree." + ) + if retained["released_at"] is None or retained["open_handoff"]: + raise WorktreeError(f"{where}, which is still reserved; refusing to adopt it.") + issue, error = checkout_identity_issue( + task["repository_path"], retained["path"], retained["branch"] + ) + if issue is not None: + raise WorktreeError( + f"{where}, but its checkout no longer matches the record ({issue}" + + (f": {error}" if error else "") + + "). Inspect it manually; refusing to adopt it." + ) + + transfer_worktree(conn, retained["id"], claim_id) + audit.record( + conn, + "worktree.continued", + "worktree", + retained["id"], + agent=claim["agent"], + detail={ + "path": retained["path"], + "branch": retained["branch"], + "from_claim_id": retained["claim_id"], + "to_claim_id": claim_id, + "from_release_reason": retained["release_reason"], + }, + ) + return { + "id": retained["id"], + "path": retained["path"], + "branch": retained["branch"], + "continued_from_claim_id": retained["claim_id"], + } + + def transfer_worktree(conn: Connection, worktree_id: int, new_claim_id: int) -> None: """Reassign a worktree to a new claim (handoff continuation).""" conn.execute( @@ -203,6 +322,30 @@ def cleanup_released_worktree( return True +def checkout_identity_issue( + repo_path: str | Path, wt_path: str | Path, branch: str +) -> tuple[str | None, str | None]: + """Compare an existing checkout with its recorded repository and branch. + + Returns (issue, error): issue is None when the checkout is the recorded + repository's worktree with the recorded branch checked out. + """ + try: + expected_common = _git_common_dir(repo_path) + actual_common = _git_common_dir(wt_path) + actual_root = Path(_git(wt_path, "rev-parse", "--show-toplevel")).resolve() + actual_branch = _git(wt_path, "rev-parse", "--symbolic-full-name", "HEAD") + except WorktreeError as exc: + return "unverifiable_checkout", str(exc) + if actual_common != expected_common or actual_root != Path(wt_path).resolve(): + return "repository_mismatch", None + if actual_branch == "HEAD": + return "detached_head", None + if actual_branch != f"refs/heads/{branch}": + return "branch_mismatch", None + return None, None + + def doctor_worktrees(conn: Connection, repair: bool = False) -> list[dict]: """Report and conservatively repair drift in recorded managed worktrees. @@ -243,27 +386,9 @@ def doctor_worktrees(conn: Connection, repair: bool = False) -> list[dict]: issue: str | None = None identity_error = None if exists: - try: - expected_common = _git_common_dir(repo_path) - actual_common = _git_common_dir(wt["path"]) - actual_root = Path( - _git(wt["path"], "rev-parse", "--show-toplevel") - ).resolve() - actual_branch = _git( - wt["path"], "rev-parse", "--symbolic-full-name", "HEAD" - ) - if ( - actual_common != expected_common - or actual_root != Path(wt["path"]).resolve() - ): - issue = "repository_mismatch" - elif actual_branch == "HEAD": - issue = "detached_head" - elif actual_branch != f"refs/heads/{wt['branch']}": - issue = "branch_mismatch" - except WorktreeError as exc: - issue = "unverifiable_checkout" - identity_error = str(exc) + issue, identity_error = checkout_identity_issue( + repo_path, wt["path"], wt["branch"] + ) if issue is not None: pass elif wt["removed_at"] is None and not exists: diff --git a/tests/conftest.py b/tests/conftest.py index 4d24a8d..da11c4b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,14 +7,19 @@ import os import subprocess +import uuid from collections.abc import Iterator from pathlib import Path import pytest -from quorumgit import store +from quorumgit import gate, registry, store from quorumgit.config import Config +# The shared store's designated approver. Workers request approvals; this +# operator votes on them, so no test relies on self-authorization. +OPERATOR = "operator" + @pytest.fixture(scope="session") def cfg(tmp_path_factory) -> Iterator[Config]: @@ -90,3 +95,36 @@ def git(*args): @pytest.fixture() def git_repo(tmp_path) -> Path: return make_git_repo(tmp_path / "repo") + + +def ensure_agent(conn, name: str, role: str | None = None) -> None: + """Register an agent if missing; with a role, also (re)assign it.""" + conn.execute( + "INSERT INTO agents (name) VALUES (?) ON CONFLICT DO NOTHING", (name,) + ) + if role is not None: + conn.execute("UPDATE agents SET role = ? WHERE name = ?", (role, name)) + + +def register_repo(conn, path: Path, prefix: str = "repo") -> str: + """Register a fresh real Git repository under a unique name.""" + name = f"{prefix}-{uuid.uuid4().hex[:8]}" + registry.add_repository(conn, name, make_git_repo(path)) + return name + + +def approve(conn, operation: dict, requested_by: str, voters=(OPERATOR,)) -> dict: + """Request an exact operation as one agent and approve it as operators.""" + ensure_agent(conn, requested_by) + for voter in voters: + ensure_agent(conn, voter, "operator") + approval = gate.request_approval(conn, operation, requested_by=requested_by) + for voter in voters: + approval = gate.vote(conn, approval["id"], voter, True) + return approval + + +@pytest.fixture() +def approval_repo(conn, tmp_path) -> str: + """A registered repository name for approval operations.""" + return register_repo(conn, tmp_path / "approval-repo", prefix="approval") diff --git a/tests/test_acceptance.py b/tests/test_acceptance.py index 49c95ca..7642460 100644 --- a/tests/test_acceptance.py +++ b/tests/test_acceptance.py @@ -27,7 +27,7 @@ import pytest from quorumgit import gate, handoff, registry, store, trees, work -from tests.conftest import make_git_repo +from tests.conftest import OPERATOR, ensure_agent, make_git_repo def test_full_workflow(committed_conn, cfg, tmp_path, initialized_store): @@ -41,7 +41,7 @@ def test_full_workflow(committed_conn, cfg, tmp_path, initialized_store): agent_a, agent_b = f"a-{suffix}", f"b-{suffix}" registry.add_agent(conn, agent_a) registry.add_agent(conn, agent_b) - conn.execute("INSERT INTO agents (name) VALUES ('operator') ON CONFLICT DO NOTHING") + ensure_agent(conn, OPERATOR, "operator") task_a = work.create_task( conn, repo_name, "implement feature", objective="build the feature in src/" @@ -152,8 +152,10 @@ def test_full_workflow(committed_conn, cfg, tmp_path, initialized_store): scope_globs=["src/**"], ) takeover_op["from_claim_id"] = holder["id"] - approval = gate.request_approval(conn, takeover_op, requested_by="operator") - gate.vote(conn, approval["id"], "operator", True) + approval = gate.request_approval(conn, takeover_op, requested_by=agent_a) + with pytest.raises(gate.GateError, match="Vote refused"): + gate.vote(conn, approval["id"], agent_a, True) + gate.vote(conn, approval["id"], OPERATOR, True) assert gate.is_approved(conn, takeover_op) gate.consume_approval(conn, approval["id"], takeover_op, agent=agent_a) claim_back, _, _ = work.claim_task( diff --git a/tests/test_approval_authority.py b/tests/test_approval_authority.py new file mode 100644 index 0000000..82cbf2e --- /dev/null +++ b/tests/test_approval_authority.py @@ -0,0 +1,388 @@ +"""Approval authority: who may authorize is owned by the repository. + +A worker cannot authorize its own operation: the requester does not choose the +quorum, only roles the repository names may vote, the requester may not vote +unless the policy allows it, and an agent that approved an operation can never +be the one that carries it out. Authority is re-derived at use time, so a +tightened policy or a demoted approver invalidates an earlier approval. +""" + +from __future__ import annotations + +import subprocess +import uuid +from pathlib import Path + +import pytest + +from quorumgit import gate, registry, store, work +from quorumgit.config import Config +from quorumgit.registry import RegistryError +from tests.conftest import ( + OPERATOR, + approve, + ensure_agent, + make_git_repo, + register_repo, +) +from tests.test_authorization_binding import _apply_001 +from tests.test_cli_hub import _cli +from tests.test_gate import _commit, _push, _setup + + +def _op(repository: str, **extra) -> dict: + return {"type": "protected_ref_update", "repository": repository, + "nonce": uuid.uuid4().hex, **extra} + + +# ------------------------------------------------------------- voting rules + + +def test_worker_cannot_approve_its_own_request(conn, approval_repo): + ensure_agent(conn, "self-worker") + approval = gate.request_approval(conn, _op(approval_repo), "self-worker") + + with pytest.raises(gate.GateError, match="is a worker"): + gate.vote(conn, approval["id"], "self-worker", True) + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "pending" + + +def test_operator_requester_cannot_vote_by_default(conn, approval_repo): + ensure_agent(conn, "requesting-op", "operator") + approval = gate.request_approval(conn, _op(approval_repo), "requesting-op") + + with pytest.raises(gate.GateError, match="requested this approval"): + gate.vote(conn, approval["id"], "requesting-op", True) + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "pending" + + +def test_policy_can_allow_requester_to_vote(conn, approval_repo): + ensure_agent(conn, OPERATOR, "operator") + ensure_agent(conn, "requesting-op", "operator") + registry.set_approval_policy( + conn, approval_repo, actor=OPERATOR, requester_may_vote=True + ) + approval = gate.request_approval(conn, _op(approval_repo), "requesting-op") + result = gate.vote(conn, approval["id"], "requesting-op", True) + assert result["status"] == "approved" + + +def test_non_approving_role_cannot_deny_either(conn, approval_repo): + ensure_agent(conn, "req") + ensure_agent(conn, "a-reviewer", "reviewer") + approval = gate.request_approval(conn, _op(approval_repo), "req") + with pytest.raises(gate.GateError, match="accepts votes only from: operator"): + gate.vote(conn, approval["id"], "a-reviewer", False) + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "pending" + + +def test_policy_roles_decide_who_votes(conn, approval_repo): + ensure_agent(conn, OPERATOR, "operator") + ensure_agent(conn, "req") + ensure_agent(conn, "a-reviewer", "reviewer") + registry.set_approval_policy( + conn, approval_repo, actor=OPERATOR, roles=["reviewer"] + ) + approval = gate.request_approval(conn, _op(approval_repo), "req") + with pytest.raises(gate.GateError, match="accepts votes only from: reviewer"): + gate.vote(conn, approval["id"], OPERATOR, True) + assert gate.vote(conn, approval["id"], "a-reviewer", True)["status"] == "approved" + + +def test_engine_refuses_ineligible_vote_rows(conn, approval_repo): + """The vote rule holds even for a writer that bypasses the gate module.""" + ensure_agent(conn, "raw-worker") + approval = gate.request_approval(conn, _op(approval_repo), "raw-worker") + worker_id = registry.get_agent(conn, "raw-worker")["id"] + with pytest.raises(ValueError, match="not eligible"): + conn.execute( + "INSERT INTO votes (approval_id, voter, vote, voter_agent_id) " + "VALUES (?, 'raw-worker', 1, ?)", + (approval["id"], worker_id), + ) + + +def test_approval_must_name_a_registered_repository(conn): + ensure_agent(conn, "req") + with pytest.raises(gate.GateError, match="registered repository"): + gate.request_approval(conn, _op("no-such-repo"), "req") + + +# -------------------------------------------------------------- threshold + + +def test_threshold_comes_from_repository_policy(conn, approval_repo): + ensure_agent(conn, OPERATOR, "operator") + ensure_agent(conn, "second-op", "operator") + ensure_agent(conn, "req") + registry.set_approval_policy(conn, approval_repo, actor=OPERATOR, threshold=2) + approval = gate.request_approval(conn, _op(approval_repo), "req") + assert approval["threshold"] == 2 + assert gate.vote(conn, approval["id"], OPERATOR, True)["status"] == "pending" + assert gate.vote(conn, approval["id"], "second-op", True)["status"] == "approved" + + +def test_cli_requester_cannot_choose_threshold(committed_conn, cfg, tmp_path): + repo = register_repo(committed_conn, tmp_path / "cli-threshold") + ensure_agent(committed_conn, "cli-req") + committed_conn.commit() + result = _cli( + cfg, "approve", "request", f'{{"type":"x","repository":"{repo}"}}', + "--threshold", "1", agent="cli-req", + ) + assert result.returncode == 2 + assert "--threshold" in result.stderr + + +def test_tightened_policy_invalidates_existing_approval(conn, approval_repo): + op = _op(approval_repo) + approval = approve(conn, op, requested_by="req") + ensure_agent(conn, "pusher") + assert gate.is_approved(conn, op) + + registry.set_approval_policy(conn, approval_repo, actor=OPERATOR, threshold=2) + assert not gate.is_approved(conn, op) + with pytest.raises(gate.GateError, match="now requires 2"): + gate.consume_approval(conn, approval["id"], op, agent="pusher") + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "approved" + + +def test_demoted_approver_no_longer_counts(conn, approval_repo): + ensure_agent(conn, OPERATOR, "operator") + ensure_agent(conn, "soon-demoted", "operator") + ensure_agent(conn, "pusher") + op = _op(approval_repo) + approval = approve(conn, op, requested_by="req", voters=("soon-demoted",)) + registry.set_agent_role(conn, "soon-demoted", "worker", actor=OPERATOR) + with pytest.raises(gate.GateError, match="0 eligible"): + gate.consume_approval(conn, approval["id"], op, agent="pusher") + + +# ------------------------------------------------------------ consumption + + +def test_approver_cannot_consume_its_own_approval(conn, approval_repo): + ensure_agent(conn, "req") + op = _op(approval_repo) + approval = approve(conn, op, requested_by="req") + assert gate.approved_instance(conn, op, consumer=OPERATOR) is None + with pytest.raises(gate.GateError, match="may not also carry it out"): + gate.consume_approval(conn, approval["id"], op, agent=OPERATOR) + assert gate.get_approval_by_id(conn, approval["id"])["status"] == "approved" + + +def test_engine_refuses_consumption_by_approver(conn, approval_repo): + op = _op(approval_repo) + approval = approve(conn, op, requested_by="req") + operator_id = registry.get_agent(conn, OPERATOR)["id"] + with pytest.raises(ValueError, match="cannot consume its own approval"): + conn.execute( + "UPDATE approvals SET status = 'consumed', consumed_at = unixepoch(), " + "consumed_by_agent_id = ? WHERE id = ?", + (operator_id, approval["id"]), + ) + + +def test_takeover_beneficiary_cannot_vote(conn, git_repo): + repo = f"beneficiary-{uuid.uuid4().hex[:8]}" + registry.add_repository(conn, repo, git_repo) + ensure_agent(conn, "holder") + ensure_agent(conn, "ambitious-op", "operator") + task = work.create_task(conn, repo, "held") + claim, _, _ = work.claim_task(conn, task, "holder", "feat/held", ["src/**"]) + operation = { + "type": "lease_takeover", + "repository": repo, + "task_id": task, + "from_claim_id": claim, + "from_agent": "holder", + "to_agent": "ambitious-op", + } + ensure_agent(conn, "req") + approval = gate.request_approval(conn, operation, "req") + with pytest.raises(gate.GateError, match="would receive this takeover"): + gate.vote(conn, approval["id"], "ambitious-op", True) + + +def test_pusher_that_approved_is_rejected_by_hook(committed_conn, tmp_path, cfg): + repo_name, hub, clone, a, _b = _setup(committed_conn, tmp_path) + approver = f"approver-{uuid.uuid4().hex[:8]}" + ensure_agent(committed_conn, approver, "operator") + _commit(clone, "approved-by-pusher.txt") + op = { + "type": "protected_ref_update", + "repository": repo_name, + "refname": "refs/heads/main", + "oldrev": subprocess.run( + ["git", "--git-dir", str(hub), "rev-parse", "refs/heads/main"], + capture_output=True, text=True, check=True, + ).stdout.strip(), + "newrev": subprocess.run( + ["git", "-C", str(clone), "rev-parse", "HEAD"], + capture_output=True, text=True, check=True, + ).stdout.strip(), + } + approval = approve(committed_conn, op, requested_by=a, voters=(approver,)) + committed_conn.commit() + + refused = _push(clone, approver, "main", cfg=cfg) + assert refused.returncode != 0 + assert "may not also carry it out" in refused.stderr + assert gate.get_approval_by_id(committed_conn, approval["id"])["status"] == ( + "approved" + ) + + accepted = _push(clone, a, "main", cfg=cfg) + assert accepted.returncode == 0, accepted.stderr + + +# ------------------------------------------------- operator administration + + +@pytest.fixture() +def fresh_store(tmp_path): + local = Config(data_dir=tmp_path / "authority-data", agent=None) + store.migrate(local) + conn = store.connect(local) + try: + yield local, conn + finally: + conn.close() + store.destroy(local) + + +def test_first_operator_is_bootstrapped_then_roles_are_operator_only( + fresh_store, tmp_path +): + local, conn = fresh_store + registry.add_agent(conn, "alice") + registry.add_agent(conn, "mallory") + repo = register_repo(conn, tmp_path / "bootstrap") + + # No operator exists yet, so the first designation needs no authority. + registry.set_agent_role(conn, "alice", "operator", actor=None) + conn.commit() + + with pytest.raises(RegistryError, match="requires an operator"): + registry.set_agent_role(conn, "mallory", "operator", actor="mallory") + conn.rollback() + with pytest.raises(RegistryError, match="requires an operator"): + registry.add_agent(conn, "eve", role="operator", actor=None) + conn.rollback() + with pytest.raises(RegistryError, match="requires an operator"): + registry.set_approval_policy(conn, repo, actor="mallory", threshold=1) + conn.rollback() + + registry.set_agent_role(conn, "mallory", "reviewer", actor="alice") + registry.set_approval_policy(conn, repo, actor="alice", roles=["operator", "reviewer"]) + conn.commit() + assert registry.get_agent(conn, "mallory")["role"] == "reviewer" + policy = registry.approval_policy(conn, registry.get_repository(conn, repo)["id"]) + assert policy["roles"] == ["operator", "reviewer"] + + events = { + row[0] + for row in conn.execute("SELECT event_type FROM audit_events").fetchall() + } + assert {"agent.role_changed", "repository.policy_changed"} <= events + + +def test_last_operator_cannot_be_demoted(fresh_store): + _local, conn = fresh_store + registry.add_agent(conn, "solo") + registry.set_agent_role(conn, "solo", "operator", actor=None) + with pytest.raises(RegistryError, match="last operator"): + registry.set_agent_role(conn, "solo", "worker", actor="solo") + + +def test_cli_agent_roles_and_policy(fresh_store, tmp_path): + local, conn = fresh_store + repo = register_repo(conn, tmp_path / "cli-policy") + conn.commit() + + assert _cli(local, "agent", "add", "boss", "--role", "operator").returncode == 0 + refused = _cli(local, "agent", "add", "sneaky", "--role", "operator", agent="nobody") + assert refused.returncode == 1 + assert _cli(local, "agent", "add", "worker-1").returncode == 0 + promoted = _cli(local, "agent", "role", "worker-1", "reviewer", agent="boss") + assert promoted.returncode == 0, promoted.stderr + + listed = _cli(local, "agent", "list") + assert "boss\toperator" in listed.stdout + assert "worker-1\treviewer" in listed.stdout + + changed = _cli( + local, "repo", "policy", repo, "--threshold", "2", + "--role", "operator", "--role", "reviewer", "--requester-may-vote", + agent="boss", + ) + assert changed.returncode == 0, changed.stderr + shown = _cli(local, "repo", "policy", repo) + assert "approval threshold: 2" in shown.stdout + assert "approving roles: operator, reviewer" in shown.stdout + assert "requester may vote: yes" in shown.stdout + + +# ---------------------------------------------------------------- upgrade + + +def test_upgrade_leaves_no_agent_with_authority(tmp_path): + """Pre-policy stores upgrade with every agent a worker, fail closed.""" + local = Config(data_dir=tmp_path / "upgrade", agent=None) + conn = _apply_001(local) + migration = Path(store.__file__).parent / "migrations" / "002_approval_identities.sql" + for statement in store._migration_statements(migration.read_text(encoding="utf-8")): + conn.execute(statement) + conn.execute( + "INSERT INTO schema_migrations (version) VALUES ('002_approval_identities.sql')" + ) + repo_path = make_git_repo(tmp_path / "legacy-repo") + conn.execute( + "INSERT INTO repositories (name, path) VALUES ('legacy', ?)", (str(repo_path),) + ) + agent_id = conn.execute( + "INSERT INTO agents (name) VALUES ('self-approver') RETURNING id" + ).fetchone()[0] + operation = {"type": "protected_ref_update", "repository": "legacy"} + approval_id = conn.execute( + "INSERT INTO approvals (operation_hash, operation, status, " + "requested_by_agent_id, decided_at) " + "VALUES (?, ?, 'approved', ?, unixepoch()) RETURNING id", + (gate.operation_hash(operation), store.json_dumps(operation), agent_id), + ).fetchone()[0] + conn.execute( + "INSERT INTO votes (approval_id, voter, vote, voter_agent_id) " + "VALUES (?, 'self-approver', 1, ?)", + (approval_id, agent_id), + ) + conn.commit() + conn.close() + + assert store.migrate(local) == ["004_approval_authority.sql"] + upgraded = store.connect(local) + try: + assert registry.get_agent(upgraded, "self-approver")["role"] == "worker" + repo = registry.get_repository(upgraded, "legacy") + assert registry.approval_policy(upgraded, repo["id"]) == { + "threshold": 1, + "requester_may_vote": False, + "roles": ["operator"], + } + backfilled = gate.get_approval_by_id(upgraded, approval_id) + assert backfilled["repository_id"] == repo["id"] + assert not gate.is_approved(upgraded, operation) + upgraded.execute("INSERT INTO agents (name) VALUES ('other')") + with pytest.raises(gate.GateError, match="0 eligible"): + gate.consume_approval(upgraded, approval_id, operation, agent="other") + finally: + upgraded.rollback() + upgraded.close() + store.destroy(local) + + +def test_contract_requires_governance_triggers(fresh_store): + local, conn = fresh_store + conn.execute("DROP TRIGGER votes_require_eligible_voter") + conn.commit() + with pytest.raises(store.ContractViolation, match="votes_require_eligible_voter"): + store.verify_contract(local) diff --git a/tests/test_authorization_binding.py b/tests/test_authorization_binding.py index 4212f72..5d12453 100644 --- a/tests/test_authorization_binding.py +++ b/tests/test_authorization_binding.py @@ -12,7 +12,7 @@ from quorumgit import gate, registry, store, work from quorumgit.config import Config from quorumgit.registry import RegistryError -from tests.conftest import make_git_repo +from tests.conftest import approve, ensure_agent, make_git_repo, register_repo from tests.test_cli_hub import _cli from tests.test_gate import _commit, _push, _setup @@ -35,6 +35,12 @@ def _apply_001(cfg: Config): return conn +UPGRADE_MIGRATIONS = [ + "002_approval_identities.sql", + "004_approval_authority.sql", +] + + def _register_001_agent(conn, name: str) -> int: row = conn.execute( "INSERT INTO agents (name) VALUES (?) RETURNING id", (name,) @@ -77,17 +83,17 @@ def _insert_001_vote(conn, approval_id: int, voter: str) -> None: ) -def test_approval_requester_must_be_registered(conn): - operation = {"type": "test", "repository": "r"} +def test_approval_requester_must_be_registered(conn, approval_repo): + operation = {"type": "test", "repository": approval_repo} before = conn.execute("SELECT count(*) FROM approvals").fetchone()[0] with pytest.raises(RegistryError, match="not registered"): gate.request_approval(conn, operation, requested_by="ghost") assert conn.execute("SELECT count(*) FROM approvals").fetchone()[0] == before -def test_approval_voter_must_be_registered(conn): +def test_approval_voter_must_be_registered(conn, approval_repo): _agents(conn, "requester") - operation = {"type": "test", "repository": "r"} + operation = {"type": "test", "repository": approval_repo} approval = gate.request_approval(conn, operation, requested_by="requester") before = conn.execute("SELECT count(*) FROM votes").fetchone()[0] @@ -97,9 +103,10 @@ def test_approval_voter_must_be_registered(conn): assert conn.execute("SELECT count(*) FROM votes").fetchone()[0] == before -def test_approval_consumer_must_be_registered(conn): - _agents(conn, "requester", "voter") - operation = {"type": "test", "repository": "r"} +def test_approval_consumer_must_be_registered(conn, approval_repo): + _agents(conn, "requester") + ensure_agent(conn, "voter", "operator") + operation = {"type": "test", "repository": approval_repo} approval = gate.request_approval(conn, operation, requested_by="requester") gate.vote(conn, approval["id"], "voter", True) @@ -108,10 +115,13 @@ def test_approval_consumer_must_be_registered(conn): assert gate.get_approval_by_id(conn, approval["id"])["status"] == "approved" -def test_approval_consumer_must_present_matching_instance_and_operation(conn): - _agents(conn, "requester", "voter", "consumer") - first_operation = {"type": "first", "repository": "r"} - second_operation = {"type": "second", "repository": "r"} +def test_approval_consumer_must_present_matching_instance_and_operation( + conn, approval_repo +): + _agents(conn, "requester", "consumer") + ensure_agent(conn, "voter", "operator") + first_operation = {"type": "first", "repository": approval_repo} + second_operation = {"type": "second", "repository": approval_repo} first = gate.request_approval(conn, first_operation, requested_by="requester") second = gate.request_approval(conn, second_operation, requested_by="requester") gate.vote(conn, first["id"], "voter", True) @@ -123,9 +133,11 @@ def test_approval_consumer_must_present_matching_instance_and_operation(conn): assert gate.get_approval_by_id(conn, second["id"])["status"] == "approved" -def test_delayed_vote_cannot_target_new_approval_instance(conn): - _agents(conn, "requester", "first-voter", "late-voter") - operation = {"type": "repeat", "repository": "r"} +def test_delayed_vote_cannot_target_new_approval_instance(conn, approval_repo): + _agents(conn, "requester") + ensure_agent(conn, "first-voter", "operator") + ensure_agent(conn, "late-voter", "operator") + operation = {"type": "repeat", "repository": approval_repo} first = gate.request_approval(conn, operation, requested_by="requester") gate.vote(conn, first["id"], "first-voter", False) second = gate.request_approval(conn, operation, requested_by="requester") @@ -135,10 +147,12 @@ def test_delayed_vote_cannot_target_new_approval_instance(conn): assert gate.get_approval_by_id(conn, second["id"])["status"] == "pending" -def test_cli_votes_by_approval_instance_id(committed_conn, cfg): - _agents(committed_conn, "cli-requester", "cli-voter") +def test_cli_votes_by_approval_instance_id(committed_conn, cfg, tmp_path): + _agents(committed_conn, "cli-requester") + ensure_agent(committed_conn, "cli-voter", "operator") + repository = register_repo(committed_conn, tmp_path / "cli", prefix="cli") committed_conn.commit() - operation = {"type": "cli", "repository": "r", "nonce": uuid.uuid4().hex} + operation = {"type": "cli", "repository": repository, "nonce": uuid.uuid4().hex} requested = _cli( cfg, "approve", @@ -149,6 +163,7 @@ def test_cli_votes_by_approval_instance_id(committed_conn, cfg): assert requested.returncode == 0, requested.stderr match = re.search(r"approval (\d+) hash=", requested.stdout) assert match is not None + assert "threshold=1" in requested.stdout voted = _cli( cfg, @@ -161,16 +176,18 @@ def test_cli_votes_by_approval_instance_id(committed_conn, cfg): assert "status=approved" in voted.stdout -def test_new_approval_rows_reference_registered_agents(conn): - _agents(conn, "requester", "voter", "consumer") - operation = {"type": "identity-columns", "repository": "r"} +def test_new_approval_rows_reference_registered_agents(conn, approval_repo): + _agents(conn, "requester", "consumer") + ensure_agent(conn, "voter", "operator") + operation = {"type": "identity-columns", "repository": approval_repo} approval = gate.request_approval(conn, operation, requested_by="requester") gate.vote(conn, approval["id"], "voter", True) gate.consume_approval(conn, approval["id"], operation, agent="consumer") row = conn.execute( """ - SELECT a.requested_by_agent_id, a.consumed_by_agent_id, v.voter_agent_id + SELECT a.requested_by_agent_id, a.consumed_by_agent_id, v.voter_agent_id, + a.repository_id FROM approvals a JOIN votes v ON v.approval_id = a.id WHERE a.id = ? """, @@ -185,13 +202,16 @@ 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', '004_approval_authority.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) == UPGRADE_MIGRATIONS store.verify_contract(cfg) upgraded = store.connect(cfg) upgraded.close() @@ -208,7 +228,7 @@ 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) == UPGRADE_MIGRATIONS migrated = store.connect(cfg) try: requester = migrated.execute( @@ -229,7 +249,7 @@ def test_identity_migration_preserves_and_backfills_history(tmp_path): assert store.json_loads(invalidation[0], {})["reason"] == ( "unregistered historical voter" ) - with pytest.raises(ValueError, match="registered agent"): + with pytest.raises(ValueError, match="registered (agent|repository)"): migrated.execute( "INSERT INTO approvals (operation_hash, operation) VALUES (?, ?)", ("sha256:" + "1" * 64, store.json_dumps(operation)), @@ -248,7 +268,7 @@ 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) == UPGRADE_MIGRATIONS migrated = store.connect(cfg) try: row = migrated.execute( @@ -280,7 +300,7 @@ 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) == UPGRADE_MIGRATIONS migrated = store.connect(cfg) try: approval = migrated.execute( @@ -333,8 +353,7 @@ def test_stale_takeover_approval_cannot_displace_reacquired_claim( "from_agent": owner, "to_agent": successor, } - approval = gate.request_approval(conn, operation, requested_by=operator) - gate.vote(conn, approval["id"], operator, True) + approval = approve(conn, operation, requested_by=successor, voters=(operator,)) work.release_claim(conn, old_claim, owner) new_claim, _, _ = work.claim_task( conn, task, owner, "feat/new", ["src/**"] diff --git a/tests/test_gate.py b/tests/test_gate.py index 849c426..00f6c7e 100644 --- a/tests/test_gate.py +++ b/tests/test_gate.py @@ -13,7 +13,7 @@ import pytest from quorumgit import gate, registry, store, work -from tests.conftest import make_git_repo +from tests.conftest import OPERATOR, approve, ensure_agent, make_git_repo, register_repo def _register_agents(conn, *names): @@ -100,30 +100,34 @@ def test_operation_hash_requires_fields(): gate.operation_hash({"type": "x"}) -def test_vote_threshold_and_deny(conn): - _register_agents(conn, "op", "voter-1", "voter-2") - op = {"type": "test_op", "repository": "r", "n": 1} - approval = gate.request_approval(conn, op, requested_by="op", threshold=2) +def test_vote_threshold_and_deny(conn, approval_repo): + _register_agents(conn, "op") + ensure_agent(conn, OPERATOR, "operator") + ensure_agent(conn, "voter-1", "operator") + ensure_agent(conn, "voter-2", "operator") + registry.set_approval_policy(conn, approval_repo, actor=OPERATOR, threshold=2) + op = {"type": "test_op", "repository": approval_repo, "n": 1} + approval = gate.request_approval(conn, op, requested_by="op") assert approval["status"] == "pending" + assert approval["threshold"] == 2 gate.vote(conn, approval["id"], "voter-1", True) assert gate.get_approval(conn, approval["operation_hash"])["status"] == "pending" result = gate.vote(conn, approval["id"], "voter-2", True) assert result["status"] == "approved" assert gate.is_approved(conn, op) - op2 = {"type": "test_op", "repository": "r", "n": 2} + op2 = {"type": "test_op", "repository": approval_repo, "n": 2} approval2 = gate.request_approval(conn, op2, requested_by="op") result2 = gate.vote(conn, approval2["id"], "voter-1", False) assert result2["status"] == "denied" assert not gate.is_approved(conn, op2) -def test_consume_approval_is_single_use(conn): +def test_consume_approval_is_single_use(conn, approval_repo): """One approval instance cannot authorize two operations.""" - op = {"type": "protected_ref_update", "repository": "r", "n": 1} + op = {"type": "protected_ref_update", "repository": approval_repo, "n": 1} _register_agents(conn, "op", "pusher-1", "pusher-2") - approval = gate.request_approval(conn, op, requested_by="op") - gate.vote(conn, approval["id"], "op", True) + approval = approve(conn, op, requested_by="op") assert gate.is_approved(conn, op) gate.consume_approval(conn, approval["id"], op, agent="pusher-1") consumed = gate.get_approval(conn, gate.operation_hash(op)) @@ -134,46 +138,46 @@ def test_consume_approval_is_single_use(conn): gate.consume_approval(conn, approval["id"], op, agent="pusher-2") -def test_consumed_operation_can_be_approved_again(conn): +def test_consumed_operation_can_be_approved_again(conn, approval_repo): """A consumed exact takeover may be requested again as a new instance.""" op = { "type": "repeatable_test_operation", - "repository": "r", + "repository": approval_repo, "task_id": 7, "from_agent": "a", "to_agent": "b", } - _register_agents(conn, "operator", "b") - first = gate.request_approval(conn, op, requested_by="operator") - gate.vote(conn, first["id"], "operator", True) + _register_agents(conn, "b") + first = approve(conn, op, requested_by="b") gate.consume_approval(conn, first["id"], op, agent="b") assert gate.get_approval(conn, gate.operation_hash(op))["status"] == "consumed" - second = gate.request_approval(conn, op, requested_by="operator") + second = gate.request_approval(conn, op, requested_by="b") assert second["id"] != first["id"] assert second["status"] == "pending" - gate.vote(conn, second["id"], "operator", True) + gate.vote(conn, second["id"], OPERATOR, True) assert gate.is_approved(conn, op) -def test_consume_approval_concurrent_single_winner(initialized_store): +def test_consume_approval_concurrent_single_winner(initialized_store, tmp_path): """Two real libSQL connections race to consume one approval; one wins. The second consumer is already in flight and blocked on SQLite's writer reservation before the first commits. This preserves the original genuine two-connection race while asserting BEGIN IMMEDIATE semantics. """ - op = { - "type": "protected_ref_update", - "repository": "race-repo", - "n": uuid.uuid4().hex, - } setup = store.connect(initialized_store) - _register_agents(setup, "op", "pusher-1", "pusher-2") - approval = gate.request_approval(setup, op, requested_by="op") - gate.vote(setup, approval["id"], "op", True) - setup.commit() - setup.close() + try: + op = { + "type": "protected_ref_update", + "repository": register_repo(setup, tmp_path / "race", prefix="race"), + "n": uuid.uuid4().hex, + } + _register_agents(setup, "op", "pusher-1", "pusher-2") + approval = approve(setup, op, requested_by="op") + setup.commit() + finally: + setup.close() conn1 = store.connect(initialized_store) racer_error: list[Exception] = [] @@ -244,9 +248,7 @@ def test_hook_protected_ref_requires_approval(committed_conn, tmp_path, cfg): "oldrev": oldrev, "newrev": newrev, } - _register_agents(committed_conn, "operator") - approval = gate.request_approval(committed_conn, op, requested_by="operator") - gate.vote(committed_conn, approval["id"], "operator", True) + approve(committed_conn, op, requested_by=a) committed_conn.commit() result = _push(clone, a, "main", cfg=cfg) diff --git a/tests/test_review_fixes.py b/tests/test_review_fixes.py index 17a9e20..2bde1d9 100644 --- a/tests/test_review_fixes.py +++ b/tests/test_review_fixes.py @@ -19,7 +19,7 @@ from quorumgit import gate, handoff, registry, store, trees, work from quorumgit.registry import RegistryError -from tests.conftest import make_git_repo +from tests.conftest import OPERATOR, approve, ensure_agent, make_git_repo, register_repo from tests.test_cli_hub import _cli from tests.test_gate import _commit, _push, _setup from tests.test_handoff import _handoff_record, _head @@ -28,15 +28,18 @@ # ------------------------------------------------------------ finding 1 -def _pending_approval(conn, threshold=1): +def _pending_approval(conn, tmp_path, threshold=1): + repo_name = register_repo(conn, tmp_path / "vote-race", prefix="vote-race") op = { "type": "protected_ref_update", - "repository": "vote-race", + "repository": repo_name, "n": uuid.uuid4().hex, } - for name in ("op", "voter-1", "voter-2"): - conn.execute("INSERT INTO agents (name) VALUES (?) ON CONFLICT DO NOTHING", (name,)) - gate.request_approval(conn, op, requested_by="op", threshold=threshold) + ensure_agent(conn, "op") + for name in (OPERATOR, "voter-1", "voter-2"): + ensure_agent(conn, name, "operator") + registry.set_approval_policy(conn, repo_name, actor=OPERATOR, threshold=threshold) + gate.request_approval(conn, op, requested_by="op") return op @@ -78,10 +81,10 @@ def racer(): conn2.close() -def test_concurrent_no_and_yes_denial_stands(initialized_store): +def test_concurrent_no_and_yes_denial_stands(initialized_store, tmp_path): """A concurrent yes vote cannot override a denial.""" setup = store.connect(initialized_store) - op = _pending_approval(setup, threshold=1) + op = _pending_approval(setup, tmp_path, threshold=1) setup.commit() setup.close() approval, second_error = _race_votes( @@ -91,10 +94,10 @@ def test_concurrent_no_and_yes_denial_stands(initialized_store): assert second_error, "yes vote on a denied approval must raise" -def test_concurrent_yes_votes_satisfy_threshold(initialized_store): +def test_concurrent_yes_votes_satisfy_threshold(initialized_store, tmp_path): """Two concurrent yes votes on a threshold-2 approval both count.""" setup = store.connect(initialized_store) - op = _pending_approval(setup, threshold=2) + op = _pending_approval(setup, tmp_path, threshold=2) setup.commit() setup.close() approval, second_error = _race_votes( @@ -104,8 +107,8 @@ def test_concurrent_yes_votes_satisfy_threshold(initialized_store): assert approval["status"] == "approved" -def test_denial_is_terminal(conn): - op = _pending_approval(conn) +def test_denial_is_terminal(conn, tmp_path): + op = _pending_approval(conn, tmp_path) op_hash = gate.operation_hash(op) approval_id = gate.get_approval(conn, op_hash)["id"] gate.vote(conn, approval_id, "voter-1", False) @@ -114,8 +117,8 @@ def test_denial_is_terminal(conn): gate.vote(conn, approval_id, "voter-2", True) -def test_duplicate_voter_counts_once(conn): - op = _pending_approval(conn, threshold=2) +def test_duplicate_voter_counts_once(conn, tmp_path): + op = _pending_approval(conn, tmp_path, threshold=2) op_hash = gate.operation_hash(op) approval_id = gate.get_approval(conn, op_hash)["id"] gate.vote(conn, approval_id, "voter-1", True) @@ -147,9 +150,7 @@ def _approve_update(conn, repo_name, hub, clone, refname="refs/heads/main"): "oldrev": oldrev, "newrev": newrev, } - 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) + approve(conn, op, requested_by="op") conn.commit() return op @@ -220,9 +221,7 @@ def test_refused_takeover_preserves_holder_and_approval(committed_conn, tmp_path "from_agent": a, "to_agent": c, } - conn.execute("INSERT INTO agents (name) VALUES ('operator') ON CONFLICT DO NOTHING") - approval = gate.request_approval(conn, operation, requested_by="operator") - gate.vote(conn, approval["id"], "operator", True) + approve(conn, operation, requested_by=c) conn.commit() refused = _cli( diff --git a/tests/test_worktree_continuation.py b/tests/test_worktree_continuation.py new file mode 100644 index 0000000..e6c5945 --- /dev/null +++ b/tests/test_worktree_continuation.py @@ -0,0 +1,202 @@ +"""A superseding claim continues the retained checkout instead of colliding. + +Git refuses to check one branch out twice. When a claim on the same task and +branch follows lease expiry, an approved takeover, or a release that kept its +worktree, the retained checkout (including uncommitted work) is verified and +transferred rather than duplicated. Managed paths never contain names. +""" + +from __future__ import annotations + +import re +import subprocess +import uuid +from pathlib import Path + +from quorumgit import audit, registry, trees, work +from tests.conftest import approve, ensure_agent, make_git_repo +from tests.test_cli_hub import _cli + + +def _repo_and_agents(conn, tmp_path, *agents): + suffix = uuid.uuid4().hex[:8] + repo = f"continue-{suffix}" + registry.add_repository(conn, repo, make_git_repo(tmp_path / "repo")) + names = [f"{agent}-{suffix}" for agent in agents] + for name in names: + ensure_agent(conn, name) + return repo, names + + +def _cli_claim(cfg, task, agent, branch, *extra): + return _cli( + cfg, "claim", str(task), "--branch", branch, "--scope", "src/**", *extra, + agent=agent, + ) + + +def _claimed_worktree(result) -> Path: + match = re.search(r"^worktree: (.+)$", result.stdout, re.MULTILINE) + assert match is not None, result.stdout + result.stderr + return Path(match.group(1)) + + +def _expire(conn, task): + conn.execute( + "UPDATE claims SET lease_expires_at = unixepoch() - 1 " + "WHERE task_id = ? AND released_at IS NULL", + (task,), + ) + + +def _events(conn, event_type): + return [ + event + for event in audit.events(conn, entity="worktree", limit=1000) + if event["event_type"] == event_type + ] + + +def test_reclaim_after_expiry_continues_retained_worktree( + committed_conn, cfg, tmp_path +): + conn = committed_conn + repo, (first, second) = _repo_and_agents(conn, tmp_path, "first", "second") + task = work.create_task(conn, repo, "expiring work") + conn.commit() + + claimed = _cli_claim(cfg, task, first, "feat/expiring") + assert claimed.returncode == 0, claimed.stderr + path = _claimed_worktree(claimed) + (path / "src" / "unfinished.py").write_text("work in progress\n") + + _expire(conn, task) + conn.commit() + reclaimed = _cli_claim(cfg, task, second, "feat/expiring") + assert reclaimed.returncode == 0, reclaimed.stderr + assert _claimed_worktree(reclaimed) == path + assert "continued retained worktree" in reclaimed.stdout + assert (path / "src" / "unfinished.py").read_text() == "work in progress\n" + + holder = work.active_claim_for_task(conn, task) + assert holder is not None and holder["agent"] == second + wt = trees.worktree_for_claim(conn, holder["id"]) + assert wt is not None and wt["path"] == str(path) + continued = [ + e for e in _events(conn, "worktree.continued") + if e["detail"]["to_claim_id"] == holder["id"] + ] + assert continued and continued[0]["detail"]["from_release_reason"] == "lease_expired" + + +def test_governed_takeover_continues_incumbent_worktree( + committed_conn, cfg, tmp_path +): + conn = committed_conn + repo, (holder, taker, requester) = _repo_and_agents( + conn, tmp_path, "holder", "taker", "requester" + ) + task = work.create_task(conn, repo, "taken work") + conn.commit() + claimed = _cli_claim(cfg, task, holder, "feat/taken") + assert claimed.returncode == 0, claimed.stderr + path = _claimed_worktree(claimed) + + incumbent = work.active_claim_for_task(conn, task) + assert incumbent is not None + approve( + conn, + { + "type": "lease_takeover", + "repository": repo, + "task_id": task, + "from_claim_id": incumbent["id"], + "from_agent": holder, + "to_agent": taker, + }, + requested_by=requester, + ) + conn.commit() + + taken = _cli_claim(cfg, task, taker, "feat/taken", "--takeover") + assert taken.returncode == 0, taken.stderr + assert _claimed_worktree(taken) == path + new_holder = work.active_claim_for_task(conn, task) + assert new_holder is not None and new_holder["agent"] == taker + + +def test_reclaim_after_release_keeping_worktree(committed_conn, cfg, tmp_path): + conn = committed_conn + repo, (owner,) = _repo_and_agents(conn, tmp_path, "owner") + task = work.create_task(conn, repo, "paused work") + conn.commit() + claimed = _cli_claim(cfg, task, owner, "feat/paused") + assert claimed.returncode == 0, claimed.stderr + path = _claimed_worktree(claimed) + first_claim = work.active_claim_for_task(conn, task) + assert first_claim is not None + released = _cli(cfg, "release", str(first_claim["id"]), agent=owner) + assert released.returncode == 0, released.stderr + + again = _cli_claim(cfg, task, owner, "feat/paused") + assert again.returncode == 0, again.stderr + assert _claimed_worktree(again) == path + + +def test_other_tasks_retained_worktree_is_not_adopted(committed_conn, cfg, tmp_path): + conn = committed_conn + repo, (a, b) = _repo_and_agents(conn, tmp_path, "a", "b") + task_one = work.create_task(conn, repo, "one") + task_two = work.create_task(conn, repo, "two") + conn.commit() + claimed = _cli_claim(cfg, task_one, a, "feat/shared") + assert claimed.returncode == 0, claimed.stderr + _expire(conn, task_one) + conn.commit() + + refused = _cli_claim(cfg, task_two, b, "feat/shared") + assert refused.returncode == 1 + assert "belongs to another task" in refused.stderr + assert "already checked out" not in refused.stderr + assert work.active_claim_for_task(conn, task_two) is None + + +def test_drifted_retained_worktree_is_not_adopted(committed_conn, cfg, tmp_path): + conn = committed_conn + repo, (a, b) = _repo_and_agents(conn, tmp_path, "a", "b") + task = work.create_task(conn, repo, "drifting") + conn.commit() + claimed = _cli_claim(cfg, task, a, "feat/drift") + assert claimed.returncode == 0, claimed.stderr + path = _claimed_worktree(claimed) + subprocess.run( + ["git", "-C", str(path), "checkout", "-q", "-b", "elsewhere"], + check=True, + capture_output=True, + ) + _expire(conn, task) + conn.commit() + + refused = _cli_claim(cfg, task, b, "feat/drift") + assert refused.returncode == 1 + assert "branch_mismatch" in refused.stderr + expired = conn.execute( + "SELECT count(*) FROM claims WHERE task_id = ? AND released_at IS NULL", + (task,), + ).fetchone()[0] + assert expired == 1, "the refused claim must roll back entirely" + + +def test_managed_paths_never_contain_names(conn, cfg, tmp_path): + hostile = "../../../escaped" + repo = f"../../outside-{uuid.uuid4().hex[:8]}" + registry.add_repository(conn, repo, make_git_repo(tmp_path / "named")) + ensure_agent(conn, hostile) + task = work.create_task(conn, repo, "hostile names") + claim, _, _ = work.claim_task(conn, task, hostile, "feat/names", ["src/**"]) + + wt = trees.create_worktree(conn, claim, cfg.worktrees_dir) + path = Path(wt["path"]) + assert path.is_relative_to(cfg.worktrees_dir.resolve()) + assert "escaped" not in wt["path"] and "outside" not in wt["path"] + assert path.is_dir() From 52438c63f0e63f0ae089070af1a908486200307b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 08:57:20 +0000 Subject: [PATCH 2/7] Close CLI libSQL connections explicitly Every CLI command used libSQL's connection context manager, which ends the transaction but leaves the native connection open, so each one was torn down implicitly when its last reference disappeared. On Windows CI the CLI child process intermittently died with 0xC0000005 and no output, in different commands on each run, as the number of CLI invocations in the suite grew. store.session() keeps the same commit-on-success / rollback-on-error semantics and then closes the connection deterministically. All CLI commands use it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SrukvuPz7kpbore3kkKpQ3 --- src/quorumgit/cli.py | 50 +++++++++++++++++++++--------------------- src/quorumgit/store.py | 20 +++++++++++++++++ tests/test_store.py | 25 +++++++++++++++++++++ 3 files changed, 70 insertions(+), 25 deletions(-) diff --git a/src/quorumgit/cli.py b/src/quorumgit/cli.py index 1ce4992..7d7b477 100644 --- a/src/quorumgit/cli.py +++ b/src/quorumgit/cli.py @@ -52,7 +52,7 @@ def cmd_status(args, cfg) -> int: except store.StoreError as exc: print(f"contract: VIOLATED — {exc}") return 1 - with store.connect(cfg) as conn: + with store.session(cfg) as conn: for table in ("repositories", "agents", "tasks", "claims", "handoffs"): row = conn.execute(f"SELECT count(*) FROM {table}").fetchone() # noqa: S608 — fixed identifier set assert row is not None @@ -71,7 +71,7 @@ def cmd_destroy(args, cfg) -> int: def cmd_doctor(args, cfg) -> int: """Inspect and optionally reconcile recorded managed-worktree drift.""" - with store.connect(cfg) as conn: + with store.session(cfg) as conn: findings = trees.doctor_worktrees(conn, repair=args.repair) if args.repair: conn.commit() @@ -102,7 +102,7 @@ def cmd_doctor(args, cfg) -> int: def cmd_repo_add(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: repo_id = registry.add_repository( conn, args.name, args.path, protected_refs=args.protected_ref ) @@ -112,7 +112,7 @@ def cmd_repo_add(args, cfg) -> int: def cmd_repo_list(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: for repo in registry.list_repositories(conn): refs = ",".join(repo["protected_refs"]) or "-" print(f"{repo['id']}\t{repo['name']}\t{repo['path']}\tprotected:{refs}") @@ -120,7 +120,7 @@ def cmd_repo_list(args, cfg) -> int: def cmd_repo_policy(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: changing = ( args.threshold is not None or args.role @@ -147,7 +147,7 @@ def cmd_repo_policy(args, cfg) -> int: def cmd_agent_add(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: agent_id = registry.add_agent( conn, args.name, @@ -160,7 +160,7 @@ def cmd_agent_add(args, cfg) -> int: def cmd_agent_role(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: registry.set_agent_role( conn, args.name, @@ -173,7 +173,7 @@ def cmd_agent_role(args, cfg) -> int: def cmd_agent_list(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: for agent in registry.list_agents(conn): print(f"{agent['id']}\t{agent['name']}\t{agent['role']}") return 0 @@ -183,7 +183,7 @@ def cmd_agent_list(args, cfg) -> int: def cmd_task_add(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: task_id = work.create_task(conn, args.repo, args.title, args.objective) conn.commit() print(f"task {task_id} created.") @@ -191,7 +191,7 @@ def cmd_task_add(args, cfg) -> int: def cmd_task_list(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: for task in work.list_tasks(conn, repository=args.repo): print(f"{task['id']}\t{task['repository']}\t{task['status']}\t" f"{task['title']}") @@ -200,7 +200,7 @@ def cmd_task_list(args, cfg) -> int: def cmd_claim(args, cfg) -> int: agent = _agent(args, cfg) - with store.connect(cfg) as conn: + with store.session(cfg) as conn: takeover_operation = None takeover_approval = None if args.takeover: @@ -288,7 +288,7 @@ def cmd_claim(args, cfg) -> int: def cmd_renew(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: work.renew_claim(conn, args.claim_id, _agent(args, cfg), lease_hours=args.lease_hours) conn.commit() @@ -298,7 +298,7 @@ def cmd_renew(args, cfg) -> int: def cmd_release(args, cfg) -> int: agent = _agent(args, cfg) - with store.connect(cfg) as conn: + with store.session(cfg) as conn: wt = trees.worktree_for_claim(conn, args.claim_id) if wt and wt["removed_at"] is None and args.remove_worktree: trees.remove_worktree(conn, args.claim_id, agent) @@ -310,7 +310,7 @@ def cmd_release(args, cfg) -> int: def cmd_checkpoint(args, cfg) -> int: agent = _agent(args, cfg) - with store.connect(cfg) as conn: + with store.session(cfg) as conn: commit = args.commit if not commit: wt = trees.worktree_for_claim(conn, args.claim_id) @@ -331,7 +331,7 @@ def cmd_checkpoint(args, cfg) -> int: def cmd_handoff_create(args, cfg) -> int: agent = _agent(args, cfg) - with store.connect(cfg) as conn: + with store.session(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 if not last_commit: @@ -357,7 +357,7 @@ def cmd_handoff_create(args, cfg) -> int: def cmd_handoff_list(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: for h in handoff.list_handoffs(conn, status=args.status): print(f"{h['id']}\ttask {h['task_id']}\t{h['from_agent']} -> " f"{h['to_agent'] or 'anyone'}\t{h['status']}") @@ -365,7 +365,7 @@ def cmd_handoff_list(args, cfg) -> int: def cmd_handoff_show(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: h = handoff.get_handoff(conn, args.handoff_id) print(json.dumps(h, indent=2, default=str)) return 0 @@ -373,7 +373,7 @@ def cmd_handoff_show(args, cfg) -> int: def cmd_handoff_accept(args, cfg) -> int: agent = _agent(args, cfg) - with store.connect(cfg) as conn: + with store.session(cfg) as conn: result = handoff.accept_handoff(conn, args.handoff_id, agent, lease_hours=args.lease_hours) conn.commit() @@ -386,7 +386,7 @@ def cmd_handoff_accept(args, cfg) -> int: def cmd_handoff_decline(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: handoff.decline_handoff(conn, args.handoff_id, _agent(args, cfg)) conn.commit() print(f"handoff {args.handoff_id} declined.") @@ -394,7 +394,7 @@ def cmd_handoff_decline(args, cfg) -> int: def cmd_handoff_cancel(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: handoff.cancel_handoff(conn, args.handoff_id, _agent(args, cfg)) conn.commit() print(f"handoff {args.handoff_id} cancelled.") @@ -412,7 +412,7 @@ def _operation_from_args(args) -> dict: def cmd_approve_request(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: approval = gate.request_approval( conn, _operation_from_args(args), requested_by=_agent(args, cfg) ) @@ -425,7 +425,7 @@ def cmd_approve_request(args, cfg) -> int: def cmd_approve_vote(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: approval = gate.vote(conn, args.approval_id, _agent(args, cfg), approve=not args.deny) conn.commit() @@ -445,7 +445,7 @@ def cmd_approve_hash(args, cfg) -> int: def cmd_hook_install(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: path = gate.install_hook(conn, args.repo) conn.commit() print(f"pre-receive hook installed: {path}") @@ -453,7 +453,7 @@ def cmd_hook_install(args, cfg) -> int: def cmd_hook_pre_receive(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: return gate.run_pre_receive(conn, args.repo, sys.stdin) @@ -461,7 +461,7 @@ def cmd_hook_pre_receive(args, cfg) -> int: def cmd_audit(args, cfg) -> int: - with store.connect(cfg) as conn: + with store.session(cfg) as conn: for event in audit.events(conn, entity=args.entity, entity_id=args.entity_id, limit=args.limit): print(f"{event['id']}\t{event['created_at']}\t{event['event_type']}" diff --git a/src/quorumgit/store.py b/src/quorumgit/store.py index 9acf079..6de246a 100644 --- a/src/quorumgit/store.py +++ b/src/quorumgit/store.py @@ -9,6 +9,8 @@ import json import time +from collections.abc import Iterator +from contextlib import contextmanager from importlib import import_module, resources from pathlib import Path from typing import Any @@ -147,6 +149,24 @@ def connect(cfg: Config) -> Connection: return conn +@contextmanager +def session(cfg: Config) -> Iterator[Connection]: + """One command's connection: commit on success, roll back on error, close. + + libSQL's own context manager ends the transaction but leaves the + connection open, so without this the native connection would be torn down + implicitly whenever its last Python reference disappeared. Closing it + explicitly, on the thread that opened it, keeps that teardown + deterministic. + """ + conn = connect(cfg) + try: + with conn: + yield conn + finally: + conn.close() + + def _is_locked_error(exc: Exception) -> bool: return "locked" in str(exc).lower() or "busy" in str(exc).lower() diff --git a/tests/test_store.py b/tests/test_store.py index b8ac808..0b5498d 100644 --- a/tests/test_store.py +++ b/tests/test_store.py @@ -136,3 +136,28 @@ def test_audit_append_only(conn): ) with pytest.raises(ValueError, match="append-only"): conn.execute("DELETE FROM audit_events WHERE entity = 'probe'") + + +def test_session_commits_rolls_back_and_closes(tmp_path): + """CLI sessions end their transaction and close the native connection.""" + local = Config(data_dir=tmp_path / "session", agent=None) + store.migrate(local) + try: + # A closed libSQL connection panics on any further use, so closure + # is not probed here; commit and rollback outcomes are. + with store.session(local) as conn: + conn.execute("INSERT INTO agents (name) VALUES ('kept')") + + with pytest.raises(RuntimeError): + with store.session(local) as conn: + conn.execute("INSERT INTO agents (name) VALUES ('dropped')") + raise RuntimeError("abort") + + check = store.connect(local) + try: + names = {r[0] for r in check.execute("SELECT name FROM agents").fetchall()} + finally: + check.close() + assert "kept" in names and "dropped" not in names + finally: + store.destroy(local) From 8e55c9c5da03169335f192c5fc6075c2a3a8b597 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 09:04:21 +0000 Subject: [PATCH 3/7] Enable faulthandler in CLI test subprocesses The Windows job intermittently loses a CLI child process to 0xC0000005 with no output, so the failing call is unknown. faulthandler prints the Python stack on a native crash, which the next occurrence will capture in the assertion message. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SrukvuPz7kpbore3kkKpQ3 --- tests/test_cli_hub.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_cli_hub.py b/tests/test_cli_hub.py index 3a601c1..c4b9714 100644 --- a/tests/test_cli_hub.py +++ b/tests/test_cli_hub.py @@ -20,6 +20,9 @@ def _cli(cfg, *args: str, agent: str | None = None): env = { **os.environ, "QUORUMGIT_DATA_DIR": str(cfg.data_dir), + # A native crash (e.g. Windows 0xC0000005) otherwise leaves no output + # at all; faulthandler prints the Python stack that was running. + "PYTHONFAULTHANDLER": "1", } env.pop("QUORUMGIT_AGENT", None) if agent: From c5ba900e4c316179d77235ab857a016f4ea1ce7a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 13:14:18 +0000 Subject: [PATCH 4/7] Never tear down libSQL connections in CLI processes The intermittent Windows 0xC0000005 crashes are a libsql 0.1.11 bug, not QuorumGit code. faulthandler put the crash in Connection.close(), and Valgrind on Linux shows why for every connection teardown, on main too: dropping the inner LibsqlConnection calls sqlite3Close and frees the handle, then the outer Connection's drop calls sqlite3Close again on the freed block ("Invalid read ... inside a block of size 848 free'd by sqlite3Close"). Linux tolerates the stray read; Windows sometimes faults. 0.1.11 is the newest libsql release, so there is no upgrade to take. CLI processes now retain their connections and end through os._exit after flushing output (cli.run, used by the console script and `python -m quorumgit`). store.release() closes normally everywhere else, and when retaining it first rolls back any open transaction so no write lock outlives the command. Valgrind reports zero invalid reads for read and write commands with this change. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SrukvuPz7kpbore3kkKpQ3 --- pyproject.toml | 2 +- src/quorumgit/__main__.py | 4 +-- src/quorumgit/cli.py | 40 +++++++++++++++++++++++++++- src/quorumgit/store.py | 56 +++++++++++++++++++++++++++++++-------- tests/test_store.py | 40 ++++++++++++++++++++++++++++ 5 files changed, 127 insertions(+), 15 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 48ad581..2ba1f3a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,7 +22,7 @@ dependencies = [ dev = ["pytest>=8", "ruff>=0.6", "pyright>=1.1.380"] [project.scripts] -quorumgit = "quorumgit.cli:main" +quorumgit = "quorumgit.cli:run" [tool.setuptools.packages.find] where = ["src"] diff --git a/src/quorumgit/__main__.py b/src/quorumgit/__main__.py index eb53e2f..27cfa48 100644 --- a/src/quorumgit/__main__.py +++ b/src/quorumgit/__main__.py @@ -1,3 +1,3 @@ -from .cli import main +from .cli import run -raise SystemExit(main()) +run() diff --git a/src/quorumgit/cli.py b/src/quorumgit/cli.py index 7d7b477..86ae6d2 100644 --- a/src/quorumgit/cli.py +++ b/src/quorumgit/cli.py @@ -8,7 +8,9 @@ import argparse import json +import os import sys +import traceback from . import __version__, audit, config, gate, handoff, registry, store, trees, work @@ -644,5 +646,41 @@ def main(argv: list[str] | None = None) -> int: return 1 +def _exit_status(code: object) -> int: + """Map a SystemExit code to a process status the way the interpreter does.""" + if code is None: + return 0 + if isinstance(code, int): + return code + print(code, file=sys.stderr) + return 1 + + +def run() -> None: + """Process entry point: run one command, then exit without libSQL teardown. + + libsql 0.1.11 double-closes the SQLite handle whenever a connection is + torn down, which intermittently crashes the process on Windows (see + store._retain_connections). Connections are therefore retained and the + process ends through os._exit once output is flushed. Every command has + committed or rolled back its transaction before it returns. + """ + store.retain_connections_for_process() + status = 1 + try: + status = main() + except SystemExit as exc: + status = _exit_status(exc.code) + except BaseException: + traceback.print_exc() + status = 1 + finally: + try: + sys.stdout.flush() + finally: + sys.stderr.flush() + os._exit(status) + + if __name__ == "__main__": - raise SystemExit(main()) + run() diff --git a/src/quorumgit/store.py b/src/quorumgit/store.py index 6de246a..9c0573b 100644 --- a/src/quorumgit/store.py +++ b/src/quorumgit/store.py @@ -37,6 +37,42 @@ LOCK_RETRY_MAX_SECONDS = 0.05 MIGRATION_SEPARATOR = "-- quorumgit-statement" +# libsql 0.1.11 closes the underlying SQLite handle twice whenever a +# connection is torn down: dropping its inner LibsqlConnection calls +# sqlite3Close and frees the handle, then the outer Connection's drop calls +# sqlite3Close again on the freed memory (Valgrind: "Invalid read ... in +# sqlite3Close ... inside a block free'd by sqlite3Close"). It happens on +# explicit close() and on implicit garbage collection alike. Linux usually +# tolerates the stray read; Windows intermittently fails it with 0xC0000005. +# No newer libsql release exists, so short-lived CLI processes never tear a +# connection down: with retention enabled, release() keeps the connection +# alive and the process ends through os._exit (see cli.run). Every caller has +# committed or rolled back by then, so nothing is lost by skipping close. +_retain_connections = False +_retained: list[Any] = [] + + +def retain_connections_for_process() -> None: + """Keep released connections alive until the process exits (CLI only).""" + global _retain_connections + _retain_connections = True + + +def release(conn: Connection) -> None: + """Close a connection, or retain it for the rest of a CLI process. + + A retained connection never keeps a transaction (or the write lock) open: + anything still uncommitted is rolled back, as close() would have done. + """ + if not _retain_connections: + conn.close() + return + try: + if conn.in_transaction: + conn.rollback() + finally: + _retained.append(conn) + REQUIRED_TABLES = ( "schema_migrations", "repositories", @@ -130,7 +166,7 @@ def open_connection( return conn except Exception as exc: if conn is not None: - conn.close() + release(conn) if isinstance(exc, StoreError): raise raise StoreError(f"Cannot open local libSQL store: {exc}") from exc @@ -144,27 +180,25 @@ def connect(cfg: Config) -> Connection: try: verify_contract(conn) except Exception: - conn.close() + release(conn) raise return conn @contextmanager def session(cfg: Config) -> Iterator[Connection]: - """One command's connection: commit on success, roll back on error, close. + """One command's connection: commit on success, roll back on error, release. - libSQL's own context manager ends the transaction but leaves the - connection open, so without this the native connection would be torn down - implicitly whenever its last Python reference disappeared. Closing it - explicitly, on the thread that opened it, keeps that teardown - deterministic. + libSQL's own context manager ends the transaction but never closes the + connection; release() then closes it or, in a CLI process, retains it (see + _retain_connections for why). """ conn = connect(cfg) try: with conn: yield conn finally: - conn.close() + release(conn) def _is_locked_error(exc: Exception) -> bool: @@ -325,7 +359,7 @@ def migrate(target: Config | str | Path) -> list[str]: raise raise StoreError(f"Migration failed: {exc}") from exc finally: - conn.close() + release(conn) # ------------------------------------------------------------ contract check @@ -404,4 +438,4 @@ def verify_contract(target: Config | Connection | str | Path) -> None: raise StoreError(f"Store contract check failed: {exc}") from exc finally: if owned: - conn.close() + release(conn) diff --git a/tests/test_store.py b/tests/test_store.py index 0b5498d..159abbe 100644 --- a/tests/test_store.py +++ b/tests/test_store.py @@ -2,6 +2,8 @@ from __future__ import annotations +import subprocess +import sys import threading import time from pathlib import Path @@ -161,3 +163,41 @@ def test_session_commits_rolls_back_and_closes(tmp_path): assert "kept" in names and "dropped" not in names finally: store.destroy(local) + + +def test_cli_process_retains_connections_instead_of_closing(tmp_path): + """libsql 0.1.11 double-closes SQLite on teardown; the CLI never tears down. + + Runs in a subprocess because retention is process-wide by design. + """ + local = Config(data_dir=tmp_path / "retain", agent=None) + store.migrate(local) + script = f""" +from pathlib import Path +from quorumgit import store +from quorumgit.config import Config +cfg = Config(data_dir=Path({str(local.data_dir)!r}), agent=None) +store.retain_connections_for_process() +with store.session(cfg) as conn: + conn.execute("INSERT INTO agents (name) VALUES ('retained')") +assert conn.execute("SELECT 1").fetchone()[0] == 1, "connection was closed" +store.begin_immediate(conn) +conn.execute("INSERT INTO agents (name) VALUES ('never-committed')") +store.release(conn) +assert not conn.in_transaction, "retained connection kept its transaction" +import os +os._exit(0) # as cli.run does: never let interpreter shutdown tear conn down +""" + try: + result = subprocess.run( + [sys.executable, "-c", script], capture_output=True, text=True + ) + assert result.returncode == 0, result.stderr + check = store.connect(local) + try: + names = {r[0] for r in check.execute("SELECT name FROM agents").fetchall()} + finally: + check.close() + assert "retained" in names and "never-committed" not in names + finally: + store.destroy(local) From b7b2f0760f7775fc9f73dd9e94894eccc2335081 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 13:14:37 +0000 Subject: [PATCH 5/7] Revert "Never tear down libSQL connections in CLI processes" This reverts commit c5ba900e4c316179d77235ab857a016f4ea1ce7a. --- pyproject.toml | 2 +- src/quorumgit/__main__.py | 4 +-- src/quorumgit/cli.py | 40 +--------------------------- src/quorumgit/store.py | 56 ++++++++------------------------------- tests/test_store.py | 40 ---------------------------- 5 files changed, 15 insertions(+), 127 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 2ba1f3a..48ad581 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,7 +22,7 @@ dependencies = [ dev = ["pytest>=8", "ruff>=0.6", "pyright>=1.1.380"] [project.scripts] -quorumgit = "quorumgit.cli:run" +quorumgit = "quorumgit.cli:main" [tool.setuptools.packages.find] where = ["src"] diff --git a/src/quorumgit/__main__.py b/src/quorumgit/__main__.py index 27cfa48..eb53e2f 100644 --- a/src/quorumgit/__main__.py +++ b/src/quorumgit/__main__.py @@ -1,3 +1,3 @@ -from .cli import run +from .cli import main -run() +raise SystemExit(main()) diff --git a/src/quorumgit/cli.py b/src/quorumgit/cli.py index 86ae6d2..7d7b477 100644 --- a/src/quorumgit/cli.py +++ b/src/quorumgit/cli.py @@ -8,9 +8,7 @@ import argparse import json -import os import sys -import traceback from . import __version__, audit, config, gate, handoff, registry, store, trees, work @@ -646,41 +644,5 @@ def main(argv: list[str] | None = None) -> int: return 1 -def _exit_status(code: object) -> int: - """Map a SystemExit code to a process status the way the interpreter does.""" - if code is None: - return 0 - if isinstance(code, int): - return code - print(code, file=sys.stderr) - return 1 - - -def run() -> None: - """Process entry point: run one command, then exit without libSQL teardown. - - libsql 0.1.11 double-closes the SQLite handle whenever a connection is - torn down, which intermittently crashes the process on Windows (see - store._retain_connections). Connections are therefore retained and the - process ends through os._exit once output is flushed. Every command has - committed or rolled back its transaction before it returns. - """ - store.retain_connections_for_process() - status = 1 - try: - status = main() - except SystemExit as exc: - status = _exit_status(exc.code) - except BaseException: - traceback.print_exc() - status = 1 - finally: - try: - sys.stdout.flush() - finally: - sys.stderr.flush() - os._exit(status) - - if __name__ == "__main__": - run() + raise SystemExit(main()) diff --git a/src/quorumgit/store.py b/src/quorumgit/store.py index 9c0573b..6de246a 100644 --- a/src/quorumgit/store.py +++ b/src/quorumgit/store.py @@ -37,42 +37,6 @@ LOCK_RETRY_MAX_SECONDS = 0.05 MIGRATION_SEPARATOR = "-- quorumgit-statement" -# libsql 0.1.11 closes the underlying SQLite handle twice whenever a -# connection is torn down: dropping its inner LibsqlConnection calls -# sqlite3Close and frees the handle, then the outer Connection's drop calls -# sqlite3Close again on the freed memory (Valgrind: "Invalid read ... in -# sqlite3Close ... inside a block free'd by sqlite3Close"). It happens on -# explicit close() and on implicit garbage collection alike. Linux usually -# tolerates the stray read; Windows intermittently fails it with 0xC0000005. -# No newer libsql release exists, so short-lived CLI processes never tear a -# connection down: with retention enabled, release() keeps the connection -# alive and the process ends through os._exit (see cli.run). Every caller has -# committed or rolled back by then, so nothing is lost by skipping close. -_retain_connections = False -_retained: list[Any] = [] - - -def retain_connections_for_process() -> None: - """Keep released connections alive until the process exits (CLI only).""" - global _retain_connections - _retain_connections = True - - -def release(conn: Connection) -> None: - """Close a connection, or retain it for the rest of a CLI process. - - A retained connection never keeps a transaction (or the write lock) open: - anything still uncommitted is rolled back, as close() would have done. - """ - if not _retain_connections: - conn.close() - return - try: - if conn.in_transaction: - conn.rollback() - finally: - _retained.append(conn) - REQUIRED_TABLES = ( "schema_migrations", "repositories", @@ -166,7 +130,7 @@ def open_connection( return conn except Exception as exc: if conn is not None: - release(conn) + conn.close() if isinstance(exc, StoreError): raise raise StoreError(f"Cannot open local libSQL store: {exc}") from exc @@ -180,25 +144,27 @@ def connect(cfg: Config) -> Connection: try: verify_contract(conn) except Exception: - release(conn) + conn.close() raise return conn @contextmanager def session(cfg: Config) -> Iterator[Connection]: - """One command's connection: commit on success, roll back on error, release. + """One command's connection: commit on success, roll back on error, close. - libSQL's own context manager ends the transaction but never closes the - connection; release() then closes it or, in a CLI process, retains it (see - _retain_connections for why). + libSQL's own context manager ends the transaction but leaves the + connection open, so without this the native connection would be torn down + implicitly whenever its last Python reference disappeared. Closing it + explicitly, on the thread that opened it, keeps that teardown + deterministic. """ conn = connect(cfg) try: with conn: yield conn finally: - release(conn) + conn.close() def _is_locked_error(exc: Exception) -> bool: @@ -359,7 +325,7 @@ def migrate(target: Config | str | Path) -> list[str]: raise raise StoreError(f"Migration failed: {exc}") from exc finally: - release(conn) + conn.close() # ------------------------------------------------------------ contract check @@ -438,4 +404,4 @@ def verify_contract(target: Config | Connection | str | Path) -> None: raise StoreError(f"Store contract check failed: {exc}") from exc finally: if owned: - release(conn) + conn.close() diff --git a/tests/test_store.py b/tests/test_store.py index 159abbe..0b5498d 100644 --- a/tests/test_store.py +++ b/tests/test_store.py @@ -2,8 +2,6 @@ from __future__ import annotations -import subprocess -import sys import threading import time from pathlib import Path @@ -163,41 +161,3 @@ def test_session_commits_rolls_back_and_closes(tmp_path): assert "kept" in names and "dropped" not in names finally: store.destroy(local) - - -def test_cli_process_retains_connections_instead_of_closing(tmp_path): - """libsql 0.1.11 double-closes SQLite on teardown; the CLI never tears down. - - Runs in a subprocess because retention is process-wide by design. - """ - local = Config(data_dir=tmp_path / "retain", agent=None) - store.migrate(local) - script = f""" -from pathlib import Path -from quorumgit import store -from quorumgit.config import Config -cfg = Config(data_dir=Path({str(local.data_dir)!r}), agent=None) -store.retain_connections_for_process() -with store.session(cfg) as conn: - conn.execute("INSERT INTO agents (name) VALUES ('retained')") -assert conn.execute("SELECT 1").fetchone()[0] == 1, "connection was closed" -store.begin_immediate(conn) -conn.execute("INSERT INTO agents (name) VALUES ('never-committed')") -store.release(conn) -assert not conn.in_transaction, "retained connection kept its transaction" -import os -os._exit(0) # as cli.run does: never let interpreter shutdown tear conn down -""" - try: - result = subprocess.run( - [sys.executable, "-c", script], capture_output=True, text=True - ) - assert result.returncode == 0, result.stderr - check = store.connect(local) - try: - names = {r[0] for r in check.execute("SELECT name FROM agents").fetchall()} - finally: - check.close() - assert "retained" in names and "never-committed" not in names - finally: - store.destroy(local) From ef8275c0cc6e05241df6adb63c7aa4120964e5af Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 13:14:37 +0000 Subject: [PATCH 6/7] Revert "Enable faulthandler in CLI test subprocesses" This reverts commit 8e55c9c5da03169335f192c5fc6075c2a3a8b597. --- tests/test_cli_hub.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/test_cli_hub.py b/tests/test_cli_hub.py index c4b9714..3a601c1 100644 --- a/tests/test_cli_hub.py +++ b/tests/test_cli_hub.py @@ -20,9 +20,6 @@ def _cli(cfg, *args: str, agent: str | None = None): env = { **os.environ, "QUORUMGIT_DATA_DIR": str(cfg.data_dir), - # A native crash (e.g. Windows 0xC0000005) otherwise leaves no output - # at all; faulthandler prints the Python stack that was running. - "PYTHONFAULTHANDLER": "1", } env.pop("QUORUMGIT_AGENT", None) if agent: From c9fc806b5ad1e124db7f3c3d42448d6b7c5513eb Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 13:24:08 +0000 Subject: [PATCH 7/7] Fix findings from the pinned ruff 0.16.9 gate main now pins ruff 0.16.9 and gates CI on it. Drop three noqa comments for a rule that is not enabled, sort registry imports, merge a nested with in the session test, and mark an unused unpacked fixture value. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SrukvuPz7kpbore3kkKpQ3 --- src/quorumgit/gate.py | 6 +++--- src/quorumgit/registry.py | 1 - tests/test_approval_authority.py | 2 +- tests/test_store.py | 7 +++---- 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/quorumgit/gate.py b/src/quorumgit/gate.py index c09da9b..26e15d6 100644 --- a/src/quorumgit/gate.py +++ b/src/quorumgit/gate.py @@ -220,7 +220,7 @@ def request_approval( WHERE operation_hash = ? AND status IN ('pending', 'approved') ORDER BY id DESC LIMIT 1 - """, # noqa: S608 -- fixed column list + """, (op_hash,), ).fetchone() if existing is not None: @@ -259,7 +259,7 @@ def get_approval(conn: Connection, op_hash: str) -> dict: WHERE operation_hash = ? ORDER BY id DESC LIMIT 1 - """, # noqa: S608 -- fixed column list + """, (op_hash,), ).fetchone() if row is None: @@ -269,7 +269,7 @@ def get_approval(conn: Connection, op_hash: str) -> dict: def get_approval_by_id(conn: Connection, approval_id: int) -> dict: row = conn.execute( - f"SELECT {_APPROVAL_COLUMNS} FROM approvals WHERE id = ?", # noqa: S608 -- fixed column list + f"SELECT {_APPROVAL_COLUMNS} FROM approvals WHERE id = ?", (approval_id,), ).fetchone() if row is None: diff --git a/src/quorumgit/registry.py b/src/quorumgit/registry.py index 88a1a0d..1dd329b 100644 --- a/src/quorumgit/registry.py +++ b/src/quorumgit/registry.py @@ -8,7 +8,6 @@ from . import audit from .store import Connection, begin_immediate - ROLES = ("worker", "reviewer", "operator") DEFAULT_ROLE = "worker" diff --git a/tests/test_approval_authority.py b/tests/test_approval_authority.py index fb28fed..063b4ce 100644 --- a/tests/test_approval_authority.py +++ b/tests/test_approval_authority.py @@ -255,7 +255,7 @@ def fresh_store(tmp_path): def test_first_operator_is_bootstrapped_then_roles_are_operator_only( fresh_store, tmp_path ): - local, conn = fresh_store + _local, conn = fresh_store registry.add_agent(conn, "alice") registry.add_agent(conn, "mallory") repo = register_repo(conn, tmp_path / "bootstrap") diff --git a/tests/test_store.py b/tests/test_store.py index c59c0f8..4dae32f 100644 --- a/tests/test_store.py +++ b/tests/test_store.py @@ -168,10 +168,9 @@ def test_session_commits_rolls_back_and_closes(tmp_path): with pytest.raises(sqlite3.ProgrammingError, match="closed"): conn.execute("SELECT 1") - with pytest.raises(RuntimeError): - with store.session(local) as conn: - conn.execute("INSERT INTO agents (name) VALUES ('dropped')") - raise RuntimeError("abort") + with pytest.raises(RuntimeError), store.session(local) as conn: + conn.execute("INSERT INTO agents (name) VALUES ('dropped')") + raise RuntimeError("abort") check = store.connect(local) try: