From 97a9abfda13cee53b650a09aafcd681f02b9c9b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 13:21:50 +0000 Subject: [PATCH] Normalize claim scopes and enforce them at push time Ports the scope fixes from PR #10 (review findings 3 and 4) onto the approval-authority work in #12. PR #10's self-approval and worktree reclaim changes are not carried over; #12 covers those. - Scope globs are normalized when a claim is made (./, //, leading /, backslashes, ..) and stored in that form. Overlap compares normalized prefixes case-insensitively. Scopes that escape the repository root are refused. - The pre-receive hook rejects a push to a claimed branch when the new commits (newrev --not --all) touch paths outside the claim's scopes. --cc means a merge commit counts only the paths it changed itself, so merging the base branch in is not blamed on the claim holder. The scope and hook functions are identical to PR #10's; _pushed_paths adds check=False to match main's lint baseline. Tests are PR #10's scope tests, and all 26 fail without the port. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SrukvuPz7kpbore3kkKpQ3 --- README.md | 4 +- src/quorumgit/gate.py | 67 ++++++++++++- src/quorumgit/work.py | 100 +++++++++++++++++-- tests/test_scope_enforcement.py | 171 ++++++++++++++++++++++++++++++++ 4 files changed, 331 insertions(+), 11 deletions(-) create mode 100644 tests/test_scope_enforcement.py diff --git a/README.md b/README.md index d8f95c2..3b87145 100644 --- a/README.md +++ b/README.md @@ -136,7 +136,7 @@ Every claim attempt is classified against all live claims before it is granted, | `CONFLICTING` | branch already claimed by another task | refused | | `BLOCKED` | task already held by an unexpired claim | refused; takeover requires approval | -Scope overlap uses a conservative literal-prefix test (the prefix of one glob up to its first wildcard against the other's) — it errs toward flagging. +Scopes are normalized when declared (`./src/**`, `/src/**`, `src//**` and `src\**` all become `src/**`; scopes that escape the repository root are refused). Scope overlap then uses a conservative literal-prefix test (the prefix of one glob up to its first wildcard against the other's), compared case-insensitively — it errs toward flagging. ## Two deployment models @@ -163,6 +163,8 @@ QUORUMGIT_AGENT=agent-two git push origin feat/x # rejected — branch is cla git push origin feat/x # rejected — unidentified ``` +**Scopes are enforced at push time.** When a branch has a live claim, every commit the push introduces must only touch paths inside that claim's scopes (`**` spans directories, `*` stays within one; a scope without wildcards covers that file or directory). Merge commits count only the paths the merge itself changed, so merging the base branch in is not attributed to the claim holder. In the local model agents commit directly in their worktree, so no hook runs and scopes remain coordination metadata. + The hook learns who is pushing from `QUORUMGIT_AGENT` in its own environment, which Git passes through only when the remote is a **local path** (as `origin` is above). Over SSH or HTTP the variable does not reach the hook, so every push is rejected as unidentified. The hook also opens the store under its own `QUORUMGIT_DATA_DIR`, so pushers must use the same data directory as the rest of the deployment. Accepting a handoff in the hub model prints `worktree: (none — create manually)`: there is no managed worktree to transfer, so the successor fetches the branch into its own clone and continues from the reported commit. diff --git a/src/quorumgit/gate.py b/src/quorumgit/gate.py index c09da9b..a38429d 100644 --- a/src/quorumgit/gate.py +++ b/src/quorumgit/gate.py @@ -29,7 +29,12 @@ get_repository, ) from .store import Connection, begin_immediate, json_dumps, json_loads -from .work import live_claim_for_branch, open_handoff_for_branch +from .work import ( + claim_scopes, + live_claim_for_branch, + open_handoff_for_branch, + path_in_scopes, +) HOOK_MARKER = "# quorumgit-managed-pre-receive v1" @@ -457,6 +462,64 @@ def _is_fast_forward(git_dir: str, oldrev: str, newrev: str) -> bool: return result.returncode == 0 +MAX_REPORTED_PATHS = 10 + + +def _pushed_paths(git_dir: str, newrev: str) -> list[str]: + """Paths changed by the commits this push introduces to the repository. + + `newrev --not --all` is exactly the set of new commits: in pre-receive no + ref points at them yet. `--cc` lists only the paths a merge commit itself + authored (differing from every parent), so merging the base branch in does + not attribute the base's files to the pusher. Renames count as both paths. + """ + result = subprocess.run( + [ + "git", + "--git-dir", + git_dir, + "log", + "--format=", + "--name-only", + "--no-renames", + "--cc", + "-z", + newrev, + "--not", + "--all", + ], + capture_output=True, + check=False, + ) + if result.returncode != 0: + raise PushRejected("Unable to determine the paths changed by this push.") + paths = ( + entry.lstrip("\n") + for entry in result.stdout.decode("utf-8", "surrogateescape").split("\0") + ) + return list(dict.fromkeys(path for path in paths if path)) + + +def enforce_claim_scopes( + conn: Connection, git_dir: str, claim: dict, branch: str, newrev: str +) -> None: + """Reject new commits that touch paths outside the claim's declared scopes.""" + scopes = claim_scopes(conn, claim["id"]) + outside = [ + path for path in _pushed_paths(git_dir, newrev) + if not path_in_scopes(path, scopes) + ] + if outside: + shown = ", ".join(outside[:MAX_REPORTED_PATHS]) + more = len(outside) - MAX_REPORTED_PATHS + if more > 0: + shown += f", and {more} more" + raise PushRejected( + f"Push to {branch!r} changes paths outside claim {claim['id']}'s " + f"scopes {scopes}: {shown}. Claim a scope that covers them first." + ) + + def _invoking_git_common_dir(git_dir: str) -> Path: result = subprocess.run( ["git", "--git-dir", git_dir, "rev-parse", "--git-common-dir"], @@ -520,6 +583,8 @@ def check_ref_update( f"(claim {claim['id']}); pusher is " f"{pusher or 'unidentified — set QUORUMGIT_AGENT'}." ) + if claim and not _is_zero(newrev): + enforce_claim_scopes(conn, git_dir, claim, branch, newrev) protected = refname in repo["protected_refs"] deletion = _is_zero(newrev) diff --git a/src/quorumgit/work.py b/src/quorumgit/work.py index 8df438b..054924a 100644 --- a/src/quorumgit/work.py +++ b/src/quorumgit/work.py @@ -8,6 +8,8 @@ from __future__ import annotations import math +import posixpath +import re import subprocess from typing import Any @@ -191,13 +193,7 @@ def live_claims_in_repository( ).fetchall() result: list[dict] = [] for row in rows: - scopes = [ - r[0] - for r in conn.execute( - "SELECT path_glob FROM scopes WHERE claim_id = ? ORDER BY id", - (row[0],), - ).fetchall() - ] + scopes = claim_scopes(conn, row[0]) result.append( { "id": row[0], @@ -210,6 +206,16 @@ def live_claims_in_repository( return result +def claim_scopes(conn: Connection, claim_id: int) -> list[str]: + return [ + r[0] + for r in conn.execute( + "SELECT path_glob FROM scopes WHERE claim_id = ? ORDER BY id", + (claim_id,), + ).fetchall() + ] + + def open_handoff_for_task(conn: Connection, task_id: int) -> dict | None: """An open handoff reserving this task, if any.""" row = conn.execute( @@ -304,6 +310,30 @@ def live_claim_for_branch( # --------------------------------------------------------- scope comparison +def normalize_scope(glob: str) -> str: + """Canonical repository-relative spelling of a scope glob. + + `./src/**`, `src//**`, `/src/**` and `src\\**` all name `src/**`; a scope + naming the whole repository (`.`, `/`) becomes `**`. Never raises, so it is + safe on historical rows; validate_scope() rejects unusable input. + """ + text = glob.strip().replace("\\", "/") + if not text: + return "" + text = posixpath.normpath(text).lstrip("/") + return "**" if text in ("", ".") else text + + +def validate_scope(glob: str) -> str: + """Normalize a newly declared scope, refusing ones outside the repository.""" + scope = normalize_scope(glob) + if not scope: + raise ClaimRefused("Scopes must be non-empty path globs.") + if scope == ".." or scope.startswith("../"): + raise ClaimRefused(f"Scope {glob!r} escapes the repository root.") + return scope + + def _glob_prefix(glob: str) -> str: """Literal path prefix of a glob (up to the first wildcard character).""" for i, ch in enumerate(glob): @@ -313,11 +343,62 @@ def _glob_prefix(glob: str) -> str: def scopes_overlap(a: str, b: str) -> bool: - """Conservative overlap test that errs toward flagging.""" - pa, pb = _glob_prefix(a), _glob_prefix(b) + """Conservative overlap test that errs toward flagging. + + Both sides are normalized and compared case-insensitively, because a + case-insensitive checkout (macOS, Windows) treats `Src/` and `src/` as one + directory; on case-sensitive systems this only adds conservative flags. + """ + pa = _glob_prefix(normalize_scope(a)).casefold() + pb = _glob_prefix(normalize_scope(b)).casefold() return pa.startswith(pb) or pb.startswith(pa) +def _glob_regex(scope: str) -> re.Pattern[str]: + """Compile a normalized scope into a full-path matcher. + + `**` spans directories, `*` and `?` stay within one path segment, and + `[...]` is a character class. A scope without wildcards names a file or a + directory, so it also matches everything beneath it. + """ + out: list[str] = [] + i = 0 + while i < len(scope): + ch = scope[i] + if scope.startswith("**/", i): + out.append("(?:.*/)?") + i += 3 + elif scope.startswith("**", i): + out.append(".*") + i += 2 + elif ch == "*": + out.append("[^/]*") + i += 1 + elif ch == "?": + out.append("[^/]") + i += 1 + elif ch == "[" and (end := scope.find("]", i + 2)) != -1 and ( + scope[i + 1 : end] != "!" + ): + body = scope[i + 1 : end] + negate = body.startswith("!") + body = re.sub(r"([\\^\[\]])", r"\\\1", body[1:] if negate else body) + out.append(f"[{'^' if negate else ''}{body}]") + i = end + 1 + else: + out.append(re.escape(ch)) + i += 1 + pattern = "".join(out) + if not any(c in scope for c in "*?["): + pattern += "(?:/.*)?" + return re.compile(pattern + r"\Z", re.DOTALL) + + +def path_in_scopes(path: str, scopes: list[str]) -> bool: + """True when a repository path is covered by any of the declared scopes.""" + return any(_glob_regex(normalize_scope(s)).match(path) for s in scopes) + + def verify_commit( repo_path: str, commit_oid: str, branch: str | None = None ) -> None: @@ -458,6 +539,7 @@ def claim_task( agent_row = get_agent(conn, agent) if not scope_globs: raise ClaimRefused("At least one --scope is required to claim a task.") + scope_globs = list(dict.fromkeys(validate_scope(g) for g in scope_globs)) if not via_handoff: pending = open_handoff_for_task(conn, task_id) diff --git a/tests/test_scope_enforcement.py b/tests/test_scope_enforcement.py new file mode 100644 index 0000000..e8d77af --- /dev/null +++ b/tests/test_scope_enforcement.py @@ -0,0 +1,171 @@ +"""Declared scopes are canonical at claim time and enforced at push time. + +Ported from PR #10 (review findings 3 and 4): + +3. Scope globs are normalized before they are stored and compared, and scopes + that escape the repository are refused. +4. Pushes to a claimed branch are held to the claim's declared scopes. +""" + +from __future__ import annotations + +import os +import subprocess +import uuid +from pathlib import Path + +import pytest + +from quorumgit import registry, work +from tests.conftest import make_git_repo +from tests.test_gate import _commit, _push, _setup + +GIT_ENV = { + **os.environ, + "GIT_AUTHOR_NAME": "t", + "GIT_AUTHOR_EMAIL": "t@localhost", + "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@localhost", +} + + +def _git(path: Path, *args: str) -> str: + return subprocess.run( + ["git", "-C", str(path), *args], + check=True, + capture_output=True, + text=True, + env=GIT_ENV, + ).stdout.strip() + + +def _local_setup(conn, tmp_path, agents=("a", "b")): + """A registered working repository (local model) plus named agents.""" + suffix = uuid.uuid4().hex[:8] + repo_path = make_git_repo(tmp_path / f"repo-{suffix}") + repo = f"local-{suffix}" + registry.add_repository(conn, repo, repo_path) + names = [f"{agent}-{suffix}" for agent in agents] + for name in names: + registry.add_agent(conn, name) + task = work.create_task(conn, repo, "work") + conn.commit() + return repo, task, names + + +# ------------------------------------------------------ claim-time scopes + + +@pytest.mark.parametrize( + ("raw", "normalized"), + [ + ("./src/**", "src/**"), + ("src//api/**", "src/api/**"), + ("/src/**", "src/**"), + ("src\\api\\**", "src/api/**"), + (".", "**"), + ("src/../lib/**", "lib/**"), + ], +) +def test_normalize_scope(raw, normalized): + assert work.normalize_scope(raw) == normalized + + +@pytest.mark.parametrize( + ("a", "b"), + [ + ("./src/**", "src/**"), + ("src\\api\\**", "src/api/handlers.py"), + ("Src/**", "src/app.py"), + ("src/../lib/**", "lib/x.py"), + ], +) +def test_differently_spelled_scopes_overlap(a, b): + assert work.scopes_overlap(a, b) + assert work.scopes_overlap(b, a) + + +def test_scopes_escaping_the_repository_are_refused(conn, tmp_path): + _repo, task, (a, _b) = _local_setup(conn, tmp_path) + with pytest.raises(work.ClaimRefused, match="escapes the repository"): + work.claim_task(conn, task, a, "feat/escape", ["../other/**"]) + + +def test_dot_slash_scope_is_classified_overlapping(conn, tmp_path): + repo, task, (a, b) = _local_setup(conn, tmp_path) + work.claim_task(conn, task, a, "feat/one", ["src/**"]) + other = work.create_task(conn, repo, "second") + with pytest.raises(work.ClaimRefused, match="overlap"): + work.claim_task(conn, other, b, "feat/two", ["./src/**"]) + + claim_id, classification, _ = work.claim_task( + conn, other, b, "feat/two", ["./src/**"], override_overlap=True + ) + assert classification == "OVERLAPPING" + assert work.claim_scopes(conn, claim_id) == ["src/**"] + + +# -------------------------------------------------------- push-time scopes + + +@pytest.mark.parametrize( + ("scope", "path", "inside"), + [ + ("src/**", "src/a/b.py", True), + ("src/**", "srcx/a.py", False), + ("src/*.py", "src/a.py", True), + ("src/*.py", "src/a/b.py", False), + ("docs", "docs/guide.md", True), + ("docs", "docs", True), + ("docs", "docs2/x", False), + ("**/*.md", "README.md", True), + ("**/*.md", "a/b/c.md", True), + ("src/[ab].py", "src/a.py", True), + ("src/[!ab].py", "src/a.py", False), + ("**", "anything/at/all", True), + ], +) +def test_path_in_scopes(scope, path, inside): + assert work.path_in_scopes(path, [scope]) is inside + + +def test_hook_holds_claimed_branch_to_its_scopes(committed_conn, tmp_path, cfg): + conn = committed_conn + repo_name, _hub, clone, a, b = _setup(conn, tmp_path) + task = work.create_task(conn, repo_name, "scoped work") + work.claim_task(conn, task, a, branch="feat/scoped", scope_globs=["src/**"]) + conn.commit() + + _commit(clone, "src/inside.py", branch="feat/scoped") + inside = _push(clone, a, "feat/scoped", cfg=cfg) + assert inside.returncode == 0, inside.stderr + + _commit(clone, "README.md", branch="feat/scoped") + outside = _push(clone, a, "feat/scoped", cfg=cfg) + assert outside.returncode != 0 + assert "outside claim" in outside.stderr + assert "README.md" in outside.stderr + + # Undo the out-of-scope commit; merging another branch's accepted work in + # does not attribute that branch's files to the claim holder. + _git(clone, "reset", "--hard", "HEAD~1") + _commit(clone, "docs/elsewhere.md", branch="feat/base") + base = _push(clone, b, "feat/base", cfg=cfg) + assert base.returncode == 0, base.stderr + _git(clone, "checkout", "feat/scoped") + _git(clone, "merge", "--no-edit", "feat/base") + merged = _push(clone, a, "feat/scoped", cfg=cfg) + assert merged.returncode == 0, merged.stderr + + +def test_hook_scope_check_covers_new_claimed_branches(committed_conn, tmp_path, cfg): + conn = committed_conn + repo_name, _hub, clone, a, _b = _setup(conn, tmp_path) + task = work.create_task(conn, repo_name, "new branch") + work.claim_task(conn, task, a, branch="feat/fresh", scope_globs=["src/**"]) + conn.commit() + + _commit(clone, "docs/outside.md", branch="feat/fresh") + rejected = _push(clone, a, "feat/fresh", cfg=cfg) + assert rejected.returncode != 0 + assert "docs/outside.md" in rejected.stderr