Better merge behavior for git-gate repos on extends #238
@@ -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
|
||||||
|
|||||||
@@ -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,
|
||||||
|
|||||||
@@ -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)
|
||||||
|
|||||||
@@ -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}}},
|
||||||
|
|||||||
Reference in New Issue
Block a user
This is way too bespoke: I want a more generic deep merge for git-config repos that looks basically like this:
Should use a safe accessor/account for possible values not being there, but this seems like way too many lines for a relatively simple merge
Agreed, this is too bespoke. The clean version is a per-name shallow merge —
git-gate.reposis already{ <name>: <entry> }in the raw manifest, so:drops both
_pre_merge_git_reposand_entry_to_raw, and makes the_merge_git_remoteshost-keying change moot — name is the identity here, so the two-repos-on-one-host collision goes away without special-casing.One wrinkle to flag:
_merge_bottlescurrently receives the parent already parsed (and the parent may itself be a merge result), so there is no parent raw repos dict to merge against — that's the only reason_entry_to_rawexisted. To keep the merge a pure dict overlay I'll thread the resolved repos dict through_resolve_one_bottlerather than reconstructing it from parsed entries. Reworking the PR to that shape and keeping the threeTestExtendsGitMergecases.