From 82240de4563ab54ac5326b804e0cae06bd3d3403 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 30 Sep 2026 05:27:47 +0000 Subject: [PATCH] Store protected refs as full names; inspect pushes before locking the store A protected ref registered as a short name (`--protected-ref main`) was stored as typed, but the hook matches the full names Git hands it (refs/heads/main), so the branch was silently unprotected while `repo policy` listed it. Protected refs are now normalized to full names (a bare name is a branch) and validated with `git check-ref-format`; migration 010 rewrites existing short names and adds triggers refusing new ones. `repo protect-ref [--remove]` adds or removes one after registration (operator only, audited). pre-receive took the store's write reservation before deriving a push's changed paths, which ran one `git diff-tree` per new commit, twice per ref. A large push held every other agent's command and push past the 5-second busy timeout. Paths are now derived before the reservation is taken (commits are immutable, and the transaction hook re-checks against the recorded paths), and each update's commits are read with a fixed number of Git processes: one walk per baseline and a single batched `diff-tree --raw` whose records keep the parse exact. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Q8BpCkArA34hB1aSyVcnZp --- README.md | 9 +- src/quorumgit/cli.py | 25 ++- src/quorumgit/gate.py | 121 +++++++--- src/quorumgit/git_objects.py | 67 +++++- .../migrations/010_protected_ref_names.sql | 34 +++ src/quorumgit/registry.py | 65 +++++- src/quorumgit/store.py | 2 + tests/test_approval_authority.py | 1 + tests/test_authorization_binding.py | 1 + tests/test_content_governance.py | 107 +++++++++ tests/test_protected_refs.py | 209 ++++++++++++++++++ 11 files changed, 594 insertions(+), 47 deletions(-) create mode 100644 src/quorumgit/migrations/010_protected_ref_names.sql create mode 100644 tests/test_protected_refs.py diff --git a/README.md b/README.md index a9cf1d1..9d3885d 100644 --- a/README.md +++ b/README.md @@ -60,7 +60,7 @@ Then initialize the store once: ```bash $ quorumgit init store: /home/you/.quorumgit/quorumgit.db -migrations applied: ['001_core.sql', '002_approval_identities.sql', '004_approval_authority.sql', '005_content_governance.sql', '006_ref_updates.sql', '007_ref_update_scope_paths.sql', '008_operation_policies.sql', '009_approval_quorum.sql'] +migrations applied: ['001_core.sql', '002_approval_identities.sql', '004_approval_authority.sql', '005_content_governance.sql', '006_ref_updates.sql', '007_ref_update_scope_paths.sql', '008_operation_policies.sql', '009_approval_quorum.sql', '010_protected_ref_names.sql'] contract: ok ``` @@ -277,6 +277,8 @@ Rules that hold no matter what: 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 to full protected ref names.** Protected refs are matched against the full names Git hands the hooks, so one registered as a short name (`--protected-ref main`) used to protect nothing. Migration `010_protected_ref_names.sql` rewrites short names to the branch they name (`main` becomes `refs/heads/main`); from then on pushes to those branches need their `protected_ref_update` approval, as intended. Check `quorumgit repo policy ` after running `quorumgit init`. + > **Upgrading to per-operation policy.** Migration `008_operation_policies.sql` adds no overrides, so every operation keeps following the repository default until an operator sets one. > **Upgrading to content governance.** Migration `005_content_governance.sql` restricts every existing repository to `refs/heads/`. If your agents push tags or other refs, allow those namespaces with `quorumgit repo allow-ref` after running `quorumgit init`. Claimed branches now also enforce their scopes at push time, so a claim declared too narrowly will ask for `out_of_scope_push` approvals. @@ -295,10 +297,11 @@ The incumbent's checkout is not duplicated. When a claim supersedes an earlier c | `quorumgit status` | Store health, contract check, row counts | | `quorumgit doctor [--repair]` | Detect and conservatively reconcile managed-worktree drift and stuck ref updates | | `quorumgit destroy --yes` | Delete the database file (managed worktrees under `QUORUMGIT_DATA_DIR/worktrees` are left in place) | -| `quorumgit repo add [--protected-ref ]… [--protected-path ]…` | Register a repository | +| `quorumgit repo add [--protected-ref ]… [--protected-path ]…` | Register a repository (a bare `--protected-ref` name is taken as a branch: `main` is stored as `refs/heads/main`) | | `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 [--operation [--inherit]] [--threshold ] [--role ]… [--[no-]requester-may-vote] [--[no-]quorum]` | Show or change a repository's default approval policy, or its override for one operation type (changes are operator only); also shows protected refs, protected paths, and allowed ref namespaces | +| `quorumgit repo protect-ref [--remove]` | Require a `protected_ref_update` approval for every update of a ref, such as `refs/heads/main`; a bare name is taken as a branch (operator only) | | `quorumgit repo protect-path [--remove]` | Require an approval for any push changing matching paths (operator only) | | `quorumgit repo allow-ref [--remove]` | Allow pushes to a ref namespace such as `refs/tags/` (operator only) | | `quorumgit task add --repo --title [--objective ]` | Create a task | @@ -320,7 +323,7 @@ The incumbent's checkout is not duplicated. When a claim supersedes an earlier c | `quorumgit hook install --repo ` | Install the pre-receive and reference-transaction hooks (hub model) | | `quorumgit audit [--entity ] [--entity-id ] [--limit ]` | Read the audit trail | -`repo list`, `agent list`, and `task list [--repo ]` enumerate what's registered. Commands that act as an agent (`claim`, `renew`, `release`, `task done`, `checkpoint`, `handoff create/accept/decline/cancel`, `approve request/vote`, `approve prepare`, and the operator actions `task reopen`, `agent add`, `agent role`, `repo policy`, `repo protect-path`, `repo allow-ref`) also take `--agent `, which overrides `QUORUMGIT_AGENT`. At least one `--scope` is required to claim. Exit codes: `0` success, `1` refused/violation/error, `2` usage error. +`repo list`, `agent list`, and `task list [--repo ]` enumerate what's registered. Commands that act as an agent (`claim`, `renew`, `release`, `task done`, `checkpoint`, `handoff create/accept/decline/cancel`, `approve request/vote`, `approve prepare`, and the operator actions `task reopen`, `agent add`, `agent role`, `repo policy`, `repo protect-ref`, `repo protect-path`, `repo allow-ref`) also take `--agent `, which overrides `QUORUMGIT_AGENT`. At least one `--scope` is required to claim. Exit codes: `0` success, `1` refused/violation/error, `2` usage error. `quorumgit doctor` only checks worktree paths already recorded by QuorumGit. It reports: diff --git a/src/quorumgit/cli.py b/src/quorumgit/cli.py index 4e4ac64..bea966d 100644 --- a/src/quorumgit/cli.py +++ b/src/quorumgit/cli.py @@ -232,6 +232,20 @@ def cmd_repo_policy(args, cfg) -> int: return 0 +def cmd_repo_protect_ref(args, cfg) -> int: + with store.session(cfg) as conn: + refs = registry.set_protected_ref( + conn, + args.name, + args.ref, + actor=getattr(args, "agent", None) or cfg.agent, + remove=args.remove, + ) + conn.commit() + print(f"protected refs for {args.name}: {', '.join(refs) or '-'}") + return 0 + + def cmd_repo_protect_path(args, cfg) -> int: with store.session(cfg) as conn: globs = registry.set_protected_path( @@ -733,7 +747,9 @@ def add(name, fn, configure=None, parent=None): add("add", cmd_repo_add, lambda sp: ( sp.add_argument("name"), sp.add_argument("path"), - sp.add_argument("--protected-ref", action="append", default=[]), + sp.add_argument("--protected-ref", action="append", default=[], + help="ref needing approval for every update, such as " + "refs/heads/main (a bare name is taken as a branch)"), sp.add_argument("--protected-path", action="append", default=[], help="path glob whose changes always need an approval"), ), parent=repo) @@ -760,6 +776,13 @@ def add(name, fn, configure=None, parent=None): help="with --operation: remove the override so the " "type inherits the default again"), ), parent=repo) + add("protect-ref", cmd_repo_protect_ref, lambda sp: ( + sp.add_argument("name"), + sp.add_argument("ref", help="ref such as refs/heads/main (a bare name " + "is taken as a branch)"), + sp.add_argument("--remove", action="store_true"), + sp.add_argument("--agent", help="operator making the change"), + ), parent=repo) add("protect-path", cmd_repo_protect_path, lambda sp: ( sp.add_argument("name"), sp.add_argument("glob"), diff --git a/src/quorumgit/gate.py b/src/quorumgit/gate.py index 15afa82..c2024e1 100644 --- a/src/quorumgit/gate.py +++ b/src/quorumgit/gate.py @@ -653,6 +653,32 @@ def _requirement_message(operation: dict, refusal: str | None) -> str: return message +def update_paths( + objects_dir: str | Path, + hub_dir: str | Path, + repo: dict, + oldrev: str, + newrev: str, + known_tips: list[str] | None = None, +) -> tuple[list[str], list[str]]: + """(paths, scope_paths) for one ref update, from one walk of its commits. + + Objects are read from ``objects_dir`` (the hub inside the hook, the + agent's clone for ``approve prepare``); both baselines come from the + registered hub: every ref tip for ``paths``, the mainline for + ``scope_paths`` (see governed_operations). + """ + if known_tips is None: + known_tips = git_objects.ref_tips(hub_dir) + paths, scope_paths = git_objects.changed_paths_against( + objects_dir, + oldrev, + newrev, + [known_tips, git_objects.mainline_tips(hub_dir, repo["protected_refs"])], + ) + return paths, scope_paths + + def evaluate_ref_update( conn: Connection, repository: str, @@ -696,17 +722,12 @@ def evaluate_ref_update( ) try: - if paths is None: - paths = git_objects.changed_paths( - git_dir, oldrev, newrev, git_objects.ref_tips(git_dir) - ) - if scope_paths is None: - scope_paths = git_objects.changed_paths( - git_dir, - oldrev, - newrev, - git_objects.mainline_tips(git_dir, repo["protected_refs"]), + if paths is None or scope_paths is None: + derived_paths, derived_scope_paths = update_paths( + git_dir, git_dir, repo, oldrev, newrev ) + paths = derived_paths if paths is None else paths + scope_paths = derived_scope_paths if scope_paths is None else scope_paths operations = governed_operations( conn, repo, refname, oldrev, newrev, git_dir, paths, scope_paths ) @@ -741,6 +762,8 @@ def check_ref_update( oldrev: str, newrev: str, refname: str, + paths: list[str] | None = None, + scope_paths: list[str] | None = None, ) -> None: """pre-receive: validate one ref update and record it for its transaction. @@ -749,7 +772,15 @@ def check_ref_update( once Git has locked the ref, in the reference-transaction hook. """ repo, granted, paths, scope_paths = evaluate_ref_update( - conn, repository, git_dir, pusher, oldrev, newrev, refname + conn, + repository, + git_dir, + pusher, + oldrev, + newrev, + refname, + paths=paths, + scope_paths=scope_paths, ) operations = [operation for _, operation in granted] row = conn.execute( @@ -844,13 +875,9 @@ def prepare_push( "ref you can push to before preparing this push, so the plan " "matches what the hook will see." ) - paths = git_objects.changed_paths(local_dir, oldrev, newrev, known) # Mainline tips are hub ref tips, so the check above covers them too. - scope_paths = git_objects.changed_paths( - local_dir, - oldrev, - newrev, - git_objects.mainline_tips(hub_dir, repo["protected_refs"]), + paths, scope_paths = update_paths( + local_dir, hub_dir, repo, oldrev, newrev, known_tips=known ) refusals: list[str] = [] @@ -890,6 +917,31 @@ def prepare_push( } +def _derive_pushed_paths( + conn: Connection, + repository: str, + git_dir: str, + updates: list[tuple[str, str, str]], +) -> list[tuple[list[str], list[str]]]: + """(paths, scope_paths) for every update, read from Git alone. + + This is the expensive part of a push check (it reads every new commit), + so it runs before the store's write reservation is taken: other agents' + commands and pushes are not held up while a large push is inspected. + Commits are immutable, and the paths are recorded with the validation, + so the reference transaction re-checks the update against these same + paths under the ref locks. + """ + repo = get_repository(conn, repository) + derived = [] + for oldrev, newrev, _refname in updates: + try: + derived.append(update_paths(git_dir, git_dir, repo, oldrev, newrev)) + except git_objects.GitObjectError as exc: + raise PushRejected(f"Unable to inspect pushed objects: {exc}") from exc + return derived + + def run_pre_receive( conn: Connection, repository: str, stdin_lines: Iterable[str] ) -> int: @@ -900,7 +952,6 @@ def run_pre_receive( return 1 pusher = os.environ.get("QUORUMGIT_AGENT") or None try: - begin_immediate(conn) if pusher is None: raise PushRejected( "Pusher identity required; set QUORUMGIT_AGENT to a registered agent." @@ -909,17 +960,25 @@ def run_pre_receive( get_agent(conn, pusher) except RegistryError as exc: raise PushRejected(f"Pusher identity is not registered: {pusher}") from exc - saw_update = False - for line in stdin_lines: - parts = line.split() - if not parts: - continue - if len(parts) != 3: - raise PushRejected(f"Malformed pre-receive input: {line!r}") - saw_update = True - check_ref_update(conn, repository, git_dir, pusher, *parts) - if not saw_update: + updates = _parse_updates(stdin_lines, hook="pre-receive") + if not updates: raise PushRejected("No ref updates supplied on stdin.") + derived = _derive_pushed_paths(conn, repository, git_dir, updates) + begin_immediate(conn) + for (oldrev, newrev, refname), (paths, scope_paths) in zip( + updates, derived, strict=True + ): + check_ref_update( + conn, + repository, + git_dir, + pusher, + oldrev, + newrev, + refname, + paths=paths, + scope_paths=scope_paths, + ) # Without the transaction hook nothing would re-validate at update # time or consume approvals, so a missing one must reject the push. _require_reference_transaction_hook(conn, repository) @@ -945,14 +1004,16 @@ def run_pre_receive( PREPARED_STUCK_AFTER_SECONDS = VALIDATION_TTL_SECONDS -def _parse_updates(stdin_lines: Iterable[str]) -> list[tuple[str, str, str]]: +def _parse_updates( + stdin_lines: Iterable[str], hook: str = "reference-transaction" +) -> list[tuple[str, str, str]]: updates = [] for line in stdin_lines: parts = line.split() if not parts: continue if len(parts) != 3: - raise PushRejected(f"Malformed reference-transaction input: {line!r}") + raise PushRejected(f"Malformed {hook} input: {line!r}") updates.append((parts[0], parts[1], parts[2])) return updates diff --git a/src/quorumgit/git_objects.py b/src/quorumgit/git_objects.py index afaf8a6..391b48c 100644 --- a/src/quorumgit/git_objects.py +++ b/src/quorumgit/git_objects.py @@ -195,18 +195,45 @@ def new_commits( def commit_paths(git_dir: str | Path, commit: str) -> list[str]: - """Paths one commit introduces. + """Paths one commit introduces (see paths_by_commit).""" + return paths_by_commit(git_dir, [commit])[commit] + + +def paths_by_commit(git_dir: str | Path, commits: list[str]) -> dict[str, list[str]]: + """Paths each commit introduces, read with a single Git process. Root commits are compared with the empty tree. Merges use Git's combined diff, so a clean merge introduces nothing of its own and only paths that differ from every parent (conflict resolutions, evil merges) are counted. + + The raw format keeps the parse exact: Git heads each commit's entries with + its object ID, and every path follows its own `:`-prefixed status record, + so no path can be mistaken for the next commit's ID. """ + result: dict[str, list[str]] = {commit: [] for commit in commits} + if not commits: + return result out = _git( git_dir, - "diff-tree", "-r", "-c", "--root", "--no-renames", "--name-only", - "--no-commit-id", "-z", commit, + "diff-tree", "--stdin", "--always", "-r", "-c", "--root", "--no-renames", + "--raw", "-z", + stdin="".join(f"{commit}\n" for commit in result), ) - return [_text_path(path) for path in out.split("\0") if path] + tokens = iter(out.split("\0")) + current: str | None = None + for token in tokens: + if not token: + continue + if token.startswith(":"): + path = next(tokens, "") + if current is None or not path: + raise GitObjectError("git diff-tree produced an unexpected record.") + result[current].append(_text_path(path)) + elif token in result: + current = token + else: + raise GitObjectError(f"git diff-tree reported an unrequested commit {token}.") + return result def changed_paths( @@ -221,12 +248,28 @@ def changed_paths( checked only on claimed branches, so they are measured against the mainline alone (mainline_tips). A deletion carries no content. """ + return changed_paths_against(git_dir, oldrev, newrev, [known_tips])[0] + + +def changed_paths_against( + git_dir: str | Path, + oldrev: str, + newrev: str, + baselines: list[list[str]], +) -> list[list[str]]: + """changed_paths for each baseline, reading every commit's paths once. + + The number of Git processes is fixed (one commit walk per baseline and a + single diff-tree), however many commits the update brings in. + """ if is_zero(newrev): - return [] - known = list(known_tips) - if not is_zero(oldrev): - known.append(oldrev) - paths: set[str] = set() - for commit in new_commits(git_dir, newrev, known): - paths.update(commit_paths(git_dir, commit)) - return sorted(paths) + return [[] for _ in baselines] + old = [] if is_zero(oldrev) else [oldrev] + walks = [new_commits(git_dir, newrev, [*tips, *old]) for tips in baselines] + by_commit = paths_by_commit( + git_dir, list(dict.fromkeys(commit for walk in walks for commit in walk)) + ) + return [ + sorted({path for commit in walk for path in by_commit[commit]}) + for walk in walks + ] diff --git a/src/quorumgit/migrations/010_protected_ref_names.sql b/src/quorumgit/migrations/010_protected_ref_names.sql new file mode 100644 index 0000000..dbd935b --- /dev/null +++ b/src/quorumgit/migrations/010_protected_ref_names.sql @@ -0,0 +1,34 @@ +-- Protected refs are matched against the full ref names Git hands the hooks +-- (refs/heads/main), so a short name stored as typed ("main") protected +-- nothing. Expand existing short names to the branch they name, dropping any +-- that duplicate a full name already protected, and refuse short names from +-- now on, including from writers that bypass the CLI. + +DELETE FROM protected_refs +WHERE refname NOT GLOB 'refs/*' + AND EXISTS ( + SELECT 1 FROM protected_refs full_name + WHERE full_name.repository_id = protected_refs.repository_id + AND full_name.refname = 'refs/heads/' || protected_refs.refname + ); + +-- quorumgit-statement +UPDATE protected_refs +SET refname = 'refs/heads/' || refname +WHERE refname NOT GLOB 'refs/*'; + +-- quorumgit-statement +CREATE TRIGGER protected_refs_require_full_name +BEFORE INSERT ON protected_refs +WHEN NEW.refname NOT GLOB 'refs/?*' +BEGIN + SELECT RAISE(ABORT, 'protected refs must be full ref names such as refs/heads/main'); +END; + +-- quorumgit-statement +CREATE TRIGGER protected_ref_updates_require_full_name +BEFORE UPDATE OF refname ON protected_refs +WHEN NEW.refname NOT GLOB 'refs/?*' +BEGIN + SELECT RAISE(ABORT, 'protected refs must be full ref names such as refs/heads/main'); +END; diff --git a/src/quorumgit/registry.py b/src/quorumgit/registry.py index e26f532..7549f4c 100644 --- a/src/quorumgit/registry.py +++ b/src/quorumgit/registry.py @@ -230,7 +230,9 @@ def add_repository( ).fetchone() assert row is not None repo_id = row[0] - for refname in dict.fromkeys(protected_refs or []): + for refname in dict.fromkeys( + normalize_protected_ref(ref) for ref in protected_refs or [] + ): conn.execute( "INSERT INTO protected_refs (repository_id, refname) VALUES (?, ?)", (repo_id, refname), @@ -531,6 +533,67 @@ def _set_operation_override( ) +def normalize_protected_ref(ref: str) -> str: + """The full ref name a protected ref is stored and matched as. + + Git hands the hooks full names (``refs/heads/main``), and a protected ref + is matched against them exactly, so a short branch name stored as typed + would protect nothing. A name outside ``refs/`` is therefore taken as a + branch; anything Git would not accept as a ref name is refused. + """ + name = ref.strip() + full = name if name.startswith("refs/") else f"refs/heads/{name}" + if not name or name.startswith("-") or name == "HEAD": + raise RegistryError(f"Invalid protected ref: {ref!r}") + result = subprocess.run( + ["git", "check-ref-format", full], + capture_output=True, + text=True, + check=False, + env=path_scoped_git_env(), + ) + if result.returncode != 0: + raise RegistryError(f"Invalid protected ref: {ref!r} is not a valid ref name.") + return full + + +def set_protected_ref( + conn: Connection, + repository: str, + ref: str, + *, + actor: str | None, + remove: bool = False, +) -> list[str]: + """Add or remove a ref whose every update needs an approval.""" + begin_immediate(conn) + repo = get_repository(conn, repository) + require_operator(conn, actor, "Changing protected refs") + refname = normalize_protected_ref(ref) + if remove: + cur = conn.execute( + "DELETE FROM protected_refs WHERE repository_id = ? AND refname = ?", + (repo["id"], refname), + ) + else: + cur = conn.execute( + "INSERT INTO protected_refs (repository_id, refname) VALUES (?, ?) " + "ON CONFLICT DO NOTHING", + (repo["id"], refname), + ) + if cur.rowcount: + audit.record( + conn, + "repository.protected_ref_removed" if remove + else "repository.protected_ref_added", + "repository", + repo["id"], + agent=actor, + detail={"refname": refname}, + ) + return _protected_refs(conn, repo["id"]) + + def _validate_path_glob(glob: str) -> str: if not glob.strip() or glob.startswith("-"): raise RegistryError(f"Invalid protected path glob: {glob!r}") diff --git a/src/quorumgit/store.py b/src/quorumgit/store.py index 6a99aa7..3ff5b5a 100644 --- a/src/quorumgit/store.py +++ b/src/quorumgit/store.py @@ -74,6 +74,8 @@ "vote_updates_require_eligible_voter", "approvals_consumer_is_not_approver", "repositories_default_ref_namespaces", + "protected_refs_require_full_name", + "protected_ref_updates_require_full_name", ) diff --git a/tests/test_approval_authority.py b/tests/test_approval_authority.py index c3387db..d155806 100644 --- a/tests/test_approval_authority.py +++ b/tests/test_approval_authority.py @@ -366,6 +366,7 @@ def test_upgrade_leaves_no_agent_with_authority(tmp_path): "007_ref_update_scope_paths.sql", "008_operation_policies.sql", "009_approval_quorum.sql", + "010_protected_ref_names.sql", ] upgraded = store.connect(local) try: diff --git a/tests/test_authorization_binding.py b/tests/test_authorization_binding.py index 4c07238..4f931f9 100644 --- a/tests/test_authorization_binding.py +++ b/tests/test_authorization_binding.py @@ -44,6 +44,7 @@ def _apply_001(cfg: Config): "007_ref_update_scope_paths.sql", "008_operation_policies.sql", "009_approval_quorum.sql", + "010_protected_ref_names.sql", ] diff --git a/tests/test_content_governance.py b/tests/test_content_governance.py index d85d059..269b39a 100644 --- a/tests/test_content_governance.py +++ b/tests/test_content_governance.py @@ -116,6 +116,113 @@ def test_changed_paths_count_only_content_new_to_the_repository(tmp_path): assert git_objects.changed_paths(git_dir, feat, git_objects.ZERO_OID, []) == [] +def _paths_one_commit_at_a_time(git_dir: Path, commit: str) -> list[str]: + """The per-commit derivation paths_by_commit replaces, as an oracle.""" + out = subprocess.run( + ["git", "--git-dir", str(git_dir), "diff-tree", "-r", "-c", "--root", + "--no-renames", "--name-only", "--no-commit-id", "-z", commit], + check=True, capture_output=True, text=True, + ).stdout + return [path for path in out.split("\0") if path] + + +def test_batched_paths_match_one_commit_at_a_time(tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-b", "main") + root = _edit(repo, "README.md", message="root") + _git(repo, "checkout", "-b", "side") + _edit(repo, "odd dir/a:b c.txt", message="side") + _git(repo, "checkout", "main") + _edit(repo, "infra/prod.tf", message="mainline") + _git(repo, "merge", "--no-edit", "side") # clean merge: nothing of its own + _git(repo, "checkout", "-b", "evil", "HEAD~1") + _edit(repo, "infra/prod.tf", message="conflicting") + _git(repo, "checkout", "main") + _git(repo, "merge", "--no-edit", "-X", "ours", "evil") + _edit(repo, "infra/prod.tf", message="hand-edited merge result") + _git(repo, "commit", "--allow-empty", "-m", "empty") + # A path spelled exactly like another commit's ID stays a path. + _edit(repo, root, message="named after the root commit") + git_dir = git_objects.absolute_git_dir(repo) + commits = _git(repo, "rev-list", "--all").split() + + batched = git_objects.paths_by_commit(git_dir, commits) + assert batched == { + commit: _paths_one_commit_at_a_time(git_dir, commit) for commit in commits + } + assert root in {path for paths in batched.values() for path in paths} + assert git_objects.paths_by_commit(git_dir, []) == {} + + +def test_path_derivation_runs_a_fixed_number_of_git_processes(tmp_path, monkeypatch): + """A push's cost in Git processes does not grow with its commit count.""" + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-b", "main") + base = head = _edit(repo, "README.md", message="base") + for i in range(40): + head = _edit(repo, f"src/file{i % 7}.py", message=f"commit {i}") + git_dir = git_objects.absolute_git_dir(repo) + + calls = [] + real_run = subprocess.run + + def counting_run(*args, **kwargs): + calls.append(args[0]) + return real_run(*args, **kwargs) + + monkeypatch.setattr(git_objects.subprocess, "run", counting_run) + paths, scope_paths = git_objects.changed_paths_against( + git_dir, git_objects.ZERO_OID, head, [[base], []] + ) + assert paths == [f"src/file{i}.py" for i in range(7)] + assert scope_paths == ["README.md", *paths] + # One commit walk per baseline plus one diff-tree, not one per commit. + assert len(calls) == 3 + + +def test_pre_receive_reads_pushed_objects_before_reserving_the_store( + committed_conn, tmp_path, monkeypatch +): + """Inspecting a push must not hold every other agent's command up.""" + conn = committed_conn + repo_name, hub, clone, agent_a, _ = _setup(conn, tmp_path) + _git(clone, "checkout", "-b", "feature") + newrev = _edit(clone, "docs/new.md", message="feature") + # Put the objects in the hub without a ref (or any hook), as a push's + # quarantine would before pre-receive runs. + subprocess.run( + ["git", "--git-dir", str(hub), "-c", "core.hooksPath=/dev/null", + "fetch", "--quiet", str(clone), "feature"], + check=True, capture_output=True, + ) + + reserved_while_reading = [] + real_update_paths = gate.update_paths + + def watching_update_paths(*args, **kwargs): + reserved_while_reading.append(conn.in_transaction) + return real_update_paths(*args, **kwargs) + + monkeypatch.setattr(gate, "update_paths", watching_update_paths) + monkeypatch.setenv("GIT_DIR", str(hub)) + monkeypatch.setenv("QUORUMGIT_AGENT", agent_a) + zero = git_objects.zero_oid_like(newrev) + status = gate.run_pre_receive( + conn, repo_name, [f"{zero} {newrev} refs/heads/feature\n"] + ) + + assert status == 0 + assert reserved_while_reading == [False] + recorded = conn.execute( + "SELECT paths FROM ref_updates WHERE refname = 'refs/heads/feature' " + "AND newrev = ?", + (newrev,), + ).fetchone() + assert recorded is not None and json.loads(recorded[0]) == ["docs/new.md"] + + # ------------------------------------------------------------ claimed scopes diff --git a/tests/test_protected_refs.py b/tests/test_protected_refs.py new file mode 100644 index 0000000..9f06c27 --- /dev/null +++ b/tests/test_protected_refs.py @@ -0,0 +1,209 @@ +"""Protected refs are stored and matched as full ref names.""" + +from __future__ import annotations + +import sqlite3 +import subprocess +import uuid +from pathlib import Path + +import pytest + +from quorumgit import audit, cli, gate, registry, store +from quorumgit.config import Config +from tests.conftest import OPERATOR, approve, ensure_agent, make_git_repo +from tests.test_gate import _commit, _push + + +def _hub(conn, tmp_path: Path, protected_refs: list[str]): + seed = make_git_repo(tmp_path / "seed") + hub = tmp_path / "hub.git" + subprocess.run( + ["git", "clone", "--bare", str(seed), str(hub)], + check=True, + capture_output=True, + ) + name = f"hub-{uuid.uuid4().hex[:8]}" + registry.add_repository(conn, name, hub, protected_refs=protected_refs) + pusher = f"pusher-{uuid.uuid4().hex[:8]}" + registry.add_agent(conn, pusher) + gate.install_hook(conn, name) + conn.commit() + clone = tmp_path / "clone" + subprocess.run( + ["git", "clone", str(hub), str(clone)], check=True, capture_output=True + ) + return name, clone, pusher + + +def test_short_protected_ref_name_protects_the_branch(committed_conn, tmp_path, cfg): + """`--protected-ref main` used to be stored as typed and protect nothing.""" + conn = committed_conn + name, clone, pusher = _hub(conn, tmp_path, ["main"]) + assert registry.get_repository(conn, name)["protected_refs"] == ["refs/heads/main"] + + _commit(clone, "change.txt") + rejected = _push(clone, pusher, "HEAD:refs/heads/main", cfg=cfg) + assert rejected.returncode != 0 + assert "protected_ref_update on refs/heads/main" in rejected.stderr + + newrev = subprocess.run( + ["git", "-C", str(clone), "rev-parse", "HEAD"], + check=True, capture_output=True, text=True, + ).stdout.strip() + oldrev = subprocess.run( + ["git", "-C", str(clone), "rev-parse", "origin/main"], + check=True, capture_output=True, text=True, + ).stdout.strip() + approve(conn, { + "type": "protected_ref_update", + "repository": name, + "refname": "refs/heads/main", + "oldrev": oldrev, + "newrev": newrev, + }, requested_by=pusher) + conn.commit() + accepted = _push(clone, pusher, "HEAD:refs/heads/main", cfg=cfg) + assert accepted.returncode == 0, accepted.stderr + + +@pytest.mark.parametrize( + ("given", "stored"), + [ + ("main", "refs/heads/main"), + (" release/1.0 ", "refs/heads/release/1.0"), + ("refs/heads/main", "refs/heads/main"), + ("refs/tags/v1", "refs/tags/v1"), + ], +) +def test_protected_ref_names_are_normalized(given, stored): + assert registry.normalize_protected_ref(given) == stored + + +@pytest.mark.parametrize("bad", ["", " ", "-main", "HEAD", "bad..name", "a b", "refs/"]) +def test_invalid_protected_refs_are_refused(bad): + with pytest.raises(registry.RegistryError, match="Invalid protected ref"): + registry.normalize_protected_ref(bad) + + +def test_protect_ref_command_is_operator_only_and_audited(conn, tmp_path): + ensure_agent(conn, OPERATOR, "operator") + ensure_agent(conn, "ref-worker") + name = f"repo-{uuid.uuid4().hex[:8]}" + registry.add_repository(conn, name, make_git_repo(tmp_path / "r")) + repo_id = registry.get_repository(conn, name)["id"] + + with pytest.raises(registry.RegistryError, match="requires an operator"): + registry.set_protected_ref(conn, name, "main", actor="ref-worker") + + assert registry.set_protected_ref(conn, name, "main", actor=OPERATOR) == [ + "refs/heads/main" + ] + # The same branch spelled in full is the same protected ref. + assert registry.set_protected_ref( + conn, name, "refs/heads/main", actor=OPERATOR + ) == ["refs/heads/main"] + assert registry.set_protected_ref( + conn, name, "main", actor=OPERATOR, remove=True + ) == [] + + events = [ + (e["event_type"], e["detail"]["refname"]) + for e in audit.events(conn, entity="repository", entity_id=repo_id) + if e["event_type"].startswith("repository.protected_ref") + ] + # Newest first; re-adding the same ref was a no-op and left no event. + assert events == [ + ("repository.protected_ref_removed", "refs/heads/main"), + ("repository.protected_ref_added", "refs/heads/main"), + ] + + +def test_store_refuses_short_protected_ref_names(conn, tmp_path): + name = f"repo-{uuid.uuid4().hex[:8]}" + registry.add_repository(conn, name, make_git_repo(tmp_path / "r")) + repo_id = registry.get_repository(conn, name)["id"] + with pytest.raises(sqlite3.IntegrityError, match="full ref names"): + conn.execute( + "INSERT INTO protected_refs (repository_id, refname) VALUES (?, 'main')", + (repo_id,), + ) + conn.execute( + "INSERT INTO protected_refs (repository_id, refname) " + "VALUES (?, 'refs/heads/main')", + (repo_id,), + ) + with pytest.raises(sqlite3.IntegrityError, match="full ref names"): + conn.execute( + "UPDATE protected_refs SET refname = 'main' WHERE repository_id = ?", + (repo_id,), + ) + + +def test_upgrade_expands_short_protected_ref_names(tmp_path): + local = Config(data_dir=tmp_path / "upgrade", agent=None) + local.data_dir.mkdir(parents=True) + conn = store.open_connection(local) + try: + conn.execute(store.SCHEMA_MIGRATIONS_DDL) + for migration in store._migration_files(): + if migration.name >= "010": + break + for statement in store._migration_statements( + migration.read_text(encoding="utf-8") + ): + conn.execute(statement) + conn.execute( + "INSERT INTO schema_migrations (version) VALUES (?)", + (migration.name,), + ) + repo_id = conn.execute( + "INSERT INTO repositories (name, path) VALUES ('old', ?) RETURNING id", + (str(tmp_path),), + ).fetchone()[0] + conn.executemany( + "INSERT INTO protected_refs (repository_id, refname) VALUES (?, ?)", + [ + (repo_id, "main"), + (repo_id, "release"), + (repo_id, "refs/heads/release"), + (repo_id, "refs/tags/v1"), + ], + ) + conn.commit() + finally: + conn.close() + + assert store.migrate(local) == ["010_protected_ref_names.sql"] + upgraded = store.connect(local) + try: + refs = sorted( + row[0] + for row in upgraded.execute( + "SELECT refname FROM protected_refs WHERE repository_id = ?", + (repo_id,), + ) + ) + finally: + upgraded.close() + assert refs == ["refs/heads/main", "refs/heads/release", "refs/tags/v1"] + store.destroy(local) + + +def test_cli_protect_ref(committed_conn, tmp_path, cfg, monkeypatch, capsys): + conn = committed_conn + ensure_agent(conn, OPERATOR, "operator") + name = f"repo-{uuid.uuid4().hex[:8]}" + registry.add_repository(conn, name, make_git_repo(tmp_path / "r")) + conn.commit() + monkeypatch.setenv("QUORUMGIT_DATA_DIR", str(cfg.data_dir)) + monkeypatch.delenv("QUORUMGIT_AGENT", raising=False) + + assert cli.main(["repo", "protect-ref", name, "main", "--agent", OPERATOR]) == 0 + assert f"protected refs for {name}: refs/heads/main" in capsys.readouterr().out + assert cli.main(["repo", "protect-ref", name, "bad..ref", "--agent", OPERATOR]) == 1 + assert "Invalid protected ref" in capsys.readouterr().err + assert cli.main([ + "repo", "protect-ref", name, "refs/heads/main", "--remove", "--agent", OPERATOR, + ]) == 0 + assert f"protected refs for {name}: -" in capsys.readouterr().out