ci: backend-agnostic integration guards + per-backend preflight #470

2026-07-23T21:16:20-04:00

didericis-claude commented

2026-07-23 21:16:20 -04:00

What & why

Closes #414. Makes the integration suite backend-agnostic and surfaces per-backend readiness at the CI job level instead of hiding it among unittest.skip lines.

Task 1 — backend-agnostic guards

New tests/_backend.py centralizes backend selection and skip guards, gating on the backend's own readiness check — bot_bottle.backend.has_backend (the same probe behind ./cli.py backend status), not a hand-rolled capability probe:

selected_backend() reads BOT_BOTTLE_BACKEND (default docker).
skip_unless_selected_backend_available() — backend-agnostic tests (test_sandbox_escape) run through whichever backend is selected and skip unless that backend is available (checking, e.g., Linux + /dev/kvm for Firecracker rather than unrelated Docker availability).
skip_unless_backend("docker") — the Docker-implementation tests (DockerBroker, DockerGateway, backend.docker.*) no-op under a run targeting a different backend instead of testing internals it doesn't exercise. These files stay named test_orchestrator_docker_* because they intentionally exercise Docker-specific code; the guard, not the name, is what now reads BOT_BOTTLE_BACKEND.

Retires tests/_docker.py and the SKIP_DOCKER_TESTS steering the KVM job relied on.

Task 2 — explicit skip visibility per backend

Each integration job runs ./cli.py backend status --backend=<name> as a preflight: it prints a clear per-check readiness summary and exits non-zero when the backend is missing, so absent infrastructure fails the job loudly rather than turning every test into a silent no-op. The integration-docker job replaces its soft "Show environment" step with this preflight; the integration-firecracker job keeps its existing binary/KVM checks plus backend status.

Verification

tests/unit/test_backend_skip_guards.py covers the guards (has_backend mocked, so the unit job needs neither backend present).
Full unit suite passes; integration discovery skips cleanly with no backend on this host. pylint 10.00/10 and pyright clean on the new/changed files.

Docs updated: tests/README.md, docs/ci.md.

🤖 Generated with Claude Code

## What & why Closes #414. Makes the integration suite backend-agnostic and surfaces per-backend readiness at the CI job level instead of hiding it among `unittest.skip` lines. ## Task 1 — backend-agnostic guards New `tests/_backend.py` centralizes backend selection and skip guards, gating on the backend's own readiness check — `bot_bottle.backend.has_backend` (the same probe behind `./cli.py backend status`), not a hand-rolled capability probe: - `selected_backend()` reads `BOT_BOTTLE_BACKEND` (default `docker`). - `skip_unless_selected_backend_available()` — backend-agnostic tests (`test_sandbox_escape`) run through whichever backend is selected and skip unless that backend is available (checking, e.g., Linux + `/dev/kvm` for Firecracker rather than unrelated Docker availability). - `skip_unless_backend("docker")` — the Docker-implementation tests (`DockerBroker`, `DockerGateway`, `backend.docker.*`) no-op under a run targeting a different backend instead of testing internals it doesn't exercise. These files stay named `test_orchestrator_docker_*` because they intentionally exercise Docker-specific code; the guard, not the name, is what now reads `BOT_BOTTLE_BACKEND`. Retires `tests/_docker.py` and the `SKIP_DOCKER_TESTS` steering the KVM job relied on. ## Task 2 — explicit skip visibility per backend Each integration job runs `./cli.py backend status --backend=<name>` as a preflight: it prints a clear per-check readiness summary and exits non-zero when the backend is missing, so absent infrastructure fails the job loudly rather than turning every test into a silent no-op. The `integration-docker` job replaces its soft "Show environment" step with this preflight; the `integration-firecracker` job keeps its existing binary/KVM checks plus `backend status`. ## Verification - `tests/unit/test_backend_skip_guards.py` covers the guards (`has_backend` mocked, so the unit job needs neither backend present). - Full unit suite passes; integration discovery skips cleanly with no backend on this host. `pylint` 10.00/10 and `pyright` clean on the new/changed files. Docs updated: `tests/README.md`, `docs/ci.md`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

didericis added 1 commit 2026-07-23 21:33:20 -04:00

ci: backend-agnostic integration guards + per-backend preflight

tracker-policy-pr / check-pr (pull_request) Successful in 15s

Details

test / integration-docker (pull_request) Successful in 20s

Details

lint / lint (push) Successful in 1m5s

Details

test / unit (pull_request) Successful in 1m49s

Details

test / integration-firecracker (pull_request) Successful in 2m0s

Details

test / coverage (pull_request) Successful in 16s

Details

test / publish-infra (pull_request) Has been skipped

Details

9fdaba4bd4

Integration tests now select their backend from BOT_BOTTLE_BACKEND and
skip on the capability that backend actually needs, instead of gating
every backend on unrelated Docker availability.

Task 1 — backend-agnostic guards (tests/_backend.py):
- Capability probes: docker_capability() (reachable daemon) and
  firecracker_capability() (accessible /dev/kvm + firecracker on PATH,
  Docker-independent). backend_capability()/selected_backend() resolve
  the target from BOT_BOTTLE_BACKEND (default docker).
- skip_unless_selected_backend_available() for backend-agnostic tests
  (test_sandbox_escape) — runs through whichever backend is selected and
  checks that backend's real capability.
- skip_unless_backend("docker") for Docker-implementation tests
  (DockerBroker, DockerGateway, backend.docker.*) — they no-op under a
  non-Docker run rather than testing internals that run doesn't target.
- Retires tests/_docker.py; the KVM job no longer needs SKIP_DOCKER_TESTS
  to steer Docker-only classes.

Task 2 — explicit per-backend skip visibility:
- tests/backend_preflight.py prints a clear PASS/FAIL capability line and
  exits non-zero when the selected backend is missing.
- Both integration jobs run it as a preflight, so absent infrastructure
  is surfaced at the job level instead of hidden among unittest.skip
  lines. The docker job replaces its soft "Show environment" step; the
  firecracker job keeps its richer backend-status check.

Docs (tests/README.md, docs/ci.md) updated; unit coverage for the probes,
guards, and preflight in test_backend_skip_guards.py.

Closes #414

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

didericis force-pushed ci/backend-agnostic-integration-414 from 2c857787b7 to 9fdaba4bd4

2026-07-23 21:33:20 -04:00

Compare

didericis reviewed 2026-07-23 21:36:27 -04:00

tests/_backend.py

						
				@@ -0,0 +92,4 @@

				        )

				    if shutil.which("firecracker") is None:

				        return Capability(name, False, "firecracker not on PATH")

				    return Capability(name, True, f"{_KVM_DEVICE} accessible; firecracker on PATH")

didericis commented

2026-07-23 21:36:27 -04:00

@didericis-claude This is way overkill. There's already a capability test built into the bot bottle cli. use that.

didericis added 1 commit 2026-07-23 21:42:24 -04:00

ci: reuse backend.has_backend / backend status for readiness

test / integration-docker (pull_request) Successful in 25s

Details

test / unit (pull_request) Successful in 52s

Details

lint / lint (push) Successful in 1m0s

Details

test / integration-firecracker (pull_request) Successful in 2m5s

Details

test / coverage (pull_request) Successful in 24s

Details

test / publish-infra (pull_request) Has been skipped

Details

tracker-policy-pr / check-pr (pull_request) Successful in 6s

Details

d4d45f835e

Review feedback: the hand-rolled `/dev/kvm` + docker-daemon capability
probes duplicated logic the CLI already owns. Delegate instead.

- tests/_backend.py skip guards now gate on `bot_bottle.backend.has_backend`
  (each backend's `is_available()` classmethod — the same probe behind
  `./cli.py backend status`), dropping the bespoke `Capability` probes.
- Remove tests/backend_preflight.py; the docker integration job runs
  `./cli.py backend status --backend=docker` as its preflight (clear
  per-check summary, non-zero exit when unready), matching the firecracker
  job. The firecracker preflight reverts to its original binary/KVM checks
  (backend status doesn't cover those).
- Unit test + docs updated to match.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

didericis-claude commented

2026-07-23 21:42:37 -04:00

Good call — dropped the hand-rolled probes. The skip guards now gate on bot_bottle.backend.has_backend(name) (each backend's is_available() classmethod — the same readiness check behind ./cli.py backend status), and I removed tests/backend_preflight.py entirely: the docker integration job now runs ./cli.py backend status --backend=docker as its preflight, matching what the firecracker job already does. The firecracker preflight is back to its original binary/KVM checks (those aren't covered by backend status). See d4d45f8.

Good call — dropped the hand-rolled probes. The skip guards now gate on `bot_bottle.backend.has_backend(name)` (each backend's `is_available()` classmethod — the same readiness check behind `./cli.py backend status`), and I removed `tests/backend_preflight.py` entirely: the docker integration job now runs `./cli.py backend status --backend=docker` as its preflight, matching what the firecracker job already does. The firecracker preflight is back to its original binary/KVM checks (those aren't covered by `backend status`). See d4d45f8.

didericis-codex requested changes 2026-07-23 21:55:53 -04:00

Dismissed

didericis-codex left a comment

There is one blocking issue with the new skip guard:

[P1] Gate on launch readiness, not has_backend() capability — tests/_backend.py:45-48,59-61. The PR says this is the same readiness probe as backend status, but has_backend() delegates to each backend's deliberately cheap is_available(). For Docker that only checks whether the CLI is on PATH (DockerBottleBackend.is_available explicitly avoids probing the daemon), whereas backend status also checks daemon reachability. Firecracker similarly checks only binary + Linux/KVM, while status validates the TAP pool/range. Consequently a local integration discovery on a partially configured host no longer “skips cleanly”: the decorators enable the classes and the tests fail later during prepare/launch. The CI preflights mask this because they fail before discovery, and the added unit tests mock has_backend rather than covering the partial-readiness cases. Please make the guard use a non-printing full-readiness probe (or otherwise retain the old Docker daemon check and equivalent Firecracker readiness checks), and add regression coverage for “Docker CLI present, daemon unavailable” plus “Firecracker capability present, pool unavailable.”

I also ran the new guard tests and the full unit suite; the new tests pass, and the current CI statuses are green, but neither exercises this distinction.

There is one blocking issue with the new skip guard: - **[P1] Gate on launch readiness, not `has_backend()` capability — `tests/_backend.py:45-48,59-61`.** The PR says this is the same readiness probe as `backend status`, but `has_backend()` delegates to each backend's deliberately cheap `is_available()`. For Docker that only checks whether the CLI is on PATH (`DockerBottleBackend.is_available` explicitly avoids probing the daemon), whereas `backend status` also checks daemon reachability. Firecracker similarly checks only binary + Linux/KVM, while status validates the TAP pool/range. Consequently a local integration discovery on a partially configured host no longer “skips cleanly”: the decorators enable the classes and the tests fail later during prepare/launch. The CI preflights mask this because they fail before discovery, and the added unit tests mock `has_backend` rather than covering the partial-readiness cases. Please make the guard use a non-printing full-readiness probe (or otherwise retain the old Docker daemon check and equivalent Firecracker readiness checks), and add regression coverage for “Docker CLI present, daemon unavailable” plus “Firecracker capability present, pool unavailable.” I also ran the new guard tests and the full unit suite; the new tests pass, and the current CI statuses are green, but neither exercises this distinction.

didericis commented

2026-07-23 22:36:38 -04:00

@didericis-codex

change the has_backend function name to is_backend_available
add a new class method to each backend called is_ready which does the full readiness check
add a new is_backend_ready method that defers to each backend's is_ready method (same idea as is_backend_available)

Also I think most has_backend calls should be switched to is_backend_ready after the above check. Flag any specific callsites that seem like they should stay is_backend_available

@didericis-codex - change the `has_backend` function name to `is_backend_available` - add a new class method to each backend called `is_ready` which does the full readiness check - add a new `is_backend_ready` method that defers to each backend's `is_ready` method (same idea as `is_backend_available`) Also I think most `has_backend` calls should be switched to `is_backend_ready` after the above check. Flag any specific callsites that seem like they should stay `is_backend_available`

didericis-codex commented

2026-07-23 22:37:34 -04:00

Yes—this matches the current layering and avoids the circular delegation problem.

For the existing production has_backend call sites, I’d use:

tests/_backend.py (both guards) → is_backend_ready(...)
bot_bottle/cli/cleanup.py:37 → keep the cheap predicate, renamed to is_backend_available(...); cleanup should still discover orphaned resources when launch prerequisites such as the Firecracker TAP pool are broken
backend/__init__.py:750,761 (macOS/Docker auto-selection) → keep is_backend_available(...); selection should identify the available backend and let its preflight explain why it is not currently ready

enumerate_active_agents() already calls each backend’s class-level is_available() directly, so it naturally stays on the cheap path.

The only additional design point I’d check in the revision is that each backend’s is_ready() and status() use the same underlying checks, with status() adding output. Otherwise the boolean guard and CLI preflight can drift again. I’ll re-review the implementation and tests against that behavior.

Yes—this matches the current layering and avoids the circular delegation problem. For the existing production `has_backend` call sites, I’d use: - `tests/_backend.py` (both guards) → `is_backend_ready(...)` - `bot_bottle/cli/cleanup.py:37` → keep the cheap predicate, renamed to `is_backend_available(...)`; cleanup should still discover orphaned resources when launch prerequisites such as the Firecracker TAP pool are broken - `backend/__init__.py:750,761` (macOS/Docker auto-selection) → keep `is_backend_available(...)`; selection should identify the available backend and let its preflight explain why it is not currently ready `enumerate_active_agents()` already calls each backend’s class-level `is_available()` directly, so it naturally stays on the cheap path. The only additional design point I’d check in the revision is that each backend’s `is_ready()` and `status()` use the same underlying checks, with `status()` adding output. Otherwise the boolean guard and CLI preflight can drift again. I’ll re-review the implementation and tests against that behavior.

didericis commented

2026-07-23 22:42:46 -04:00

@didericis-codex ooo, forgot about status. that actually does most of what we want. Use status for the readiness checks/ensure it returns 0. also add a BACKEND_READY = 0 status enum we check against (and use that other places we check for status 0) so we can add more detailed status flags if needed, and a quiet boolean flag that turns off printing when set to true. But do NOT make it quiet for the test check (we want the specific reason it's not ready printed in the test action to debug it if it doesn't pass)

@didericis-codex ooo, forgot about status. that actually does most of what we want. Use `status` for the readiness checks/ensure it returns 0. also add a `BACKEND_READY = 0` status enum we check against (and use that other places we check for status 0) so we can add more detailed status flags if needed, and a `quiet` boolean flag that turns off printing when set to true. But do NOT make it quiet for the test check (we want the specific reason it's not ready printed in the test action to debug it if it doesn't pass)

didericis-codex commented

2026-07-23 22:43:58 -04:00

That works and reuses the authoritative checks. I’ll evaluate the revision with this shape in mind:

each backend keeps status(*, quiet: bool = False), returning the readiness status value
is_backend_ready(name, *, quiet: bool = False) resolves the backend and checks backend.status(quiet=quiet) == BACKEND_READY
the integration guards call it non-quiet, as requested, so a skipped test run shows the concrete failed prerequisite
the CLI remains non-quiet and returns the status as its process exit code
is_backend_available remains the cheap predicate for cleanup and auto-selection

One naming/type detail: if this is an actual enum, I’d prefer something like BackendStatus.READY = 0; if the intended public API is specifically BACKEND_READY, a module-level integer/enum alias is fine too. Either way, comparisons should use that symbol rather than raw 0, including existing status checks.

Because decorators run during discovery, non-quiet checks may print the same failed status once per decorated module/class. That is consistent with your instruction; the CI preflight will still fail earlier with one report, while direct local discovery gets the diagnostic alongside its skips.

That works and reuses the authoritative checks. I’ll evaluate the revision with this shape in mind: - each backend keeps `status(*, quiet: bool = False)`, returning the readiness status value - `is_backend_ready(name, *, quiet: bool = False)` resolves the backend and checks `backend.status(quiet=quiet) == BACKEND_READY` - the integration guards call it non-quiet, as requested, so a skipped test run shows the concrete failed prerequisite - the CLI remains non-quiet and returns the status as its process exit code - `is_backend_available` remains the cheap predicate for cleanup and auto-selection One naming/type detail: if this is an actual enum, I’d prefer something like `BackendStatus.READY = 0`; if the intended public API is specifically `BACKEND_READY`, a module-level integer/enum alias is fine too. Either way, comparisons should use that symbol rather than raw `0`, including existing status checks. Because decorators run during discovery, non-quiet checks may print the same failed status once per decorated module/class. That is consistent with your instruction; the CI preflight will still fail earlier with one report, while direct local discovery gets the diagnostic alongside its skips.

👍 1

didericis commented

2026-07-23 22:45:29 -04:00

@didericis-codex module level enum makes sense.

@didericis-claude implement this

@didericis-codex module level enum makes sense. @didericis-claude implement this

didericis added 1 commit 2026-07-23 23:03:10 -04:00

feat(backend): BackendStatus enum, quiet status(), is_backend_ready()

tracker-policy-pr / check-pr (pull_request) Successful in 11s

Details

test / integration-docker (pull_request) Successful in 17s

Details

lint / lint (push) Failing after 54s

Details

test / unit (pull_request) Failing after 1m33s

Details

test / integration-firecracker (pull_request) Successful in 3m17s

Details

test / coverage (pull_request) Has been skipped

Details

test / publish-infra (pull_request) Has been skipped

Details

0ca39cf4d5

Add a module-level BackendStatus(IntEnum) with READY=0 to replace magic
0 comparisons. Extend BottleBackend.status() with a quiet parameter:
quiet=False (default) prints diagnostics; quiet=True returns the code
silently for programmatic checks.

Add is_backend_available() (cheap PATH check, alias for has_backend) and
is_backend_ready(name, *, quiet=False) (full status() check) to the
backend package for callers that need to distinguish availability from
readiness.

Update tests/_backend.py guards to use is_backend_ready(quiet=False) so
diagnostic output is printed during test discovery, giving the operator a
concrete reason for each skip rather than a bare "prerequisites
unavailable" message.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

didericis added 1 commit 2026-07-24 00:25:11 -04:00

fix: align skip-guard tests with is_backend_ready and drop unused import

tracker-policy-pr / check-pr (pull_request) Successful in 14s

Details

test / integration-docker (pull_request) Successful in 16s

Details

test / unit (pull_request) Successful in 37s

Details

lint / lint (push) Successful in 53s

Details

test / integration-firecracker (pull_request) Successful in 3m18s

Details

test / coverage (pull_request) Failing after 35s

Details

test / publish-infra (pull_request) Has been skipped

Details

1f9a15dede

tests/_backend.py imported is_backend_available but never called it,
causing a pyright reportUnusedImport error. Remove the unused import.

test_backend_skip_guards.py was patching tests._backend.has_backend but
_backend.py calls is_backend_ready — the mock never intercepted the real
call, producing AttributeError at test runtime. Update all patch targets
to is_backend_ready and add quiet=False to the assert_called_once_with
assertion to match the actual call signature.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

didericis added 1 commit 2026-07-24 02:12:10 -04:00

test: cover is_backend_available, is_backend_ready, and status(quiet=True)

test / integration-docker (pull_request) Successful in 21s

Details

lint / lint (push) Failing after 59s

Details

test / unit (pull_request) Successful in 1m47s

Details

test / integration-firecracker (pull_request) Successful in 3m22s

Details

test / coverage (pull_request) Successful in 18s

Details

test / publish-infra (pull_request) Has been skipped

Details

tracker-policy-pr / check-pr (pull_request) Failing after 13m0s

Details

05b62ce805

Add unit tests for the three new public symbols introduced by this PR:

- TestIsBackendAvailable — delegates to has_backend; returns True/False
  correctly for available and unavailable backends
- TestIsBackendReady — unknown names return False; known names return
  True/False based on status() return code; quiet flag is forwarded
- TestBackendStatusQuiet — verifies the quiet=True branch in
  DockerBottleBackend, FirecrackerBottleBackend, and
  MacosContainerBottleBackend: return code is propagated and
  diagnostic stderr is suppressed

Together these bring diff-coverage for the PR's changed lines to 100%.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

didericis added 1 commit 2026-07-24 02:19:00 -04:00

fix: add missing type annotations and remove unused variables in new tests

test / integration-docker (pull_request) Successful in 23s

Details

test / unit (pull_request) Successful in 47s

Details

lint / lint (push) Successful in 3m3s

Details

test / integration-firecracker (pull_request) Successful in 3m24s

Details

test / coverage (pull_request) Successful in 20s

Details

test / publish-infra (pull_request) Has been skipped

Details

tracker-policy-pr / check-pr (pull_request) Successful in 17s

Details

e074c6959f

Pyright flagged three inner class `status` methods missing a type
annotation on the `quiet` parameter, and two unused variables
(`written`, `real_status`) left over from an earlier draft of
test_docker_quiet_true_suppresses_stderr.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

didericis-codex requested changes 2026-07-24 02:33:23 -04:00

Dismissed

didericis-codex left a comment

The Docker daemon-readiness issue is fixed, but one blocking Firecracker case remains:

[P1] Firecracker status() still omits basic runtime capability — bot_bottle/backend/__init__.py:836-847, bot_bottle/backend/firecracker/setup.py:222-258. is_backend_ready() now calls only status(). Firecracker's status checks the TAP pool and route overlap, but it does not check Linux, the firecracker executable, or usable KVM access. A host with a configured pool but a removed/broken binary or inaccessible /dev/kvm therefore enables the integration suite and fails later during launch.

There is no single Firecracker equivalent of docker info, because Firecracker has no daemon. The full non-mutating readiness probe should combine:

firecracker --version to confirm the executable can run.
Open /dev/kvm and issue KVM_GET_API_VERSION to verify actual KVM access, including permissions—not merely device existence.
Existing kernel/Dropbear artifact, TAP-pool, route-conflict, and nft-boundary checks as applicable.

The KVM ioctl is the closest analogue to docker info: it asks the kernel virtualization interface whether this process can actually use it. Please incorporate the capability checks into Firecracker status() (or combine a strengthened availability probe with status) and add regressions for a ready pool with a missing Firecracker binary and with unusable KVM.

Focused backend selection, setup, and skip-guard tests otherwise pass (79 tests).

The Docker daemon-readiness issue is fixed, but one blocking Firecracker case remains: - **[P1] Firecracker `status()` still omits basic runtime capability — `bot_bottle/backend/__init__.py:836-847`, `bot_bottle/backend/firecracker/setup.py:222-258`.** `is_backend_ready()` now calls only `status()`. Firecracker's status checks the TAP pool and route overlap, but it does not check Linux, the `firecracker` executable, or usable KVM access. A host with a configured pool but a removed/broken binary or inaccessible `/dev/kvm` therefore enables the integration suite and fails later during launch. There is no single Firecracker equivalent of `docker info`, because Firecracker has no daemon. The full non-mutating readiness probe should combine: 1. `firecracker --version` to confirm the executable can run. 2. Open `/dev/kvm` and issue `KVM_GET_API_VERSION` to verify actual KVM access, including permissions—not merely device existence. 3. Existing kernel/Dropbear artifact, TAP-pool, route-conflict, and nft-boundary checks as applicable. The KVM ioctl is the closest analogue to `docker info`: it asks the kernel virtualization interface whether this process can actually use it. Please incorporate the capability checks into Firecracker `status()` (or combine a strengthened availability probe with status) and add regressions for a ready pool with a missing Firecracker binary and with unusable KVM. Focused backend selection, setup, and skip-guard tests otherwise pass (79 tests).

didericis added 1 commit 2026-07-24 02:52:05 -04:00

firecracker status(): add binary and KVM readiness checks

tracker-policy-pr / check-pr (pull_request) Successful in 11s

Details

test / integration-docker (pull_request) Successful in 19s

Details

lint / lint (push) Successful in 1m0s

Details

test / unit (pull_request) Successful in 1m43s

Details

test / integration-firecracker (pull_request) Successful in 3m22s

Details

test / coverage (pull_request) Successful in 16s

Details

test / publish-infra (pull_request) Has been skipped

Details

755a11a608

Adds `_firecracker_binary_ok()` (runs `firecracker --version`) and
`_kvm_accessible()` (opens /dev/kvm, issues KVM_GET_API_VERSION ioctl)
and gates `status()` on both, so the launch preflight reports missing
binary or inaccessible KVM before the caller ever attempts a boot.

Regression tests in TestFirecrackerBinaryCheck, TestFirecrackerKvmCheck,
and TestFirecrackerStatusRuntime cover all branches. Updated the
pre-existing TestFirecrackerStatus fixture to also stub the two new
helpers so it still exercises only the tap-pool/nft logic it was
designed for.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

didericis-codex requested changes 2026-07-24 11:53:30 -04:00

didericis-codex left a comment

The new binary execution and KVM ioctl checks address the previous finding, but two Firecracker readiness gaps remain:

[P1] Open /dev/kvm read-write when testing actual usability — bot_bottle/backend/firecracker/setup.py:239-252. _kvm_accessible() currently opens the device with "rb". KVM_GET_API_VERSION can succeed on a read-only descriptor, while creating a VM requires /dev/kvm opened with O_RDWR. A user with read-only device access can therefore pass backend status and the integration guard, then fail during launch. Use os.open(path, os.O_RDWR | os.O_CLOEXEC) (or "rb+") before issuing the ioctl, and cover a read-only/open failure.
[P1] Include the remaining hard launch prerequisites in Firecracker status — bot_bottle/backend/firecracker/util.py:85-105, bot_bottle/backend/firecracker/setup.py:255-311. require_firecracker() also requires the guest kernel image, static Dropbear binary, and mke2fs, but status() can still return ready when any of those are missing. The new skip guard will consequently enable integration tests on a host that cannot launch the backend. Add the same non-mutating file/tool checks to status() and regression coverage for each missing prerequisite.

Focused backend tests pass (127 tests), but these cases are not currently covered.

The new binary execution and KVM ioctl checks address the previous finding, but two Firecracker readiness gaps remain: - **[P1] Open `/dev/kvm` read-write when testing actual usability — `bot_bottle/backend/firecracker/setup.py:239-252`.** `_kvm_accessible()` currently opens the device with `"rb"`. `KVM_GET_API_VERSION` can succeed on a read-only descriptor, while creating a VM requires `/dev/kvm` opened with `O_RDWR`. A user with read-only device access can therefore pass `backend status` and the integration guard, then fail during launch. Use `os.open(path, os.O_RDWR | os.O_CLOEXEC)` (or `"rb+"`) before issuing the ioctl, and cover a read-only/open failure. - **[P1] Include the remaining hard launch prerequisites in Firecracker status — `bot_bottle/backend/firecracker/util.py:85-105`, `bot_bottle/backend/firecracker/setup.py:255-311`.** `require_firecracker()` also requires the guest kernel image, static Dropbear binary, and `mke2fs`, but `status()` can still return ready when any of those are missing. The new skip guard will consequently enable integration tests on a host that cannot launch the backend. Add the same non-mutating file/tool checks to `status()` and regression coverage for each missing prerequisite. Focused backend tests pass (127 tests), but these cases are not currently covered.

didericis added 1 commit 2026-07-24 12:04:02 -04:00

firecracker status(): open /dev/kvm O_RDWR; check kernel, dropbear, mke2fs

test / integration-docker (pull_request) Successful in 19s

Details

tracker-policy-pr / check-pr (pull_request) Successful in 19s

Details

test / unit (pull_request) Successful in 40s

Details

lint / lint (push) Successful in 58s

Details

test / integration-firecracker (pull_request) Successful in 3m20s

Details

test / coverage (pull_request) Successful in 22s

Details

test / publish-infra (pull_request) Has been skipped

Details

65a49a239e

Two reviewer findings addressed:

1. _kvm_accessible() now opens /dev/kvm with O_RDWR|O_CLOEXEC instead
   of read-only. VM creation requires write access; a read-only
   descriptor can satisfy KVM_GET_API_VERSION but fails at boot time.

2. status() now checks every hard prerequisite that require_firecracker()
   checks at launch: guest kernel image, static dropbear binary, and
   mke2fs. Previously a host with a configured TAP pool but missing
   artifacts could pass status() and then fail during launch.

Regression coverage: TestFirecrackerKvmCheck gains test_kvm_open_rdwr_fails;
TestFirecrackerArtifactCheck covers each missing artifact; existing
TestFirecrackerStatus fixtures updated to stub the new checks.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

didericis-codex approved these changes 2026-07-24 21:21:06 -04:00

didericis-codex left a comment

Re-reviewed at 65a49a239effc9cbbb741b568905e079c0610a4c. The prior readiness findings are resolved: /dev/kvm is opened with O_RDWR before KVM_GET_API_VERSION, and Firecracker status now checks the executable, usable KVM access, kernel, static Dropbear, mke2fs, TAP pool, and route conflicts. Regression coverage was added for the missing/unusable cases.

Focused backend tests pass (131 tests), and the current real-runner integration-firecracker job is green. No remaining findings.

Re-reviewed at `65a49a239effc9cbbb741b568905e079c0610a4c`. The prior readiness findings are resolved: `/dev/kvm` is opened with `O_RDWR` before `KVM_GET_API_VERSION`, and Firecracker status now checks the executable, usable KVM access, kernel, static Dropbear, `mke2fs`, TAP pool, and route conflicts. Regression coverage was added for the missing/unusable cases. Focused backend tests pass (131 tests), and the current real-runner `integration-firecracker` job is green. No remaining findings.

didericis-codex approved these changes 2026-07-24 21:21:21 -04:00

didericis-codex left a comment

Re-reviewed at 65a49a239effc9cbbb741b568905e079c0610a4c. The prior readiness findings are resolved: /dev/kvm is opened with O_RDWR before KVM_GET_API_VERSION, and Firecracker status now checks the executable, usable KVM access, kernel, static Dropbear, mke2fs, TAP pool, and route conflicts. Regression coverage was added for the missing/unusable cases.

Focused backend tests pass (131 tests), and the current real-runner integration-firecracker job is green. No remaining findings.

Re-reviewed at `65a49a239effc9cbbb741b568905e079c0610a4c`. The prior readiness findings are resolved: `/dev/kvm` is opened with `O_RDWR` before `KVM_GET_API_VERSION`, and Firecracker status now checks the executable, usable KVM access, kernel, static Dropbear, `mke2fs`, TAP pool, and route conflicts. Regression coverage was added for the missing/unusable cases. Focused backend tests pass (131 tests), and the current real-runner `integration-firecracker` job is green. No remaining findings.

didericis merged commit a68ee778f1 into main

2026-07-24 21:25:23 -04:00

didericis deleted branch ci/backend-agnostic-integration-414

2026-07-24 21:25:24 -04:00

didericis referenced this issue from a commit

2026-07-24 21:36:03 -04:00

Merge main into fix/db-off-data-plane-469

Sign in to join this conversation.

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: didericis/bot-bottle#470