diff --git a/agent-notes/flat-tree-ref-leases-303.md b/agent-notes/flat-tree-ref-leases-303.md new file mode 100644 index 0000000..10907e1 --- /dev/null +++ b/agent-notes/flat-tree-ref-leases-303.md @@ -0,0 +1,32 @@ +# Flat-tree branch leases (#303 consumer) + +## Goal +Use the existing mutation/CAS owner to reject stale source heads and an unexpectedly created target during a consumer's planned flat-tree commit. Publish a tested draft only; no ready/merge/comments. + +## State +COMPATIBILITY CORRECTION in progress under parent authorization. Confirmed using actual Dulwich `BaseRepo.__getitem__` (repo.py:1267) and unchanged prior module `f3668cc`: `HEAD`/`refs/heads/x` literal branches at base `8f91007cdebf78a7656f006fea1f42072242db7d`, x/symbolic HEAD at left `d4ece5a12676640d2809ab1f1530c3729dfd664c`. Prior public API returned base for both; draft `0e961cc` returned left. Preserved reproduction script `.tmp/reproduce-merge-base-compatibility.py`. New owner regressions produced **8 failed, 22 passed in 2.86s**, log `merge-base-compatibility-reproduction-20261020-004055.log`; includes wrong ancestry and accidental admission of missing branch names as ref aliases in both memory/disk stores. + +Cause: I sent every branch-name input through Dulwich's broader ref-or-object lookup. Smallest correction (~8 lines) restricts immutable-ID dispatch with the dependency's existing `valid_hexsha` before `_commit_object`, retaining `branch_sha` for other inputs. UTF-8 bytes preserve admitted Unicode branch names. No new ref conventions, validator or lease changes. The broken dependency edge is an already-owned branch-name capability passed to a broader representation; use each owner interface for exactly its input. Full-ID and same-named-branch precedence regression retained; fresh focused run/consumer pin/full draft CI next. + +Corrected focused file PASSED **30 tests in 2.59s**, `logs/test-runs/merge-base-literal-branch-fix-20261020-004138.log`. Full captured-ID precedence still passes; literal HEAD/ref-shaped/20-character/nonhex-40-character/Unicode branch names resolve via the branch owner, and missing literal names are not admitted through Git ref aliases. Publish correction to existing draft #21, repin #325 and verify fresh exact-head full CI. Commit/ref lease code is unchanged. + +COMPLETE: draft https://github.com/propstore/quire/pull/21 is OPEN/DRAFT at exact published source `0e961ccb77d772af61254b34dc87f3a32d953611`, base `f3668cc07719793d2430343ffd996e7b78355f2e`. PR CI https://github.com/propstore/quire/actions/runs/39380651728 and push CI https://github.com/propstore/quire/actions/runs/39380627834 both completed SUCCESS. All eight CheckRuns passed lint, typecheck, tests and build across Ubuntu/Windows Python 3.11/3.13; each job reports **623 passed, 12 benchmarks deselected**. Final PR-run test durations: Ubuntu 3.11 99.27s, Ubuntu 3.13 66.92s, Windows 3.11 184.06s, Windows 3.13 172.69s. Consumer draft https://github.com/propstore/propstore/pull/325 pins this exact commit and passed its full suite (1992 passed, 4 skipped, 4 warnings in 109.67s). Both remain unmerged/draft; no ready, merge, public comments/reviews, external edits, user-branch changes or local dependency installs. #303 remains OPEN. No blocker. This final note-only checkpoint is local; the remote source head remains the tested commit above. + +Fresh branch `fix/flat-tree-ref-leases-303` at default `f3668cc07719793d2430343ffd996e7b78355f2e`. Global/project instructions, existing transaction/ref helpers and architecture read. Original checkout/worktrees preserved. + +Owner focused run passed **16 tests in 1.62s**, `logs/test-runs/flat-tree-ref-leases-20261020-002329.log`, using the existing shared interpreter read-only and the consumer logged wrapper. Memory/disk cases cover absence, moved/deleted sources, normalization interleaving, target CAS interleaving, conflicting leases, exact successful tree/parents and pinned merge bases. Added explicit precedence for immutable full commit IDs over a same-named branch before final focused rerun. + +Final owner rerun PASSED **16 tests in 1.40s**, `logs/test-runs/flat-tree-ref-leases-final-20261020-002602.log`, including a branch named exactly the captured commit ID pointing elsewhere. Original temporary repositories/logs remain untracked and must not be staged with the source. + +The initial un-escalated GitHub branch reads failed with sandbox socket errors. Parent clarified this was an execution sandbox limit, authorized the supported escalation for the same reads, and continued only the bounded draft workflow. The escalated reads succeeded: Quire default remains `f3668cc07719793d2430343ffd996e7b78355f2e`; Propstore remains `8429ef6abc4f8655e7b5a09bb3fcca81031078ee`. No approval denial, alternate route, sandbox/security change or secret access. Commit/publish the existing patch, then verify full PR CI. No dependency installs or shared-environment writes. + +## Decisions and why +Smallest repair (~35 generic lines): extend `commit_flat_tree` with named expected heads, using the existing absence-aware ref assertion under its mutation guard, and allow `merge_base` to consume immutable commit IDs. Existing single-target callers remain valid. No new lock, transaction protocol or application policy. Quire writers share the existing guard; this does not promise an atomic multi-ref transaction against arbitrary external Git writers. + +Cleanup-refactor is active. The broken edge is a valid generic ref-precondition capability in its correct owner, currently losing absence at the public boundary. Extend the owner directly. Propstore uses these mechanics, owning only which branch snapshots its workflow requires. Git update-ref primary documentation distinguishes unspecified old refs from expected nonexistence; Quire's architecture already requires captured commits for planning and typed mismatches before commit objects. + +## Surprises +None yet. + +## Next step +Hand off both tested drafts for independent review. Preserve original failing consumer artifacts, temporary repositories, logs and branches. No ready/merge/comments permitted. Named-source checks serialize Quire writers using the existing mutation guard; this patch introduces no atomic multi-ref transaction with arbitrary external Git writers. diff --git a/quire/git_store.py b/quire/git_store.py index 15ef5c9..79cb8ed 100644 --- a/quire/git_store.py +++ b/quire/git_store.py @@ -15,7 +15,7 @@ from dulwich.client import get_transport_and_path from dulwich.index import build_index_from_tree from dulwich.graph import find_merge_base -from dulwich.objects import Blob, Commit, ObjectID, Tree +from dulwich.objects import Blob, Commit, ObjectID, Tree, valid_hexsha from dulwich.refs import Ref from dulwich.repo import BaseRepo, MemoryRepo, Repo @@ -656,13 +656,26 @@ def commit_flat_tree( parents: Sequence[str | bytes], branch: str | None = None, expected_head: str | None = None, + expected_heads: Mapping[str, str | None] | None = None, ) -> str: + """Publish a planned tree with branch leases checked under the mutation guard. + + An entry in ``expected_heads`` requires that branch to retain its captured + head; ``None`` requires absence. Omitted entries impose no caller lease. + The existing guard serializes Quire writers across checking and publishing; + this is not a multi-ref transaction with external Git writers. + """ with self._mutation_guard(): branch_name = self._resolve_write_branch_name(branch) branch_ref = f"refs/heads/{branch_name}".encode() current_head = _ref_get(self._repo.refs, branch_ref) + heads = dict(expected_heads or {}) if expected_head is not None: - _assert_ref_equals(self._repo.refs, branch_name, branch_ref, expected_head.encode("ascii")) + if branch_name in heads and heads[branch_name] != expected_head: + raise ValueError(f"conflicting expected heads for branch {branch_name!r}") + heads[branch_name] = expected_head + heads.setdefault(branch_name, _format_ref_value(current_head)) + self._assert_branch_heads(heads) normalized_entries: dict[str, bytes] = {} for path, sha in entries.items(): @@ -675,7 +688,7 @@ def commit_flat_tree( self._check_add_path_conflicts({ PurePosixPath(path).parts: sha for path, sha in normalized_entries.items() }) - _assert_ref_equals(self._repo.refs, branch_name, branch_ref, current_head) + self._assert_branch_heads(heads) root_tree = self._build_tree_from_flat(normalized_entries) commit = Commit() @@ -708,6 +721,13 @@ def commit_flat_tree( _set_symbolic_ref(self._repo.refs, b"HEAD", branch_ref) return commit.id.decode("ascii") + def _assert_branch_heads(self, heads: Mapping[str, str | None]) -> None: + for name, head in heads.items(): + _assert_ref_equals( + self._repo.refs, name, f"refs/heads/{name}".encode(), + None if head is None else head.encode("ascii"), + ) + def head_sha(self) -> str | None: try: return self._repo.head().decode("ascii") @@ -864,12 +884,22 @@ def ancestor_distances(self, start_sha: str) -> dict[str, int]: return distances def merge_base(self, branch_a: str, branch_b: str) -> str: - sha_a = self.branch_sha(branch_a) - sha_b = self.branch_sha(branch_b) - if sha_a is None: - raise ValueError(f"Branch {branch_a!r} does not exist") - if sha_b is None: - raise ValueError(f"Branch {branch_b!r} does not exist") + """Find the base of branch names or full commit IDs (IDs take precedence).""" + shas: list[str] = [] + for revision in (branch_a, branch_b): + sha = None + encoded = revision.encode("utf-8") + if valid_hexsha(encoded): + try: + sha = self._commit_object(encoded).id.decode("ascii") + except (KeyError, ValueError, TypeError): + pass + if sha is None: + sha = self.branch_sha(revision) + if sha is None: + raise ValueError(f"Branch or commit {revision!r} does not exist") + shas.append(sha) + sha_a, sha_b = shas if sha_a == sha_b: return sha_a diff --git a/tests/test_flat_tree_ref_leases.py b/tests/test_flat_tree_ref_leases.py new file mode 100644 index 0000000..f19ed2d --- /dev/null +++ b/tests/test_flat_tree_ref_leases.py @@ -0,0 +1,137 @@ +"""Named branch expectations retain absence and guard captured source snapshots.""" + +import pytest + +from quire import git_store +from quire.git_store import GitStore, HeadMismatchError + + +@pytest.fixture(params=["memory", "disk"]) +def store(request, tmp_path): + return GitStore.init_memory() if request.param == "memory" else GitStore.init(tmp_path / "repo") + + +def test_absent_target_lease_rejects_a_new_branch_before_objects(store): + parent = store.commit_files({"kept": b"parent"}, "parent") + concurrent = store.commit_files({"other": b"concurrent"}, "concurrent", branch="result") + before = set(store.raw_repo.object_store) + with pytest.raises(HeadMismatchError) as caught: + store.commit_flat_tree({}, "stale", parents=[parent], branch="result", expected_heads={"result": None}) + assert caught.value.branch == "result" + assert caught.value.expected_head is None + assert caught.value.actual_head == concurrent + assert store.branch_sha("result") == concurrent + assert set(store.raw_repo.object_store) == before + + +@pytest.mark.parametrize("deleted", [False, True]) +def test_source_lease_rejects_moved_or_deleted_source_before_objects(store, deleted): + parent = store.commit_files({"kept": b"parent"}, "parent") + store.create_branch("source", source_commit=parent) + if deleted: + store.delete_branch("source") + actual = None + else: + actual = store.commit_files({"late": b"late"}, "late", branch="source") + before = set(store.raw_repo.object_store) + with pytest.raises(HeadMismatchError) as caught: + store.commit_flat_tree({}, "stale", parents=[parent], branch="result", expected_heads={"source": parent, "result": None}) + assert caught.value.branch == "source" + assert caught.value.expected_head == parent + assert caught.value.actual_head == actual + assert store.branch_sha("result") is None + assert set(store.raw_repo.object_store) == before + + +def test_source_lease_is_rechecked_after_entry_normalization(store): + parent = store.commit_files({"kept": b"parent"}, "parent") + store.create_branch("source", source_commit=parent) + blob = store.store_blob(b"candidate") + before = set() + actual = None + + class RacingEntries(dict): + def items(self): + nonlocal before, actual + yield from super().items() + actual = store.commit_files({"late": b"late"}, "late", branch="source") + before = set(store.raw_repo.object_store) + + with pytest.raises(HeadMismatchError) as caught: + store.commit_flat_tree(RacingEntries({"candidate": blob}), "stale", parents=[parent], branch="result", expected_heads={"source": parent, "result": None}) + assert caught.value.actual_head == actual + assert store.branch_sha("result") is None + assert set(store.raw_repo.object_store) == before + + +def test_target_creation_during_compare_and_swap_cannot_be_overwritten(store, monkeypatch): + parent = store.commit_files({"kept": b"parent"}, "parent") + original = git_store._ref_set_if_equals + + def interleave(refs, name, old, new): + if name == b"refs/heads/result": + assert original(refs, name, None, parent.encode("ascii")) + return original(refs, name, old, new) + + monkeypatch.setattr(git_store, "_ref_set_if_equals", interleave) + with pytest.raises(HeadMismatchError) as caught: + store.commit_flat_tree({}, "stale", parents=[parent], branch="result", expected_heads={"result": None}) + assert caught.value.expected_head is None + assert caught.value.actual_head == parent + assert store.branch_sha("result") == parent + + +def test_valid_leases_allow_exact_tree_and_parent_publication(store): + left = store.commit_files({"left": b"left"}, "left") + store.create_branch("peer", source_commit=left) + right = store.commit_files({"right": b"right"}, "right", branch="peer") + entries = dict(store.iter_flat_tree_entries(right)) + merged = store.commit_flat_tree(entries, "merge", parents=[left, right], branch="result", expected_heads={"master": left, "peer": right, "result": None}) + assert store.branch_sha("result") == merged + assert dict(store.iter_flat_tree_entries(merged)) == entries + assert list(store.iter_commit_parent_shas(merged)) == [left, right] + assert (store.branch_sha("master"), store.branch_sha("peer")) == (left, right) + + +def test_conflicting_target_expectations_reject_before_objects(store): + parent = store.commit_files({"kept": b"parent"}, "parent") + before = set(store.raw_repo.object_store) + with pytest.raises(ValueError, match="conflicting expected heads"): + store.commit_flat_tree({}, "invalid", parents=[parent], expected_head=parent, expected_heads={"master": None}) + assert set(store.raw_repo.object_store) == before + + +def test_merge_base_accepts_captured_commits_after_refs_move(store): + base = store.commit_files({"base": b"base"}, "base") + store.create_branch("peer", source_commit=base) + left = store.commit_files({"left": b"left"}, "left") + right = store.commit_files({"right": b"right"}, "right", branch="peer") + store.commit_files({"later": b"later"}, "later") + assert store.merge_base(left, right) == base + assert store.merge_base("master", right) == base + assert store.merge_base(left, left) == left + store.create_branch(left, source_commit=right) + assert store.merge_base(left, left) == left + with pytest.raises(ValueError, match="does not exist"): + store.merge_base("missing", right) + + +@pytest.mark.parametrize("name", ["HEAD", "refs/heads/x", "a" * 20, "z" * 40, "é" * 40]) +def test_merge_base_resolves_literal_branch_names_with_divergent_ancestry(store, name): + base = store.commit_files({"base": b"base"}, "base") + left = store.commit_files({"left": b"left"}, "left") + store.create_branch("x", source_commit=left) + store.create_branch(name, source_commit=base) + assert store.branch_sha(name) == base + assert store.branch_sha("x") == left + assert store.merge_base(name, "x") == base + assert store.merge_base("x", name) == base + + +@pytest.mark.parametrize("name", ["HEAD", "refs/heads/x"]) +def test_merge_base_does_not_admit_ref_aliases_as_missing_branches(store, name): + base = store.commit_files({"base": b"base"}, "base") + store.create_branch("x", source_commit=base) + assert store.branch_sha(name) is None + with pytest.raises(ValueError, match="does not exist"): + store.merge_base(name, "x")