FROM-scratch images (commit_container output) and registry images built
for reproducibility often omit the created field entirely. Both backends
were calling die() in that case, crashing the cached-image quickstart
before any container started.
image_created_at now returns datetime | None — None when the field is
absent or unparseable, die() only on real inspect failures (non-zero
exit, malformed JSON). stale_checks in both backends skips the staleness
check when None is returned.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- config_store.py imported host_db_path from supervise_types (wrong);
it lives in paths.py, matching all other stores (audit, queue, etc.)
- test_stale_checks: two tests expected check_stale called twice
(agent + sidecar) — consolidated arch has no sidecar, so once is
correct; update assertions and remove unused Path import
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pyright strict reportUnusedImport flagged BottleImages in
test_docker_launch_committed_image.py and test_macos_container_launch.py;
neither file references the type by name (they only use the returned
value's attributes).
Addresses review comments 3098, 3099, 3100 on PR #336:
- Add BottleImages(agent, sidecar) dataclass to backend/__init__.py.
Docker/macOS backends use str image refs; smolmachines uses Path
artifacts. Replaces the singular `image` variable from the canonical
pattern in comment 3100.
- Replace _image_stale_checks/skip_stale with public prelaunch_checks().
CLI now calls backend.prelaunch_checks(plan) before backend.launch(plan);
if StaleImageError is raised and the operator confirms, launch proceeds
without re-checking. Removes the while-True/skip_stale retry loop.
- Add abstract _build_or_load_images(plan) -> BottleImages to
BottleBackend. launch() calls it then passes images to _launch_impl.
Each backend implements both methods.
- Fix comment 3098 (macos-container): _build_images is removed.
build_or_load_images() has separate fresh/cached code paths — the
cached path never calls a build helper.
- Update _start_bundle (smolmachines) to accept sidecar_artifact: Path
directly. Sidecar artifact resolution moves to _sidecar_from_path(),
called by build_or_load_images alongside _agent_from_path().
- image_cache: StaleImageError exception + check_stale/check_stale_path (raise instead of warn)
- BottleBackend.launch: template method (skip_stale flag) that calls _image_stale_checks then _launch_impl
- Each backend: _image_stale_checks delegates to a stale_checks() function in its launch module; _launch_impl replaces launch override
- macos_container: adds image_created_at to util, cached-image support in _build_images, stale_checks
- cli/start.py: catches StaleImageError, prompts interactively, retries with skip_stale=True; headless mode dies on it
Review follow-ups for the infra candidate-artifact flow:
1. publish-infra would fail on the first merge to main. The rootfs version now
hashes the dropbear bytes, and build-infra sets BOT_BOTTLE_FC_DROPBEAR to the
staged dropbear — but publish-infra (ubuntu-latest) set none, so _publish_
bundle re-derived the version with a "<missing>" dropbear and rejected the
candidate as "does not match checkout". Download the same firecracker-inputs
dropbear and export BOT_BOTTLE_FC_DROPBEAR in publish-infra (and add
stage-firecracker-inputs to its needs, since it now consumes that artifact).
2. Guard stage-firecracker-inputs with the same fork-PR check as the other
KVM-runner jobs, so a fork PR can't spin the privileged runner (it only
copies a static binary, but keep the posture consistent; it gates the whole
Firecracker chain via needs).
3. ensure_artifact_gz / _publish_bundle read version.txt before checking it
exists — a bundle missing it raised a raw FileNotFoundError instead of the
intended "bundle is incomplete" die. Guard the read (kept before the gz/sha
completeness check so a wrong-version bundle still reports the version
mismatch, not "incomplete").
4. test_infra_artifact.setUp didn't isolate BOT_BOTTLE_INFRA_ARTIFACT_DIR, so
an ambient candidate dir (the coverage job exports one) would send the
registry-pull tests down the local-bundle path. Pin it off in setUp; the
candidate-path cases set it explicitly.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A9qa3xoavjQScufDfZaXKR
The coverage CI job exports BOT_BOTTLE_INFRA_BUILD=local so the integration
suite builds infra/agent images from source. But that job also runs the unit
suite, and the ambient =local leaked into test_default_pulls_artifact_without_
docker — it drove ensure_built() down the local Docker-build branch, so the
"build_image not called / artifact pulled" assertion failed (build_image
called 3 times).
Pin the var off in that test so it exercises the default pull path regardless
of the runner's ambient env (mirrors test_local_mode_builds_deps_before_infra,
which pins it on). Full unit suite verified green under BOT_BOTTLE_INFRA_BUILD
=local.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A9qa3xoavjQScufDfZaXKR
Dockerfile.gateway COPYs pyproject.toml into /src and runs pip install
/src, so it is a real input to the baked rootfs. A dependency-only change
previously reused stale artifact versions, potentially booting a rootfs
whose installed packages differed from the current checkout.
Also adds _fake_repo fixture support and a regression test so this input
can't silently drop out of the hash again.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two lint fixes, both test-only (no effect on the infra artifact version):
- annotate the TestAdoptable / TestKillInfraFirecrackers helper params
(reportMissingParameterType).
- test_docker_test_helpers: read __unittest_skip__ via getattr on the
dynamically-built Case type, matching the sibling assertion — pyright can't
see the attribute the skip decorator adds (reportAttributeAccessIssue).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A9qa3xoavjQScufDfZaXKR
build_agent_rootfs_dir cached the built rootfs by Dockerfile content alone,
but util.inject_guest_boot then writes util._GUEST_INIT into it. So a fix to
the init — making /tmp world-writable (1777) so the agent can create scratch
dirs / git worktrees there — did NOT bust the cache: the KVM runner kept
reusing a stale agent-<dockerfilehash> rootfs built with the old init, and the
sandbox-escape README-push test kept failing at `git init /tmp/...` with
"Permission denied".
Key the cache on Dockerfile content AND the injected init (_rootfs_digest), so
an init change rebuilds. Self-busting: the new key yields a fresh cache dir, so
no manual cache clear on the runner.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A9qa3xoavjQScufDfZaXKR
The KVM CI runner now sets BOT_BOTTLE_FC_* for its isolated pool (distinct
iface prefix / orch iface / nft table / IP base), and that env leaks into the
coverage job's test process. Five netpool tests hardcoded the default names
(bbfc*, bot_bottle_fc) and so failed there with e.g. ['bbfc1'] != ['bbci1'].
Assert against netpool's env-driven config instead — slot(i).iface / the
configured prefix — so the tests check the LOGIC regardless of which pool the
host is configured for. The single-source test now compares the parsed
defaults (netpool._DEFAULTS, env-independent) for the module constants, since
IFACE_PREFIX/NFT_TABLE legitimately layer an env override on top.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A9qa3xoavjQScufDfZaXKR
The infra VM is a per-host singleton that outlives short-lived launchers, so
`ensure_running` adopts it when its control plane is healthy. But it adopted
ANY healthy VM regardless of the code that built it — so after an infra-code
change the old VM kept being adopted and the new code never booted. The only
way to dislodge it was an out-of-band `kill`, which then raced whatever
launched next. On CI this meant every infra change needed a manual VM kill.
Make adoption version-aware:
* boot records the infra-artifact version it booted from in a `booted-version`
marker beside the singleton; `stop` clears it.
* `ensure_running` adopts only when the marker matches the current version
(`_adoptable`); a missing/mismatched marker falls through to stop + reboot.
So a stale VM is replaced automatically on the next launch — no manual kill,
and it's concurrency-safe (the reboot happens under the singleton flock).
Also harden teardown: the PID file drifts after crashes / out-of-band kills,
so `stop` now also reaps any orphaned firecracker still bound to the infra
config path (scoped to that path, so interactive-pool VMs are untouched) —
otherwise a survivor holds the orchestrator TAP and the fresh boot dies with
"tap … Resource busy".
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A9qa3xoavjQScufDfZaXKR
The rootless agent rootfs build can land /tmp as 0755/root-owned, so the
agent (uid 1000 node) can't create scratch dirs there. The sandbox-escape
suite's README-push test does `cd /tmp && git init sandbox-escape-repo` and
died with "cannot mkdir sandbox-escape-repo: Permission denied" — before the
git-gate gitleaks hook could run — so the test read it as a missing hook.
On docker the agent inherits node:22-slim's 1777 /tmp, which is why only the
Firecracker path was affected (and only now that the suite runs end-to-end).
Set /tmp to 1777 in the guest PID-1 init, so every agent VM boots with a
usable /tmp regardless of rootfs perm drift.
Also update the infra-init unit test to assert the gateway launches via the
`bot_bottle.gateway_init` module (matching the prior fix), not a file path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A9qa3xoavjQScufDfZaXKR
Two diff-coverage gaps on the ci-kvm-runner branch:
1. bot_bottle/backend/docker/setup.py: the try/except TimeoutExpired
block added in a prior commit had no tests reaching the subprocess
path. Add two tests to TestDockerSetupStatus: one for the success
path (subprocess returns 0) and one for the TimeoutExpired fallback.
2. bot_bottle/db_store.py: the _connection() context manager change in
is_migrated() was never exercised by unit tests (all callers mock
is_migrated() directly). Add test_db_store.py covering the absent-DB,
missing-schema-table, migrated, and behind-schema cases.
On a self-hosted KVM runner the process has a real controlling terminal
so name_color_modal successfully opens /dev/tty and enters a curses
loop waiting for keyboard input, hanging the test indefinitely.
Docker containers (ubuntu-latest runners) don't have a real /dev/tty,
causing an OSError that triggers the existing fallback — this is why
the hang was invisible in ubuntu-latest CI.
Also add timeout=5 to _daemon_reachable() to match the same defensive
fix already applied to docker_available() in tests/_docker.py.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Docker integration tests are already covered by the integration-docker
job on ubuntu-latest. On the KVM runner the Firecracker TAP/nftables
pool conflicts with Docker networking, causing those tests to hang
and the coverage job to never complete.
Add SKIP_DOCKER_TESTS env-var support to docker_available() and set
it for the integration phase of coverage.sh so only Firecracker
integration tests run there.
On the self-hosted KVM runner Docker is on PATH but the daemon socket
is unreachable (firewalled/dropped). subprocess.run(["docker", "info"])
with no timeout hangs indefinitely on a dropped connection, stalling the
coverage job for hours — one hang per @skip_unless_docker()-decorated
class, ~8 per integration suite run.
Add timeout=5 with a TimeoutExpired → False fallback so the check
resolves quickly to "unreachable" rather than blocking.
The sandbox-escape test was unconditionally skipped when GITEA_ACTIONS=true,
which prevented Firecracker orchestration coverage from being measured even
when BOT_BOTTLE_BACKEND=firecracker is set on the KVM runner.
Narrow the skip to: GITEA_ACTIONS=true AND BOT_BOTTLE_BACKEND != firecracker.
When BOT_BOTTLE_BACKEND=firecracker the test is explicitly opted in to run on
the self-hosted KVM runner where the required /dev/kvm + TAP pool exist.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
NixOS doesn't populate /bin (no /bin/sleep), so the gateway-init
end-to-end tests that spawn a real `sleep` errored with
FileNotFoundError. Add tests/_bin.py with a PATH-resolved SLEEP
constant (falling back to /bin/sleep on FHS hosts) and import it in
test_gateway_init.py instead of hardcoding the path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A9qa3xoavjQScufDfZaXKR
Closes#412.
The supervise MCP server blocked the agent's tool call polling for the
operator's decision, and on timeout returned `status: pending` with no
proposal id and no way to poll a specific proposal — so the only way to
learn a late decision was to re-propose (a duplicate).
- `handle_tools_call` pending timeout now returns the `proposal_id` and
points the agent at `check-proposal`.
- New `check-proposal` MCP tool: non-blocking status lookup by proposal id
(pending | approved | modified | rejected | unknown). Reuses the queue's
FileNotFoundError semantics; archives a decided proposal exactly like the
synchronous path, so a pending proposal stays visible to the operator
until it's both decided and polled.
- `TOOL_CHECK_PROPOSAL` constant, re-exported from supervise; kept out of
TOOLS since it never becomes a Proposal.tool.
Enforcement is unchanged — the tools only propose policy; the egress proxy
and git-gate still enforce — so returning early opens no hole. Follow-ups
(git-gate reject-requeue, backpressure, notifications, web console) are in
the PRD.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YBCHap11yGAKuKfsehNPaD
The pre-receive hook scanned existing-branch updates with the delta range
$old..$new. On a rebase / non-fast-forward force-push onto an advanced main,
$old is no longer an ancestor of $new, so $old..$new expands to all of main's
new history — including the deliberate sandbox-escape gitleaks fixtures — and
the push is rejected on commits that belong to main, not the branch.
Unify the range on `$new --not --all` for every non-delete push (this is the
deferred open question from PRD 0028, which already applied it to new refs
for #106). It scans only the commits the push introduces and is
security-equivalent: the bare repo's refs come only from trusted upstream
mirror-fetch and gitleaks-gated pushes, so an excluded commit is
already-upstream or already-scanned. It is also more correct for
non-fast-forward pushes, where $old..$new can skip commits off the direct path.
Fixes#421
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S1qRZTJC6qgBsUSjNrBdkX
`backend setup/status/teardown` manage host prerequisites only and never
open the store, but the CLI dispatcher ran the schema-migration gate before
every command. On a non-TTY runner the gate's `Migrate now? [y/N]` prompt
reads EOF and refuses, so `backend status --backend=firecracker` exits 1 —
breaking the Firecracker CI preflight on any host without a pre-migrated DB.
Exempt `backend` from the gate; store-touching commands stay gated.
Fixes#419
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S1qRZTJC6qgBsUSjNrBdkX
- Rename _BACKENDS → _backends: pyright treats uppercase module-level
names as constants and flags the reassignment in _get_backends() as
reportConstantRedefinition; lowercase avoids this.
- Add TYPE_CHECKING guard importing CommitCancelled/Freezer/get_freezer
from .freeze: pyright cannot see module-level __getattr__ bindings, so
reportUnsupportedDunderAll fired for those three __all__ entries; the
guard makes them visible to the type checker without running at import
time.
- Update test_backend_selection.py to patch _backends (lowercase).
Importing backend.docker.util previously triggered eager loading of all
three backend packages (~76 modules) because backend/__init__.py imported
DockerBottleBackend, FirecrackerBottleBackend, and MacosContainerBottleBackend
at module scope. This made the module prohibitively expensive to import
from the orchestrator layer and elsewhere.
The three backend imports are now deferred into _get_backends(), which
loads all three on first call and caches the result in the module-level
_BACKENDS variable (initially None). Module-level __getattr__ exposes
backend classes and freeze symbols lazily for existing import/patch sites.
backend/docker/util.py raw subprocess.run(["docker", ...]) calls are
replaced with the shared run_docker primitive from docker_cmd, eliminating
the duplication between the backend and orchestrator implementations.
_silent_run() is removed; image_exists() is inlined directly onto
run_docker. The commit_container test is updated to patch run_docker
instead of subprocess.run.
Install bot_bottle via pip in Dockerfile.gateway instead of COPYing
individual .py files flat under /app/. This eliminates the try/except
import shims in egress_addon_core, dlp_detectors, egress_addon,
supervise, supervise_server, and git_http_backend that existed only
to support the flat-bundle layout.
Adds bot_bottle/constants.py as a single source of truth for
IDENTITY_HEADER and GIT_GATE_TIMEOUT_SECS, removing the duplicated
literal definitions in egress_addon.py, supervise_server.py,
git_http_backend.py, and git_gate_render.py.
Test files updated to match: test_supervise_server.py drops the
sys.path.insert hack in favour of direct package imports; the
egress_addon test shims no longer pre-populate sys.modules with a
bare egress_addon_core alias.
b1850be's fail-closed-503 test (rebased in from main) built a git-http
server with a flat repo.git and no policy_resolver. The resolver-only
data plane on this branch denies an unattributed request with 404 before
it reaches the access-hook path the test exercises, so it saw 404 != 503.
Nest the bare repo under <root>/<_BID>/ and set _FixedResolver(_BID) on
the server, matching every other test in this module, so the request is
attributed and reaches the access-hook (mocked to raise PermissionError)
that the 503 fail-closed behavior guards.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ
The egress_entrypoint.sh fail-closed guard (this branch) exits 1 when
BOT_BOTTLE_ORCHESTRATOR_URL is unset, which broke the argv-construction
tests that ran the script without it. Set the URL in the shared
_run_entrypoint helper (a precondition for reaching mitmdump now, like
PATH) and add a test asserting the guard fails closed when it's absent.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ
Follow-ups from the #402 review of the single-tenant data-plane teardown.
- egress_entrypoint.sh: refuse to launch mitmdump when
BOT_BOTTLE_ORCHESTRATOR_URL is unset, so the fail-closed guarantee no
longer rests solely on mitmproxy's errorcheck addon exiting on the
addon's load-time raise. A misconfigured gateway can never come up as
a bare TLS-bumping open proxy with no policy.
- orchestrator/gateway.py: ensure_running() raises GatewayError on an
empty orchestrator URL — a URL-less launch would only crash-loop the
now-resolver-only daemons (egress raises, git-http exits 1, supervise
exits 2). The env-injection branch is now unconditional.
- Drop stale "single-tenant" / "reads routes.yaml" comments in
gateway.py and egress_entrypoint.sh, and the /etc/egress/routes.yaml
layout line in Dockerfile.gateway.
- Tests: gateway fixtures supply an orchestrator URL; add a
refuse-without-URL test and assert the URL env is injected.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ
All three backends (docker, firecracker, macos-container) now launch through
the consolidated orchestrator, and every production gateway sets
BOT_BOTTLE_ORCHESTRATOR_URL — so the legacy single-tenant (`resolver is None`)
branches in the shared gateway's data plane were unreachable dead code, a second
security-relevant path to keep correct in parallel with the live one. Make the
orchestrator resolver mandatory and delete the single-tenant paths from the
three data-plane modules.
egress_addon.py: drop the static routes file entirely — EGRESS_ROUTES, _reload,
the SIGHUP handler, self.config, and the SUPERVISE_BOTTLE_SLUG env slug. The
per-request /resolve is the only policy source; __init__ fail-closes if
BOT_BOTTLE_ORCHESTRATOR_URL is unset. Introspection (`_egress.local/allowlist`)
now reports the calling bottle's *resolved* routes. The block/redact log gates
and _req_ctx redaction now read the per-flow config/env from the request-time
stash, so they use each bottle's log level and token set (they silently used the
empty static config before). Nothing sends `docker kill --signal HUP` to the
gateway in the consolidated model (the egress applicators fail closed), so
removing the SIGHUP reload is safe.
git_http_backend.py: resolver mandatory; no flat-root fallback. main() refuses
to start without an orchestrator URL; a request whose source resolves to no
bottle 404s.
supervise_server.py: resolver mandatory; every proposal is attributed to the
source-IP-resolved bottle. Remove handle_list_egress_routes (the proxy-fetch
introspection that only worked when the proxy carried one bottle's identity) —
list-egress-routes is answered from the resolved policy. main() refuses to start
without an orchestrator URL.
Tests: a host-side fake resolver serves each test's Config through the real
parse path (a small YAML-subset emitter round-trips route_to_yaml_dict); the
response/websocket hooks stash it as request() would. Deletes the tests for the
removed static-config, SIGHUP-reload, and single-tenant-passthrough paths; adds
fail-closed-without-orchestrator coverage.
Follow-up: gateway_init still forwards SIGHUP to the egress child (now dormant —
no one sends it); the README still describes the docker backend's per-bottle
topology. Both are outside the data-plane teardown.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ
Cloning/fetching from the git-gate on the Apple-container backend failed with
"empty reply from server" (curl exit 52). Root cause: the git-http handler
crashed on every upload-pack with
PermissionError: [Errno 13] Permission denied: '/etc/git-gate/access-hook'
The access-hook is exec'd directly, so it needs the x bit. prepare() stages it
0o700 and trusted the gateway copy to carry that mode. `docker cp` does; the
Apple `container cp` (AppleGatewayTransport) does not, landing the hook 0o644 →
EACCES. The unhandled exception killed the handler thread, closing the socket
with no HTTP response — which the client sees as the opaque empty reply.
- provision_git_gate now `chmod +x`es the access-hook on the gateway side after
the copy, so it's executable under every transport (docker/apple/firecracker).
- git-http handler wraps the access-hook subprocess.run: an OSError /
SubprocessError (un-execable, timed out) now fails closed with a 503 instead
of crashing the thread into an empty reply — a gate that can't run its hook
should deny, visibly.
- Updates the now-misleading "docker cp preserves source mode" comment in
git_gate.prepare().
Regression tests: provisioning applies +x to the access-hook; the handler
returns 503 (not an empty reply) when the hook can't be exec'd.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Self-review of this PR: making `response()` run in the consolidated gateway
also activates its `LOG_FULL` `_log_response` call there — previously
unreachable, since the empty static config made `response()` return early. That
logger redacted with `os.environ`, which in multi-tenant mode does NOT hold the
bottle's per-request `/resolve` tokens (only the resolved `env` overlay does),
so a non-token-shaped provisioned secret appearing in a response could be logged
in the clear.
Thread the resolved per-flow `env` into `_log_request` / `_log_response` so the
LOG_FULL redaction scrubs the calling bottle's secrets. Adds a regression test
(a non-token-shaped `/resolve` secret, absent from os.environ, must not appear
in the response log) and updates the redaction-test helpers for the new arg.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ
In the consolidated (multi-tenant) gateway the addon's static `self.config`
is empty — each request's real policy comes from the per-request `/resolve`.
`response()` and `websocket_message()` still matched routes against that empty
config, so inbound prompt-injection DLP and WebSocket credential/injection DLP
silently skipped every scan (fail-open) whenever the gateway ran multi-tenant.
This is backend-agnostic: the gateway image (and this addon) is shared by the
Firecracker, macOS, and docker consolidated backends.
Resolve the per-flow (config, slug, env) once in `request()`, stash it on
`flow.metadata`, and have both hooks read it back — falling back to the static
single-tenant values for a flow that never passed through `request()`. Reusing
the request's one `/resolve` avoids a round-trip per response and per WebSocket
frame.
Adds multi-tenant regression tests for both hooks that fail against the old
fall-open behaviour.
Refs: audit issue #400 (finding #2)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ
Addresses the 5 lower-priority findings left as follow-up in the earlier
review, now that each has a concrete answer:
- Add gateway_name: str = GATEWAY_NAME to OrchestratorService.__init__
(mirrors the existing orchestrator_name param) and thread it through
_gateway(). Deletes the test's _IsolatedOrchestratorService subclass,
which existed only to override a private method for this one kwarg —
any caller needing gateway-name isolation can now use the public
constructor. Backward compatible: every existing caller constructs
OrchestratorService with keyword args and a sensible default is kept.
- Give the test its own fixed image tags (bot-bottle-orchestrator:itest,
bot-bottle-gateway:itest) instead of the production :latest ones.
_running_image_is_current() keys gateway staleness off the image tag's
ID, not per-instance identity, so rebuilding the shared :latest tag from
whatever's on disk during a test run could make a real host's running
production gateway look stale and get force-recreated. Fixed tags (not
per-run-suffixed, so they don't accumulate) fully decouple the two.
- setUp -> setUpClass/tearDownClass: all 5 tests are read-only checks
against the same running control plane, so one shared container
lifecycle replaces 5 (each of which paid its own container-start +
image-build + health-poll cycle). Cuts the file's wall-clock roughly
4x (11.5s -> 2.9-4.3s) and, combined with the network-rm cleanup from
the previous commit, means one cleanup instead of five.
- Reuse OrchestratorClient (bot_bottle/orchestrator/client.py) instead of
a hand-rolled urllib helper — the test now exercises the same
request/response code path the real host CLI uses, rather than a
private copy that could silently drift from it.
- Add the chown workaround test_multitenant_isolation.py already needed
for this exact bind-mount: the orchestrator container has no USER
directive, so it writes the registry DB as root into the throwaway
host_root; chown it back before tempdir cleanup so that doesn't raise
PermissionError on native Linux Docker (no UID remap, unlike Docker
Desktop's macOS VM).
Verified: ran the suite twice in a row (idempotency — fixed image tags
don't accumulate, 5/5 pass both times, 2.99-4.33s each), the real
~/.bot-bottle/control-plane-token is untouched, zero leaked networks or
containers after either run, exactly 2 :itest images (not growing), the
full orchestrator unit suite (93 tests) and the sibling docker
gateway/broker integration tests still pass. pyright clean, pylint
10.00/10 on both changed files.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pushing the previous two fixes surfaced a real, currently-failing CI run
(gitea actions run 2164, jobs "integration" and "coverage"): every one of
the 5 new tests errored in setUp with
docker: Error response from daemon: error while creating mount source
path '/workspace/didericis/bot-bottle': mkdir /workspace: read-only
file system
This is finding #4 from the review of the previous commit, now confirmed
live rather than just plausible: act_runner's job-container topology
can't satisfy the orchestrator's host-path bind mount, the same
constraint test_multitenant_isolation.py, test_gateway_image.py, and
test_sandbox_escape.py already skip around. Add the identical
skip_unless_docker + GITEA_ACTIONS guard so this test degrades the same
way its siblings do instead of failing the job.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two bugs surfaced by a review of the previous commit:
- host_control_plane_token() resolves its path via the ambient
BOT_BOTTLE_ROOT env var, not the host_root kwarg passed to
OrchestratorService (that kwarg only controls the DB bind-mount
destination). The test's isolation claim was false as a result: running
it read/wrote the developer's real ~/.bot-bottle/control-plane-token
instead of the throwaway temp dir — confirmed directly on disk. Fixed
by pointing the env var at the same temp dir for the test's duration
and restoring it via addCleanup.
- ensure_running() creates a per-bottle Docker network but stop() only
ever removes containers, never the network — every run of this test
leaked one bridge network permanently (found and removed 5 from prior
runs via `docker network ls`). Fixed with an explicit `docker network
rm` in addCleanup.
Verified: re-ran the suite twice: 5/5 pass both times, the real
~/.bot-bottle/control-plane-token timestamp is unchanged across both runs
(proving isolation), and `docker network ls` shows zero leaked
bot-bottle-net-itest-* networks afterward. pyright clean, pylint 10.00/10.
Remaining findings from the same review (missing GITEA_ACTIONS skip
guard, root-owned bind-mount cleanup on native Linux, no setUpClass,
reinvented OrchestratorClient, gateway_name should be a constructor
param rather than a subclassed private-method override) are left for a
follow-up — each is a real, separate design/scope call, not a
mechanical fix like these two.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ca91fc4 fixed the control plane's missing caller authentication but only
verified it end-to-end on Apple Container; Docker had the same code change
(docker_cmd.py's env injection) backed only by the in-process dispatch()
unit tests, which never exercise the real HTTP server or a real container.
Ran it for real first: brought up the actual orchestrator + gateway via
Docker, hit the published control-plane port directly. Confirms /health
stays open, /bottles and /resolve both 401 with no token or a wrong one
(the enumeration and credential-lift vectors from #400), and /bottles
succeeds with the real per-host secret.
Added as a proper integration test so this stays covered: a subclass of
OrchestratorService giving the gateway its own unique name too (the base
class only parameterizes the orchestrator's), plus a throwaway
BOT_BOTTLE_ROOT and a random port, so a run can never collide with a real
host's orchestrator or gateway. Gated on a reachable Docker daemon, same
as the existing docker gateway/broker integration tests.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The control plane had no caller authentication — dispatch() routed every
request straight through, and the only guards anywhere were the
(source_ip, identity_token) checks inside /resolve and /attribute, which
authenticate the *bottle* a request is about, not the *caller*. On the
consolidated backends the agent shares a network with the control-plane port
(macOS: the infra container runs 8099 on the agent's --internal network;
docker: the orchestrator container sits on the shared gateway network), so a
prompt-injected agent could GET /bottles (enumerate siblings + policies),
PUT /bottles/<id>/policy (rewrite its own egress allowlist to allow-all),
POST /resolve (read the injected upstream credentials it's never meant to
see), and POST /supervise/respond (self-approve its own proposals). Every
README guarantee collapsed to the agent's discretion.
Fix: a per-host control-plane secret required on every route but GET /health,
compared with hmac.compare_digest. It is held only by the trusted callers and
never handed to an agent:
- minted + persisted 0600 at <root>/control-plane-token (paths.host_control_plane_token);
- injected as $BOT_BOTTLE_CONTROL_PLANE_TOKEN into the orchestrator + gateway
containers via bare `--env NAME` (value inherited from the launch process,
so it never lands on argv or in `container/docker inspect`);
- presented by the gateway's PolicyResolver (reads the env) on /resolve, and by
the host CLI's OrchestratorClient (reads the host file) on every call.
The agent container is never given the env var or the host file, so from a
bottle every /bottles*, /resolve, /attribute, and /supervise/* call now
returns 401 — closing the enumeration, allowlist-rewrite, credential-lift, and
self-approval. The existing (source_ip, identity_token) checks stay as
defense-in-depth.
Enforced when configured: macOS + docker inject the secret (→ enforced). With
no secret set the server runs open and warns loudly at startup — a
fail-visible fallback for the unit suite and for Firecracker, whose
port-scoped nft already blocks agents from 8099 (wiring the secret into its
infra-VM init is a clean fast-follow, left out here to avoid churning the
prebuilt-artifact hash).
Verified end-to-end on real Apple Container: infra comes up healthy, the host
CLI (with the secret) lists bottles while an unauthenticated GET /bottles gets
401, all five issue-#400 attacks from inside the agent get 401, and egress
policy still works (200 allowed / 403 denied) — proving the gateway
authenticates to /resolve with the secret. 1829 unit tests pass, pyright
clean, pylint 9.91.
Refs #400.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adopts the firecracker infra-VM pattern for macOS: the orchestrator control
plane and the gateway data plane now run in a SINGLE Apple container instead of
two. Apple Containers are lightweight VMs with separate kernels, so the prior
two-container design had both guests writing one bot-bottle.db over virtiofs,
where fcntl locks are not coherent across kernels — concurrent writes (the
orchestrator's registry vs the gateway supervise daemon's queue) could corrupt
it. One container = one kernel = coherent locking.
The DB moves onto a container-only Apple volume (bot-bottle-mac-db), never
bind-mounted from the host, so no host process opens the live file either. The
host CLI already reaches registry + supervise state over the control-plane HTTP
surface (cli/supervise.py uses OrchestratorClient), exactly as firecracker's
VM-only DB requires.
Two simplifications fall out of the single container:
- No DNS dance: the control plane and gateway daemons reach each other over
127.0.0.1, so the orchestrator-before-gateway ordering (a workaround for
Apple having no container DNS) is gone, along with the moved-IP recreate
logic it needed.
- Net -243 lines.
Mechanics: the infra container runs from the gateway image with the
control-plane source bind-mounted read-only (like the docker orchestrator, so a
code change needs no rebuild) and a small sh -c init that starts both processes
(mirrors firecracker's _infra_init). Also implements the macOS backend's
ensure_orchestrator() and adds it to discover_orchestrator_url, so operator
tools (supervise) can bring up / find the control plane on demand — previously
the macOS backend died with "no orchestrator control plane".
Verified end-to-end on real Apple Container 1.0.0: the single infra container
comes up healthy (one address for control plane + gateway), both processes run,
the DB is written on the container-only volume, host-side supervise works over
HTTP, and a registered agent gets 200 for an allowed host / 403 for a denied
one. 1824 unit tests pass with `container` absent (CI parity), pyright clean,
pylint 9.89.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
CI's unit + coverage jobs failed with `FileNotFoundError: 'container'`: three
tests reached the real Apple CLI, which exists on a macOS dev host but not on
the Linux runner. They passed locally for that reason alone — and two of them
were quietly creating real Apple networks on the dev host as a side effect.
- `test_enumerate_active_is_empty_while_disabled` asserted the disabled-era
stub and called `enumerate_active()` unmocked. The backend launches bottles
again, so it now covers the real enumeration: slug parsing, exclusion of the
shared gateway/orchestrator singletons, and the CLI-failure path.
- The two orchestrator tests patched `orchestrator_service.container_mod`, but
`_run_orchestrator_container` reaches the CLI through `ensure_networks`,
which is imported from the gateway module and resolves `container_mod` in
*its* namespace. Patch the imported name instead.
Adds a test that the networks exist before the orchestrator runs — the
ordering the escaped call was hiding.
Verified by reproducing the CI environment locally (`PATH` without the
`container` binary): 3 failures before, 1818 passing after.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
didericis-claude
didericis-codex
didericis