fix(macos): review fixes — token on plan, self-heal, symmetric digest, DHCP poll
Addresses findings from a high-effort review of the PRD 0070 macOS backend. Correctness: - Stamp identity_token onto MacosContainerBottlePlan after registration. git's gitconfig extraHeader and the supervise MCP --header read getattr(plan,"identity_token","") at provision time, and both reach the gateway on NO_PROXY (bypassing the egress proxy that carries the token). The plan never carried it, so /resolve fail-closed and every git fetch/push and supervise call from a macOS bottle would have been denied. Registration precedes provision(), so — unlike the run-time env — the plan can carry it. - Self-heal the orchestrator: recreate when it is not (source-current AND answering /health), not on the source-hash label alone. A container running current code but with a wedged HTTP server was left alone and polled to death, failing every launch until manual deletion. - image_digest and container_image_digest now read the same descriptor.digest field; dropped image_digest's id/tag fallback that could yield a value the container side can't produce — a permanent mismatch would have recreated the shared gateway on every launch (severing every live bottle's egress, since the replacement gets a new DHCP address). - Poll for the agent's and gateway's DHCP address instead of a fatal read right after `container run` (there is no --ip; the address can lag start). Cleanup: - One _inspect_first + _descriptor_digest behind the four inspect readers. - Shared bind_mount_spec (util) and host_db_dir (paths) replace per-module copies; _GIT_HTTP_PORT now imports git_http_backend.DEFAULT_PORT. - Drop the dead _url cache / url property and the write-only agent_proxy_url. Deferred (noted on the PR, not fixed here): the gateway image rebuilding on every launch (needs source-hash-labeled build), SQLite shared across VM guests, and the sh -lc profile-override edge — each is design-level or behavior-risk beyond a review fix. Verified: real Apple Container bring-up is green and idempotent; 1826 unit tests pass with `container` absent (CI parity), pyright clean, pylint 9.86. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -158,6 +158,36 @@ class TestIdentityTokenDelivery(unittest.TestCase):
|
||||
argv = bottle.agent_argv(["--help"], tty=False)
|
||||
self.assertNotIn("--env", argv)
|
||||
|
||||
|
||||
class TestPlanIdentityToken(unittest.TestCase):
|
||||
"""git-gate's gitconfig extraHeader and the supervise MCP --header read
|
||||
`getattr(plan, "identity_token", "")` at provision time and both bypass the
|
||||
egress proxy (NO_PROXY), so the exec-time proxy token never reaches them —
|
||||
the plan must carry the token or /resolve fail-closes and git + supervise
|
||||
are denied on macOS."""
|
||||
|
||||
def test_macos_plan_has_the_identity_token_field(self) -> None:
|
||||
from dataclasses import fields
|
||||
|
||||
from bot_bottle.backend.macos_container.bottle_plan import (
|
||||
MacosContainerBottlePlan,
|
||||
)
|
||||
names = {f.name for f in fields(MacosContainerBottlePlan)}
|
||||
self.assertIn("identity_token", names)
|
||||
|
||||
def test_matches_the_docker_plan(self) -> None:
|
||||
"""Both consolidated backends must expose identity_token so a shared
|
||||
provision-time consumer degrades to neither backend silently."""
|
||||
from dataclasses import fields
|
||||
|
||||
from bot_bottle.backend.docker.bottle_plan import DockerBottlePlan
|
||||
from bot_bottle.backend.macos_container.bottle_plan import (
|
||||
MacosContainerBottlePlan,
|
||||
)
|
||||
docker = {f.name for f in fields(DockerBottlePlan)}
|
||||
macos = {f.name for f in fields(MacosContainerBottlePlan)}
|
||||
self.assertIn("identity_token", docker & macos)
|
||||
|
||||
def test_provisioning_exec_also_carries_the_token(self) -> None:
|
||||
"""`provision` runs through `exec`; a provider whose provision step
|
||||
fetches anything would otherwise egress token-less and be denied."""
|
||||
|
||||
@@ -273,5 +273,55 @@ resolver #2
|
||||
)
|
||||
|
||||
|
||||
def _completed(stdout: str, returncode: int = 0):
|
||||
return util.subprocess.CompletedProcess(args=[], returncode=returncode, stdout=stdout, stderr="")
|
||||
|
||||
|
||||
class TestInspectDigests(unittest.TestCase):
|
||||
"""image_digest and container_image_digest must read the SAME field shape
|
||||
(a `descriptor.digest`) so a running container and its image are
|
||||
comparable. An asymmetry recreates the gateway on every launch."""
|
||||
|
||||
_IMAGE = '{"configuration": {"descriptor": {"digest": "sha256:abc123"}}, "id": "abc123"}'
|
||||
_CONTAINER = '[{"configuration": {"image": {"descriptor": {"digest": "sha256:abc123"}}}}]'
|
||||
|
||||
def test_image_and_container_digests_agree_for_the_same_image(self):
|
||||
with patch.object(util, "run_container_argv") as run:
|
||||
run.return_value = _completed(self._IMAGE)
|
||||
img = util.image_digest("bot-bottle-gateway:latest")
|
||||
run.return_value = _completed(self._CONTAINER)
|
||||
ctr = util.container_image_digest("bot-bottle-mac-gateway")
|
||||
self.assertEqual("abc123", img)
|
||||
self.assertEqual(img, ctr)
|
||||
|
||||
def test_image_digest_never_falls_back_to_a_tag_or_id(self):
|
||||
"""The old `id` fallback could yield a value container_image_digest
|
||||
can't produce, permanently mismatching. Missing descriptor → '' (don't
|
||||
churn), never a stray id/tag."""
|
||||
with patch.object(util, "run_container_argv") as run:
|
||||
run.return_value = _completed('{"id": "bot-bottle-gateway:latest"}')
|
||||
self.assertEqual("", util.image_digest("bot-bottle-gateway:latest"))
|
||||
|
||||
def test_unreadable_inspect_returns_empty(self):
|
||||
with patch.object(util, "run_container_argv") as run:
|
||||
run.return_value = _completed("", returncode=1)
|
||||
self.assertEqual("", util.image_digest("x"))
|
||||
self.assertEqual("", util.container_image_digest("x"))
|
||||
self.assertEqual({}, util.container_env("x"))
|
||||
|
||||
|
||||
class TestWaitContainerIpv4(unittest.TestCase):
|
||||
def test_returns_address_once_dhcp_assigns_it(self):
|
||||
with patch.object(util, "try_container_ipv4_on_network", side_effect=["", "", "192.168.128.4"]), \
|
||||
patch.object(util.time, "sleep"):
|
||||
ip = util.wait_container_ipv4_on_network("c", "net", timeout=5, poll=0)
|
||||
self.assertEqual("192.168.128.4", ip)
|
||||
|
||||
def test_returns_empty_on_timeout(self):
|
||||
with patch.object(util, "try_container_ipv4_on_network", return_value=""), \
|
||||
patch.object(util.time, "sleep"):
|
||||
self.assertEqual("", util.wait_container_ipv4_on_network("c", "net", timeout=-1))
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
@@ -194,6 +194,28 @@ class TestMacosOrchestratorService(unittest.TestCase):
|
||||
svc.ensure_running()
|
||||
run.assert_not_called()
|
||||
|
||||
def test_wedged_but_current_orchestrator_is_recreated(self) -> None:
|
||||
"""A container running the current code but whose HTTP server is dead
|
||||
must be recreated, not left alone and polled to death forever. Checking
|
||||
the source-hash label alone (without health) would strand the host."""
|
||||
svc = MacosOrchestratorService(repo_root=Path("/r"), host_root=Path("/h"))
|
||||
run = Mock()
|
||||
# Unhealthy on the pre-check, healthy once recreated + waited.
|
||||
health = Mock(side_effect=[False, True])
|
||||
with patch(f"{_ORCH}.container_mod") as mod, \
|
||||
patch(f"{_ORCH}.source_hash", return_value="h1"), \
|
||||
patch.object(svc, "_run_orchestrator_container", run), \
|
||||
patch.object(svc, "_ensure_orchestrator_image"), \
|
||||
patch.object(svc, "gateway"), \
|
||||
patch.object(svc, "is_healthy", health):
|
||||
mod.container_is_running.return_value = True
|
||||
mod.inspect_container.return_value = {
|
||||
"configuration": {"labels": {"bot-bottle-orchestrator-source-hash": "h1"}}
|
||||
}
|
||||
mod.try_container_ipv4_on_network.return_value = "192.168.128.2"
|
||||
svc.ensure_running()
|
||||
run.assert_called_once()
|
||||
|
||||
def test_changed_source_recreates_the_orchestrator(self) -> None:
|
||||
"""The control-plane process loaded its bind-mounted source at startup
|
||||
and won't reload it."""
|
||||
|
||||
Reference in New Issue
Block a user