Better merge behavior for git-gate repos on extends #238

Merged
didericis merged 5 commits from git-gate-repo-field-merge-on-extends into main 2026-06-22 14:49:50 -04:00
4 changed files with 155 additions and 47 deletions
+5 -17
View File
@@ -45,7 +45,7 @@ from ..agent_provider import AgentProvisionPlan, get_provider, build_agent_provi
from ..egress import EgressPlan from ..egress import EgressPlan
from ..git_gate import GitGatePlan from ..git_gate import GitGatePlan
from ..log import die, info from ..log import die, info
from ..manifest import ManifestGitEntry, Manifest from ..manifest import Manifest
from ..supervise import SupervisePlan from ..supervise import SupervisePlan
from ..util import expand_tilde from ..util import expand_tilde
from ..env import resolve_env, ResolvedEnv from ..env import resolve_env, ResolvedEnv
@@ -356,16 +356,14 @@ class BottleBackend(ABC, Generic[PlanT, CleanupT]):
pass pass
def _validate(self, spec: BottleSpec) -> None: def _validate(self, spec: BottleSpec) -> None:
"""Cross-backend pre-launch checks. Confirms the agent exists, """Cross-backend pre-launch checks. Confirms the agent exists
the named skills are present on the host, and every git and the named skills are present on the host. Subclasses with
IdentityFile resolves. Subclasses with additional preconditions additional preconditions should override and call
should override and call `super()._validate(spec)` first.""" `super()._validate(spec)` first."""
manifest = spec.manifest manifest = spec.manifest
manifest.require_agent(spec.agent_name) manifest.require_agent(spec.agent_name)
agent = manifest.agents[spec.agent_name] agent = manifest.agents[spec.agent_name]
bottle = manifest.bottle_for(spec.agent_name)
self._validate_skills(agent.skills) self._validate_skills(agent.skills)
self._validate_git_entries(bottle.git)
self._validate_agent_provider_dockerfile(spec) self._validate_agent_provider_dockerfile(spec)
def _validate_skills(self, skills: Sequence[str]) -> None: def _validate_skills(self, skills: Sequence[str]) -> None:
@@ -380,16 +378,6 @@ class BottleBackend(ABC, Generic[PlanT, CleanupT]):
f"Create it under ~/.claude/skills/, then re-run." f"Create it under ~/.claude/skills/, then re-run."
) )
def _validate_git_entries(self, entries: Sequence[ManifestGitEntry]) -> None:
"""Each entry's IdentityFile must exist on the host (after
expanding leading ~) — the git-gate copies it in at start time
to authenticate the upstream push (PRD 0008). Shape is already
enforced by Manifest validation; this only checks presence."""
for entry in entries:
key = expand_tilde(entry.IdentityFile)
if not os.path.isfile(key):
die(f"git upstream key file not found for '{entry.Name}': {key}")
def _validate_agent_provider_dockerfile(self, spec: BottleSpec) -> None: def _validate_agent_provider_dockerfile(self, spec: BottleSpec) -> None:
bottle = spec.manifest.bottle_for(spec.agent_name) bottle = spec.manifest.bottle_for(spec.agent_name)
dockerfile = bottle.agent_provider.dockerfile dockerfile = bottle.agent_provider.dockerfile
+66 -18
View File
@@ -5,16 +5,20 @@ from __future__ import annotations
from typing import TYPE_CHECKING from typing import TYPE_CHECKING
if TYPE_CHECKING: if TYPE_CHECKING:
from .manifest import ManifestBottle, ManifestGitEntry from .manifest import ManifestBottle
from .manifest_egress import ManifestEgressConfig from .manifest_egress import ManifestEgressConfig
def resolve_bottles(raws: dict[str, dict[str, object]]) -> dict[str, ManifestBottle]: def resolve_bottles(raws: dict[str, dict[str, object]]) -> dict[str, ManifestBottle]:
"""Apply `extends:` chains and return resolved ManifestBottle objects.""" """Apply `extends:` chains and return resolved ManifestBottle objects."""
cache: dict[str, ManifestBottle] = {} cache: dict[str, ManifestBottle] = {}
# Per-bottle effective git-gate.repos, as raw dicts keyed by repo name.
# Threaded alongside `cache` so a child can field-merge against its
# parent's repos without reconstructing them from parsed entries.
repos_cache: dict[str, dict[str, object]] = {}
for name in raws: for name in raws:
if name not in cache: if name not in cache:
_resolve_one_bottle(name, raws, cache, ()) _resolve_one_bottle(name, raws, cache, repos_cache, ())
return cache return cache
@@ -22,6 +26,7 @@ def _resolve_one_bottle(
name: str, name: str,
raws: dict[str, dict[str, object]], raws: dict[str, dict[str, object]],
cache: dict[str, ManifestBottle], cache: dict[str, ManifestBottle],
repos_cache: dict[str, dict[str, object]],
seen: tuple[str, ...], seen: tuple[str, ...],
) -> ManifestBottle: ) -> ManifestBottle:
from .manifest import ManifestBottle, ManifestError from .manifest import ManifestBottle, ManifestError
@@ -41,6 +46,7 @@ def _resolve_one_bottle(
if parent_name_raw is None: if parent_name_raw is None:
bottle = ManifestBottle.from_dict(name, child_raw) bottle = ManifestBottle.from_dict(name, child_raw)
cache[name] = bottle cache[name] = bottle
repos_cache[name] = _resolve_repos_raw({}, child_raw)
return bottle return bottle
if not isinstance(parent_name_raw, str): if not isinstance(parent_name_raw, str):
@@ -60,20 +66,33 @@ def _resolve_one_bottle(
f"bottle '{name}' extends '{parent_name}' which is not " f"bottle '{name}' extends '{parent_name}' which is not "
f"defined. Available bottles: {avail}" f"defined. Available bottles: {avail}"
) )
parent = _resolve_one_bottle(parent_name, raws, cache, seen + (name,)) parent = _resolve_one_bottle(
bottle = _merge_bottles(parent, child_raw, name) parent_name, raws, cache, repos_cache, seen + (name,)
)
merged_repos_raw = _resolve_repos_raw(repos_cache[parent_name], child_raw)
bottle = _merge_bottles(parent, child_raw, merged_repos_raw, name)
cache[name] = bottle cache[name] = bottle
repos_cache[name] = merged_repos_raw
return bottle return bottle
def _merge_bottles( def _merge_bottles(
parent: ManifestBottle, parent: ManifestBottle,
child_raw: dict[str, object], child_raw: dict[str, object],
merged_repos_raw: dict[str, object],
name: str, name: str,
) -> ManifestBottle: ) -> ManifestBottle:
"""Apply PRD 0025 merge rules.""" """Apply PRD 0025 merge rules."""
from .manifest import ManifestBottle, ManifestGitUser from .manifest import ManifestBottle, ManifestGitUser
from .manifest_egress import validate_egress_routes from .manifest_egress import validate_egress_routes
from .manifest_util import as_json_object
# git-gate.repos: when the child declares repos, inject the already
# name-merged repo set (computed by _resolve_repos_raw) so the child
# parses with the full inherited+overridden list (issue #237).
if _child_declares_git_gate_repos(child_raw):
git_raw = as_json_object(child_raw.get("git-gate", {}), "child git-gate")
child_raw = {**child_raw, "git-gate": {**git_raw, "repos": merged_repos_raw}}
# Parse the child's declared fields into a ManifestBottle (with the # Parse the child's declared fields into a ManifestBottle (with the
# usual defaults for anything missing). Validation runs the same # usual defaults for anything missing). Validation runs the same
@@ -92,11 +111,11 @@ def _merge_bottles(
email=child.git_user.email or parent.git_user.email, email=child.git_user.email or parent.git_user.email,
) )
# git-gate.repos: missing means inherit; an explicit empty object # git-gate.repos: when declared, child.git already holds the merged
# clears; otherwise parent and child merge by UpstreamHost with # set (an explicit empty dict clears parent, leaving child.git empty).
# child entries replacing duplicate hosts. # When omitted, the parent's entries are inherited verbatim.
if _child_declares_git_gate_repos(child_raw): if _child_declares_git_gate_repos(child_raw):
merged_git = _merge_git_remotes(parent.git, child.git) if child.git else () merged_git = child.git
else: else:
merged_git = parent.git merged_git = parent.git
@@ -130,6 +149,45 @@ def _merge_bottles(
) )
def _resolve_repos_raw(
parent_repos: dict[str, object],
child_raw: dict[str, object],
) -> dict[str, object]:
"""Compute a bottle's effective git-gate.repos as raw dicts.
Repos are keyed by name. When the child omits git-gate.repos it
inherits the parent's set verbatim; an explicit empty dict clears it.
Otherwise parent and child unite by name, with same-name entries
field-merged (parent fields are defaults, child fields win)."""
from .manifest_util import as_json_object
if not _child_declares_git_gate_repos(child_raw):
return parent_repos
child_repos = _declared_repos_raw(child_raw)
if not child_repos:
return {}
# Parent entries keep their order; child-only names are appended.
names = list(parent_repos) + [n for n in child_repos if n not in parent_repos]
return {
name: {
**as_json_object(parent_repos.get(name, {}), "parent git-gate repo"),
**as_json_object(child_repos.get(name, {}), "child git-gate repo"),
}
for name in names
}
def _declared_repos_raw(child_raw: dict[str, object]) -> dict[str, object]:
"""Return the child's explicitly declared git-gate.repos as raw dicts,
or an empty dict when none are declared."""
from .manifest_util import as_json_object
if not _child_declares_git_gate_repos(child_raw):
return {}
git_raw = as_json_object(child_raw.get("git-gate", {}), "child git-gate")
return as_json_object(git_raw.get("repos", {}), "child git-gate.repos")
def _child_declares_git_gate_repos(child_raw: dict[str, object]) -> bool: def _child_declares_git_gate_repos(child_raw: dict[str, object]) -> bool:
from .manifest_util import as_json_object from .manifest_util import as_json_object
@@ -140,16 +198,6 @@ def _child_declares_git_gate_repos(child_raw: dict[str, object]) -> bool:
return "repos" in git_obj return "repos" in git_obj
def _merge_git_remotes(
parent: tuple[ManifestGitEntry, ...],
child: tuple[ManifestGitEntry, ...],
) -> tuple[ManifestGitEntry, ...]:
by_host = {entry.UpstreamHost: entry for entry in parent}
for entry in child:
by_host[entry.UpstreamHost] = entry
return tuple(by_host.values())
def _merge_egress( def _merge_egress(
parent: ManifestEgressConfig, parent: ManifestEgressConfig,
child: ManifestEgressConfig, child: ManifestEgressConfig,
+3 -4
View File
@@ -92,10 +92,9 @@ class TestSandboxEscape(unittest.TestCase):
"on PATH: curl -sSL https://smolmachines.com/install.sh | sh" "on PATH: curl -sSL https://smolmachines.com/install.sh | sh"
) )
# Throwaway "identity file" so the manifest's _validate_git_entries # Throwaway "identity file" for the git-gate's `identity` field.
# passes (it only checks `os.path.isfile`, not that the content is # It need not be a real SSH key: test 5 reaches gitleaks before
# a real SSH key). Test 5 reaches gitleaks before any SSH attempt # any SSH attempt anyway.
# anyway.
fd, kp = tempfile.mkstemp(prefix="sandbox-test-key.") fd, kp = tempfile.mkstemp(prefix="sandbox-test-key.")
os.close(fd) os.close(fd)
cls._key_path = Path(kp) cls._key_path = Path(kp)
+81 -8
View File
@@ -113,8 +113,8 @@ class TestExtendsEnvMerge(unittest.TestCase):
class TestExtendsGitMerge(unittest.TestCase): class TestExtendsGitMerge(unittest.TestCase):
"""git-gate.user overlays by field; git-gate.repos merges by upstream """git-gate.user overlays by field; git-gate.repos merges by name,
host, with child entries replacing duplicate hosts.""" with same-name child entries merging field-by-field (child wins)."""
_GIT_ENTRY_A = {"url": "ssh://git@host-a/a.git", "key": {"provider": "static", "path": "/dev/null"}} _GIT_ENTRY_A = {"url": "ssh://git@host-a/a.git", "key": {"provider": "static", "path": "/dev/null"}}
_GIT_ENTRY_B = {"url": "ssh://git@host-b/b.git", "key": {"provider": "static", "path": "/dev/null"}} _GIT_ENTRY_B = {"url": "ssh://git@host-b/b.git", "key": {"provider": "static", "path": "/dev/null"}}
@@ -130,19 +130,21 @@ class TestExtendsGitMerge(unittest.TestCase):
names = [e.Name for e in m.bottles["child"].git] names = [e.Name for e in m.bottles["child"].git]
self.assertEqual(["a", "b"], names) self.assertEqual(["a", "b"], names)
def test_child_git_repo_replaces_same_host(self): def test_child_git_repo_different_name_same_host_coexists(self):
replacement = {"url": "ssh://git@host-a/replacement.git", "key": {"provider": "static", "path": "/dev/null"}} # Repos are keyed by Name, not UpstreamHost: two repos with
# different names on the same host both survive the merge.
same_host_b = {"url": "ssh://git@host-a/b.git", "key": {"provider": "static", "path": "/dev/null"}}
m = _build( m = _build(
base={"git-gate": {"repos": {"a": self._GIT_ENTRY_A}}}, base={"git-gate": {"repos": {"a": self._GIT_ENTRY_A}}},
child={ child={
"extends": "base", "extends": "base",
"git-gate": {"repos": {"a2": replacement}}, "git-gate": {"repos": {"a2": same_host_b}},
}, },
) )
entries = m.bottles["child"].git entries = m.bottles["child"].git
self.assertEqual(1, len(entries)) self.assertEqual(2, len(entries))
self.assertEqual("a2", entries[0].Name) names = {e.Name for e in entries}
self.assertEqual("replacement.git", entries[0].UpstreamPath) self.assertEqual({"a", "a2"}, names)
def test_child_omits_git_gate_inherits_full_list(self): def test_child_omits_git_gate_inherits_full_list(self):
m = _build( m = _build(
@@ -164,6 +166,77 @@ class TestExtendsGitMerge(unittest.TestCase):
) )
self.assertEqual((), m.bottles["child"].git) self.assertEqual((), m.bottles["child"].git)
def test_child_same_name_repo_merges_key_field(self):
# Issue #237: child repo with same name as parent should merge
# field-by-field. Child overrides only `key`; parent's url and
# host_key are preserved.
parent_entry = {
"url": "ssh://git@host-a/repo.git",
"host_key": "ecdsa-sha2-nistp256 AAAA",
"key": {"provider": "static", "path": "/keys/id_rsa"},
}
m = _build(
base={"git-gate": {"repos": {"repo": parent_entry}}},
child={
"extends": "base",
"git-gate": {"repos": {"repo": {
"key": {"provider": "gitea", "forge_token_env": "GITEA_TOKEN"},
}}},
},
)
entries = m.bottles["child"].git
self.assertEqual(1, len(entries))
e = entries[0]
self.assertEqual("repo", e.Name)
self.assertEqual("ssh://git@host-a/repo.git", e.Upstream)
self.assertEqual("ecdsa-sha2-nistp256 AAAA", e.KnownHostKey)
self.assertEqual("gitea", e.Key.provider)
self.assertEqual("GITEA_TOKEN", e.Key.forge_token_env)
def test_child_same_name_repo_overrides_url(self):
# Child can override url on a same-name repo; other parent fields
# fall through.
parent_entry = {
"url": "ssh://git@host-a/old.git",
"key": {"provider": "static", "path": "/keys/id_rsa"},
}
m = _build(
base={"git-gate": {"repos": {"repo": parent_entry}}},
child={
"extends": "base",
"git-gate": {"repos": {"repo": {
"url": "ssh://git@host-b/new.git",
"key": {"provider": "static", "path": "/keys/id_rsa"},
}}},
},
)
entries = m.bottles["child"].git
self.assertEqual(1, len(entries))
self.assertEqual("ssh://git@host-b/new.git", entries[0].Upstream)
def test_child_same_name_plus_new_repo(self):
# Same-name repo is field-merged; a distinct new name in child
# is appended.
parent_entry = {
"url": "ssh://git@host-a/repo.git",
"key": {"provider": "static", "path": "/keys/id_rsa"},
}
m = _build(
base={"git-gate": {"repos": {"repo": parent_entry}}},
child={
"extends": "base",
"git-gate": {"repos": {
"repo": {"key": {"provider": "gitea", "forge_token_env": "TOK"}},
"other": self._GIT_ENTRY_B,
}},
},
)
child = m.bottles["child"]
names = {e.Name for e in child.git}
self.assertEqual({"repo", "other"}, names)
repo_entry = next(e for e in child.git if e.Name == "repo")
self.assertEqual("gitea", repo_entry.Key.provider)
def test_child_git_user_inherits_parent_repos(self): def test_child_git_user_inherits_parent_repos(self):
m = _build( m = _build(
base={"git-gate": {"repos": {"a": self._GIT_ENTRY_A}}}, base={"git-gate": {"repos": {"a": self._GIT_ENTRY_A}}},