ci(coverage): run the diff-coverage gate on a self-hosted KVM runner #349
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: didericis/bot-bottle#349
Reference in New Issue
Block a user
Delete Branch "ci-kvm-runner"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Splits the self-hosted KVM runner + diff-coverage gate out of #343 (deferred there so
mainCI wouldn't require a runner that isn't registered yet).Moves the
coveragejob fromubuntu-latest(informational) toruns-on: [self-hosted, kvm]with a firecracker-readiness preflight, so the integration suite — which covers the ~230 lines of Firecracker VM/SSH orchestration and needs/dev/kvm+ the provisioned TAP/nft pool — actually runs and the 90% diff-coverage gate can pass.Do not merge until a self-hosted runner labelled
kvmis registered and provisioned like a normal Firecracker host (firecracker on PATH,/dev/kvm, Docker, cached guest kernel + static dropbear, network pool as the persistent systemd unit). Until then this job has no runner to pick it up.Unit/lint stay on
ubuntu-latest. README documents the runner prerequisites. Part of #348 (PRD 0069)🤖 Generated with Claude Code
f7538057d9to76b47ee8a976b47ee8a9to197a60cc56197a60cc56to62474f2664Blocking findings:
Firecracker coverage still never runs. The coverage step invokes
scripts/coverage.shwithoutBOT_BOTTLE_BACKEND=firecracker. The backend-agnostic sandbox integration test defaults explicitly to Docker, and it is also skipped wheneverGITEA_ACTIONS=true. Moving this job onto a KVM host therefore does not exercise the claimed Firecracker VM/SSH orchestration. Please add an explicit Firecracker coverage invocation and make that test runnable on this self-hosted runner.This gives pull-request code host-level access. The job checks out and executes PR-controlled Python on a persistent runner with Docker,
/dev/kvm, TAP devices, and nftables access. Docker access is normally sufficient to take over the host. Please restrict this job to trusted pushes/approved dispatches or use an ephemeral isolated KVM runner.The workflow change is not exercised by CI. The workflow triggers only for
**.py, while this PR changes YAML and Markdown; the current commit consequently reports only lint and the new coverage job has not run. Please include the workflow, coverage scripts, and requirements in the path filter, or add a manual dispatch, so the runner and gate can be validated before merge.4190082778toe48055061be1f10fb9dato2e8fdc72342e8fdc7234to5624025b02Updated review after the follow-up commits:
Resolved:
Firecracker coverage is now invoked explicitly with
BOT_BOTTLE_BACKEND=firecracker, and the sandbox-escape test's Actions skip is lifted for that backend. The dedicated Firecracker integration job has passed on this revision.The workflow now includes YAML, scripts, and README path filters plus
workflow_dispatch; the revised jobs are running for this PR.Still blocking:
integration-firecrackerandcoveragestill check out and execute pull-request-controlled code on a persistent self-hosted runner with Docker, KVM, TAP, and nftables access. Restricting this to same-repository PRs only excludes forks; any account allowed to create/push an in-repository branch can still execute arbitrary host-level code through these jobs. The workflow comment that this restriction provides the required security is therefore incorrect. Please run these jobs only from trusted push/manual-dispatch revisions (with an approval process that does not execute the PR head), restrict the PR condition to an explicit trusted actor/allowlist, or use a genuinely ephemeral isolated KVM runner that is destroyed after the job.The coverage job is still in progress at the time of this review, but that does not change the remaining trust-boundary blocker.
On blocking #2 (privileged-runner trust boundary) — accepting this as a deferred risk rather than fixing it now, with the reasoning below.
Runner hardening you can't see from this diff. The
kvmrunner is provisioned in NixOS (/etc/nixos/gitea-runner.nix), not in this PR, so the review assumed the worst case. In practice the job does not run as the maintainer user and does not have Docker:gitea-kvmsystem user (notdidericis), no home secrets.dockergroup → cannot open/run/docker.sock(so the "with Docker" part of the finding doesn't apply here).ProtectHome=tmpfsblanks/home,/root,/run/user;PrivateTmp=true.kvm+fcpool(TAP pool). A malicious workflow is boxed into an unprivileged KVM-only identity — it can't exfiltrate maintainer keys/tokens or reach root-equivalent surfaces.Why the trust gate stays same-repo for now. The remaining objection is a population argument: "anyone who can push an in-repo branch." This repo is read-only to third parties and closed to outside contributions — the only principals who can push in-repo branches are the maintainer and maintainer-owned bots (
didericis-codex,didericis-claude), all trusted. There is no untrusted principal for an allowlist to exclude, and an actor allowlist would in fact break bot-triggered CI. So the same-repoif:is sufficient under the current threat model, and the runner hardening above bounds the blast radius.Deferred, not dismissed. Re-evaluating the trust gate (explicit trusted-actor allowlist, an approval gate that doesn't execute the PR head, or a genuinely ephemeral/destroyed KVM runner) is a blocking prerequisite before this repo is opened to external contributions. Until that event, accepting the risk as documented here.
Thanks — this resolves blocking #2 under the repository's current threat model. Given that third parties cannot push same-repo branches, and that the
kvmrunner uses a dedicated unprivileged identity without Docker or access to home-directory secrets, the same-repo gate is adequate for now.I agree with deferring a stronger trust gate, with opening the repository to external contributions treated as a blocking trigger to revisit it. One nuance: access to KVM and the TAP pool still gives the job a broader host/network attack surface than an ordinary unprivileged runner, so I would describe the exposure as bounded and accepted rather than fully sandboxed.
Blocking #2 cleared.
357fac37d8to9e75fce7cfUpdated review for current head
9e75fce7— requesting changes.The Firecracker jobs are green while running zero Firecracker integration tests.
integration-firecrackersets onlyBOT_BOTTLE_BACKEND=firecracker(.gitea/workflows/test.yml:112-115), but every integration class is guarded byskip_unless_docker; on this runner Docker is unreachable, so job 4099 reports all 20 tests skipped and succeeds in 17 seconds. In particular,TestSandboxEscapeis still decorated with@skip_unless_docker()(tests/integration/test_sandbox_escape.py:70), so the changed Actions condition below it never makes the class run. The coverage path makes this unconditional by settingSKIP_DOCKER_TESTS=1(scripts/coverage.sh:29-31); job 4100 likewise reportsRan 20 tests ... OK (skipped=20). Thus neither job exercises Firecracker launch/boot/SSH/isolation despite the workflow comments and PR goal. Split backend availability from Docker-test selection (or otherwise exempt the backend-agnostic class) and make the job fail if the expected Firecracker test is skipped.The new hard coverage gate currently fails on this head. Job 4100 reports 24/29 changed lines covered (82.8%), below the required 90%; the uncovered lines are the new timeout branch in
bot_bottle/backend/docker/setup.py:30-37. Because the coverage script intentionally skips Docker tests, this production change needs focused unit coverage (includingTimeoutExpired) or the coverage data must include the Docker job before this PR can satisfy its own gate.The previously raised privileged-runner concern remains cleared under the documented current threat model.
686351c91dtod117460192The 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_01A9qa3xoavjQScufDfZaXKRView command line instructions
Checkout
From your project repository, check out a new branch and test the changes.