Conversation
One definition of the host layer under infra/vm with three runners: build.sh customizes a pinned Ubuntu cloud image offline, apply.sh runs the same steps over SSH on a live host, and dev/boot.sh boots a built image locally under KVM. Dev and production differ only in the first-boot seed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 💬 COMMENT
This PR introduces infra/vm, a single definition of the Rome host layer (pinned Docker, fail2ban, metadata egress block, compose project, optional WeChat helper) driven by three runners — build.sh (offline via virt-customize), apply.sh (over SSH), and dev/boot.sh (KVM) — over the same idempotent provision/ steps. The design is clean: one source of truth in pins.env/provision/, build-only identity scrubbing isolated in 90-seal.sh, and the Rome image kept out of the host layer as a first-boot accelerator. I verified the cross-references (packages/host-helper binary and its /etc/rome-host/config.json default, docs/wechat-personal.md, the socketGid config contract) and they are consistent.\n\nOverall this is a good change. The findings are correctness/robustness gaps rather than architectural problems: apply.sh's SSH-option parser assumes every flag takes a value; the DOCKER-USER metadata block is not re-applied after a Docker restart (weakening a stated hardening guarantee, and lost in the dev boot flow that restarts docker); and fail2ban installation is coupled to the Docker version check. None are blocking.
Verdict: COMMENT — Solid, well-documented infra addition with no critical issues; a few robustness gaps (SSH arg parsing, non-durable metadata firewall rule) are worth fixing before relying on it for the tenant fleet.
4 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P2 | error-handling | infra/vm/apply.sh |
apply.sh SSH-option parser assumes every -flag takes a value |
| P2 | security | infra/vm/files/rome-block-metadata.service |
Metadata egress block is not durable across Docker restarts |
| P3 | code-quality | infra/vm/provision/10-docker.sh |
fail2ban install is coupled to the Docker version check |
| P3 | code-quality | infra/vm/dev/boot.sh |
dev/boot.sh is hardcoded to amd64 despite taking any image path |
| case "$1" in | ||
| --wechat) wechat=true; shift ;; | ||
| --hostd) hostd="$2"; shift 2 ;; | ||
| -*) ssh_args+=("$1" "$2"); shift 2 ;; |
There was a problem hiding this comment.
[P2] error-handling — apply.sh SSH-option parser assumes every -flag takes a value
The -*) ssh_args+=("$1" "$2"); shift 2 branch consumes two tokens for every option, which is correct for value flags (-p 22, -i key) but wrong for valueless SSH flags like -A, -v, -4, -C, -q, -t. apply.sh --wechat -v root@host would swallow root@host as -v's argument, leaving target empty and failing the usage check — or worse, silently misparse. Since Rome Cloud is expected to invoke this over its existing SSH channel, a valueless flag would break the apply. Consider a small allowlist of value-taking options, or pass SSH options via a single --ssh 'opts' string / after a -- separator.
There was a problem hiding this comment.
Fixed. SSH options now go after a -- separator and pass through verbatim, so -v and -p 22 both work. A flag before the target is rejected with a message. Verified against the dev VM with -- -p 2322 -v -o ….
|
|
||
| [Service] | ||
| Type=oneshot | ||
| ExecStart=/bin/sh -c "iptables -C DOCKER-USER -d 169.254.169.254 -j DROP 2>/dev/null || iptables -I DOCKER-USER -d 169.254.169.254 -j DROP" |
There was a problem hiding this comment.
[P2] security — Metadata egress block is not durable across Docker restarts
Rules inserted into the DOCKER-USER chain do not survive systemctl restart docker — dockerd recreates the chain and drops externally-added rules. This oneshot unit only inserts the rule at its own start, so any Docker restart silently removes the metadata block until the next boot. This is already observable in dev/boot.sh, whose runcmd runs systemctl restart docker after the unit fired at boot, so the dev VM ends up with no metadata block. Bind the unit to Docker's lifecycle (e.g. PartOf=docker.service + WantedBy on docker, or a docker.service drop-in / restart hook) so the rule is reasserted whenever Docker restarts.
There was a problem hiding this comment.
Fixed. The unit is now PartOf=docker.service, and 20-harden.sh installs a docker.service.d drop-in with Wants=rome-block-metadata.service, so every docker start pulls it in and every docker restart restarts it. Verified on the dev VM: the rule is present after the seed's systemctl restart docker and again after a manual restart.
|
|
||
| if ! dpkg -s docker-ce 2>/dev/null | grep -q "^Version: ${DOCKER_CE_VERSION}$"; then | ||
| apt-get update | ||
| apt-get install -y --no-install-recommends ca-certificates curl gnupg fail2ban |
There was a problem hiding this comment.
[P3] code-quality — fail2ban install is coupled to the Docker version check
fail2ban (and ca-certificates, curl, gnupg) is installed only inside the if ! dpkg -s docker-ce ... == pin block. On a host that already has Docker at the pinned version but lacks fail2ban (e.g. Docker installed by other means, or the pin coincidentally matching), this step is skipped and the subsequent 20-harden.sh fails at fail2ban-client -t. It's an edge case, but the coupling is fragile and mixes two concerns in a file named 10-docker.sh. Consider installing fail2ban unconditionally (or in 20-harden.sh) so hardening isn't gated on the Docker branch.
There was a problem hiding this comment.
Fixed. fail2ban is installed by 20-harden.sh when dpkg -s says it is missing, independent of the Docker branch. 10-docker.sh now only concerns Docker.
| # ROME_VM_CONSOLE_LOG=path detaches the console to a file for scripted runs. | ||
| console=(-nographic -serial mon:stdio) | ||
| [[ -n "${ROME_VM_CONSOLE_LOG:-}" ]] && console=(-display none -monitor none -serial "file:$ROME_VM_CONSOLE_LOG") | ||
| exec qemu-system-x86_64 -enable-kvm -cpu host -smp 4 -m 6G "${console[@]}" \ |
There was a problem hiding this comment.
[P3] code-quality — dev/boot.sh is hardcoded to amd64 despite taking any image path
exec qemu-system-x86_64 ... -enable-kvm -cpu host and the OVMF discovery are x86-only, but the script accepts an arbitrary image path with no arch guard. Passing an arm64 image (the desktop use case flagged as upcoming) would boot the wrong emulator or fail obscurely. A quick guard that rejects a non-amd64 image, or derives the qemu binary/machine type from the image name, would make the failure explicit rather than silent.
There was a problem hiding this comment.
Fixed. dev/boot.sh rejects an image whose name is not *amd64* and a non-x86_64 host with an explicit message, and the comment points arm64 at the desktop's Lima provider.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
…n apply.sh parsing - rome-block-metadata is PartOf docker.service and pulled in by a docker.service drop-in, so dockerd recreating DOCKER-USER no longer drops the rule - apply.sh takes ssh options after --, so valueless flags parse - fail2ban installs in 20-harden.sh, independent of the Docker pin check - dev/boot.sh rejects non-amd64 images and non-x86_64 hosts explicitly - biome-format the JSON files Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Jessie-QingYu
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR adds infra/vm: a single, pinned definition of the Rome host layer (Docker at a pinned apt version, fail2ban, the metadata-endpoint DROP, the compose project under /opt/rome, optional rome-hostd) with three runners over the same idempotent provision/ steps — build.sh (offline virt-customize into a qcow2, Rome image preloaded by digest via skopeo), apply.sh (the same steps over SSH on a live host), and dev/boot.sh (KVM + NoCloud seed). The direction is clearly correct: it replaces three divergent provisioning paths with one tree, the "the Rome image is not part of the host layer" split is the right seam, and choosing virt-customize + skopeo over Packer is well-justified given no boot is needed. The scripts themselves are unusually careful — set -euo pipefail throughout, .part-then-rename for every artifact, digest-pinned base image verified with sha256sum -c, fail2ban-client -t validating the jail offline, the atomic install-beside-and-rename for a running rome-hostd, and a build-only seal step.
Two issues should block. First, provision/30-rome.sh unconditionally installs files/docker-compose.yml over /opt/rome/docker-compose.yml, and run.sh apply then runs docker compose up -d — but that file's own header says "Production uses Rome Cloud's deployment bundle; this file is the subset needed to prove the image boots Rome." Since apply.sh is explicitly for bringing existing tenants up to the tree, this path replaces a tenant's real stack (otel-collector, healthcheck, shm_size, tailscale/socket mounts) with the test subset and recreates containers from it. Second, --wechat bakes "enabled": true into /etc/rome-host/config.json alongside ROME_HOST_EXECUTION_ENABLED: "true" in the compose override, which contradicts the invariant in docs/architecture/host-execution.md that installation and permission to execute are separate gates; the helper's own README example ships "enabled": false. Beyond those, several P2s around pin completeness (containerd.io unpinned/unheld, the Rome tar cached by tag rather than digest), idempotency coupling (fail2ban's install hidden inside the docker-version guard), and an apply.sh argument-parsing bug that mis-handles valueless ssh flags.
Verdict: REQUEST_CHANGES — The direction is right and the scripts are well-crafted, but apply.sh on a live tenant clobbers the production compose project with a file the PR itself calls a proof-of-boot subset, and --wechat bakes an unconditionally-enabled host-root helper into the image, collapsing a documented two-party authorization gate.
13 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | architecture | infra/vm/provision/30-rome.sh |
apply.sh clobbers a live tenant's production compose project with the proof-of-boot subset |
| P1 | security | infra/vm/files/rome-host-config.json |
--wechat bakes an unconditionally-enabled host-root helper into the image |
| P2 | error-handling | infra/vm/provision/10-docker.sh |
fail2ban's install is gated behind the docker-ce version check |
| P2 | design | infra/vm/provision/10-docker.sh |
containerd.io is the one unpinned, unheld input |
| P2 | design | infra/vm/build.sh |
The preloaded Rome tar is cached by tag, not by digest |
| P2 | code-quality | infra/vm/apply.sh |
apply.sh's -* branch always consumes two arguments |
| P2 | error-handling | infra/vm/provision/run.sh |
Every apply restarts rome-hostd, orphaning any in-flight root job |
| P2 | design | infra/vm/files/docker-compose.yml |
Base compose omits shm_size, the exact trap the WeChat doc already documents |
| P2 | security | infra/vm/files/rome-host-config.json |
A fleet-constant hostId undermines the seal step's purpose |
| P2 | security | infra/vm/provision/90-seal.sh |
Seal does not clear the systemd random seed |
| P3 | error-handling | infra/vm/files/rome-block-metadata.service |
The metadata DROP is not re-asserted when dockerd restarts |
| P3 | code-quality | infra/vm/dev/boot.sh |
dev/boot.sh is x86-only while build.sh takes --arch arm64 |
| P3 | code-quality | infra/vm/apply.sh |
apply.sh never prunes files removed from the tree |
| set -euo pipefail | ||
| files=/etc/rome-host/files | ||
| mkdir -p /opt/rome /var/lib/rome | ||
| install -m 0644 "$files/docker-compose.yml" /opt/rome/docker-compose.yml |
There was a problem hiding this comment.
[P1] architecture — apply.sh clobbers a live tenant's production compose project with the proof-of-boot subset
files/docker-compose.yml says of itself: "Production uses Rome Cloud's deployment bundle; this file is the subset needed to prove the image boots Rome." But this line installs it unconditionally over /opt/rome/docker-compose.yml, and run.sh apply then runs docker compose up -d in that directory. Since apply.sh is explicitly for existing tenants (README: "existing machines through apply.sh, which Rome Cloud can call over the SSH channel its upgrade script already uses"), applying to a live host silently replaces the real stack with the subset and recreates containers from it — dropping otel-collector, the healthcheck, shm_size, and the tailscale/host-socket mounts. 40-wechat.sh:16 has the same problem with docker-compose.override.yml.
Make the compose file a build-time seed rather than something apply enforces, e.g. only write it when absent, or install it under a host-layer-owned name the runner never overwrites:
| install -m 0644 "$files/docker-compose.yml" /opt/rome/docker-compose.yml | |
| [[ -f /opt/rome/docker-compose.yml ]] || install -m 0644 "$files/docker-compose.yml" /opt/rome/docker-compose.yml |
If the intent really is for the host layer to own the compose project on every tenant, then this file must stop being a "subset" and become the production definition — and the README/header comment should say so.
There was a problem hiding this comment.
Fixed: 30-rome.sh writes the compose file only when /opt/rome has none, so a tenant's bundle is never replaced. Verified on the dev VM by editing the file and applying; the edit survived. The WeChat override remains host-layer owned because it is the enabling mechanism, and --no-wechat removes it. The file's header now says what it is for: hosts that boot without a bundle.
| @@ -0,0 +1 @@ | |||
| {"hostId":"rome-host","enabled":true,"socketPath":"/run/rome-host/control.sock","stateDir":"/var/lib/rome-host","socketGid":0,"maxTimeoutSeconds":600,"maxOutputBytes":131072} | |||
There was a problem hiding this comment.
[P1] security — --wechat bakes an unconditionally-enabled host-root helper into the image
This ships "enabled": true, and docker-compose.override.wechat.yml simultaneously sets ROME_HOST_EXECUTION_ENABLED: "true" and ROME_DOCKER_USER_MODE: root. docs/architecture/host-execution.md states the invariant "Installation and permission to execute are separate. Both Rome and the host helper reject execution until explicitly enabled", and packages/host-helper/README.md's example config uses "enabled": false. A single --wechat build flag now flips both gates at image-build time, so every VM cloned from that image accepts root job submissions from the container before any operator decision — and the image is the artifact a tenant boots.
Ship "enabled": false in the baked config and let the enabling step be a per-instance decision (seed/.env or an explicit apply.sh flag), so that installing the helper and granting it authority remain two separate acts.
There was a problem hiding this comment.
Fixed. build.sh --hostd installs the binary and unit with "enabled": false and no override, and does not enable the service. Enabling is a separate per-host act: apply.sh --wechat or a first-boot seed sets the /etc/rome-host/wechat marker, and only then does 40-wechat.sh write an enabled config and the override. --no-wechat reverses it. Verified both directions on the dev VM.
|
|
||
| if ! dpkg -s docker-ce 2>/dev/null | grep -q "^Version: ${DOCKER_CE_VERSION}$"; then | ||
| apt-get update | ||
| apt-get install -y --no-install-recommends ca-certificates curl gnupg fail2ban |
There was a problem hiding this comment.
[P2] error-handling — fail2ban's install is gated behind the docker-ce version check
fail2ban (plus ca-certificates/curl/gnupg) is installed inside the if ! dpkg -s docker-ce | grep -q Version: $PIN block. On any host that already has Docker at the pin but no fail2ban — a future host-layer bump that touches only files/, or a tenant provisioned differently — the package is never installed, and 20-harden.sh then fails at install ... /etc/fail2ban/jail.d/rome-sshd.local (directory missing) and fail2ban-client -t (command not found), aborting the apply with a confusing error. run.sh's systemctl restart fail2ban.service would fail for the same reason.
Two unrelated concerns share one guard. Move the fail2ban install out so its presence is ensured independently and idempotently:
dpkg -s fail2ban >/dev/null 2>&1 || { apt-get update && apt-get install -y --no-install-recommends fail2ban; }There was a problem hiding this comment.
Fixed in 04a98ae: fail2ban installs in 20-harden.sh when dpkg -s says it is missing, independent of the Docker branch.
| apt-get install -y --no-install-recommends --allow-downgrades \ | ||
| "docker-ce=${DOCKER_CE_VERSION}" \ | ||
| "docker-ce-cli=${DOCKER_CE_VERSION}" \ | ||
| containerd.io \ |
There was a problem hiding this comment.
[P2] design — containerd.io is the one unpinned, unheld input
pins.env and the PR description both claim "every input is pinned," but containerd.io is installed unversioned and is absent from the apt-mark hold on line 28. Two builds from the same tree can therefore ship different container runtimes, and an unattended-upgrades run can move containerd off whatever was built — exactly the drift apt-mark hold exists to prevent for the other three packages. Add CONTAINERD_VERSION to pins.env, install "containerd.io=${CONTAINERD_VERSION}", and include it in both the unhold and hold lists.
There was a problem hiding this comment.
Fixed: CONTAINERD_VERSION in pins.env, installed at that version and included in both the unhold and hold lists.
| # 3. Rome image by digest, one platform, no daemon. The digest pin is the | ||
| # integrity check, so the signature policy accepts anything. docker-archive is what | ||
| # `docker load` consumes on first boot. | ||
| rome_tar="$out/rome-${arch}-${ROME_IMAGE_TAG}.tar" |
There was a problem hiding this comment.
[P2] design — The preloaded Rome tar is cached by tag, not by digest
rome_tar is keyed on $ROME_IMAGE_TAG, but the pin that actually identifies the content is $ROME_IMAGE_DIGEST. skopeo verifies the digest only on a cache miss; if the digest is bumped while the tag stays the same (a rebuild/retag of 1.1.115, or a correction to a mis-recorded digest), [[ ! -s "$rome_tar" ]] is false and the stale tar is baked in silently — defeating the "every input is verified before use" invariant that the base image gets via sha256sum -c.
| rome_tar="$out/rome-${arch}-${ROME_IMAGE_TAG}.tar" | |
| rome_tar="$out/rome-${arch}-${ROME_IMAGE_DIGEST#sha256:}.tar" |
There was a problem hiding this comment.
Fixed: the tar is keyed on the digest.
| @@ -0,0 +1 @@ | |||
| {"hostId":"rome-host","enabled":true,"socketPath":"/run/rome-host/control.sock","stateDir":"/var/lib/rome-host","socketGid":0,"maxTimeoutSeconds":600,"maxOutputBytes":131072} | |||
There was a problem hiding this comment.
[P2] security — A fleet-constant hostId undermines the seal step's purpose
"hostId": "rome-host" is baked into the image, so every VM cloned from it reports the same host identity in GET /v1/capabilities. 90-seal.sh exists specifically so each clone generates its own machine identity, and the host-execution contracts say "The helper never silently changes the execution target" and "Replace the host identity and retire the old target when provisioning a fresh job store." A constant identity makes that check vacuous across tenants and across a rebuild-and-restore. Generate hostId at first boot from /etc/machine-id (or the instance id the seed already writes) instead of shipping a literal.
There was a problem hiding this comment.
Fixed: hostId is derived from /etc/machine-id when the helper is enabled on a host, never baked. A sealed image carries an empty, disabled config.
| # generates its own. Never run on a live host. | ||
| set -euo pipefail | ||
| truncate -s0 /etc/machine-id | ||
| rm -f /var/lib/dbus/machine-id /etc/ssh/ssh_host_* |
There was a problem hiding this comment.
[P2] security — Seal does not clear the systemd random seed
The step truncates /etc/machine-id and removes the dbus id and ssh host keys, but leaves /var/lib/systemd/random-seed — so every VM cloned from this image credits the same seed into the kernel entropy pool at first boot, which is what the seed file's own documentation warns against for images. Add rm -f /var/lib/systemd/random-seed (and consider /var/lib/dhcp/*, /var/log/*). Separately, cloud-init clean --logs || true swallows failure: if the clean fails, /var/lib/cloud/instance* stays baked in and the seed on first boot is ignored, which is a silent and confusing failure mode — let it fail the build, or at least log loudly.
There was a problem hiding this comment.
Fixed: the seal removes /var/lib/systemd/random-seed and DHCP leases, and cloud-init clean failing now fails the build.
| [Unit] | ||
| Description=Block container egress to the cloud metadata endpoint | ||
| After=docker.service | ||
| Requires=docker.service |
There was a problem hiding this comment.
[P3] error-handling — The metadata DROP is not re-asserted when dockerd restarts
The unit is Type=oneshot + RemainAfterExit=yes with Requires=docker.service. Requires= does not propagate a restart, so after systemctl restart docker (which dev/boot.sh's runcmd does, and which any Docker upgrade does) the unit stays active (exited) and never re-runs, while dockerd rebuilds its iptables chains. Adding PartOf=docker.service makes systemd stop-and-restart it with dockerd, so the DROP is always re-checked. Worth noting the rule is IPv4-only and lives in DOCKER-USER, so it covers bridge-network egress only — fine for parity with the current cloud-init, but worth stating in the README as a known boundary.
There was a problem hiding this comment.
Fixed in 04a98ae: PartOf=docker.service plus a docker.service.d drop-in with Wants=, verified across a manual docker restart. The DOCKER-USER scope is in the README's known boundaries.
| # ROME_VM_CONSOLE_LOG=path detaches the console to a file for scripted runs. | ||
| console=(-nographic -serial mon:stdio) | ||
| [[ -n "${ROME_VM_CONSOLE_LOG:-}" ]] && console=(-display none -monitor none -serial "file:$ROME_VM_CONSOLE_LOG") | ||
| exec qemu-system-x86_64 -enable-kvm -cpu host -smp 4 -m 6G "${console[@]}" \ |
There was a problem hiding this comment.
[P3] code-quality — dev/boot.sh is x86-only while build.sh takes --arch arm64
qemu-system-x86_64, the OVMF discovery rooted at that binary's prefix, and -cpu host -enable-kvm hardcode amd64, yet build.sh --arch arm64 is supported and the README names the arm64 build as the desktop's next step. Booting an arm64 artifact here fails in a non-obvious way. Either derive the qemu binary and firmware from the image name / an --arch flag, or add an explicit guard that says "dev/boot.sh supports amd64 images only" so the failure is legible.
There was a problem hiding this comment.
Fixed in 04a98ae: the script rejects non-amd64 images and non-x86_64 hosts with an explicit message.
| run() { ssh "${ssh_args[@]}" "$target" "$@"; } | ||
|
|
||
| # The tree lands whole; run.sh reads pins.env and files/ from there. | ||
| tar -C "$here" -c pins.env provision files | |
There was a problem hiding this comment.
[P3] code-quality — apply.sh never prunes files removed from the tree
tar -x -C /etc/rome-host overlays but never deletes, so a files/ entry or provision/ step removed from the repo lingers on every previously-applied host — and a lingering provision/NN-*.sh is inert only because run.sh enumerates steps by name, while a stale files/*.service could still be referenced by an old enabled symlink. Since the README sells /etc/rome-host as "provenance," it should reflect the tree exactly: extract into a fresh directory and swap it in, or rm -rf /etc/rome-host/{provision,files} before extracting (keeping the wechat marker and applied).
There was a problem hiding this comment.
Fixed: provision/ and files/ are removed before the extract; per-host state beside them is kept.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
…containerd - 30-rome.sh seeds /opt/rome/docker-compose.yml only when absent, so apply never replaces a tenant's Rome Cloud bundle - the host helper installs disabled (build --hostd); enabling is a per-host act via apply.sh --wechat or the first-boot seed, with hostId derived from /etc/machine-id, and --no-wechat reverses it - rome-hostd restarts only when its binary, unit, or config changed - containerd.io pinned and held; the Rome tar is cached by digest - seal removes the systemd random seed and fails on a failed cloud-init clean - apply.sh replaces provision/ and files/ whole; empty arrays are safe under set -u; the metadata block also covers the IPv6 metadata address - base compose carries shm_size and the healthcheck Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
…oss boots - 40-wechat.sh disables and stops rome-hostd when the marker is absent; run.sh apply starts it when enabled and restarts only on change - dev/boot.sh writes the seed once with the overlay, so the JWT secret and instance id survive later boots - build.sh stages under the output dir and drops a base image that fails its checksum - README drops the reference to scripts/vm/vm.sh and states the pin and version semantics Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Addressed the third review round in 3b199c4 (the bots removed their inline comments, so replying here):
|
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
…he helper config valid when disabled - pins.env carries the sha256 of Docker's apt signing key; 10-docker.sh verifies it before trusting the repository - build.sh refuses a pins.env whose tag resolves to a different digest, and refuses --arch that the host cannot build - a disabled helper config keeps a valid hostId, the build placeholder is reset by the seal, so the unit starts if asked - run.sh apply takes the compose project name from the file Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Fourth round, on the review of c5109d2, addressed in d63926e:
|
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
…, guard the helper path - provision/lib.sh tracks which files a step changed; run.sh apply restarts fail2ban, the metadata unit, rome-hostd, and the compose project only when their inputs moved - 10-docker.sh reinstalls when any of the four pinned packages is off its pin, not only docker-ce - the IPv6 metadata rule cannot fail the unit on hosts without IPv6 - run.sh refuses --wechat on a host with no helper installed, and build.sh refuses a --hostd that is not a linux/<arch> ELF - /etc/rome-host gets fixed modes regardless of the build umask - dev/boot.sh picks the plain OVMF firmware and rejects --wechat on an already-booted VM Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Fifth round, on the reviews of 3b199c4, addressed in 42bf40c:
|
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
… the image load room - run.sh validates its mode first, then makes the tree root-owned with fixed modes before running anything from it; apply.sh no longer restores the developer's permissions - /etc/rome-host/applied records TREE_SHA256 beside HOST_LAYER_VERSION, so a host states what it actually received - rome-load-image.service gets a 30 minute start timeout - 10-docker.sh refuses a host whose Ubuntu release is not the pinned one - dev/boot.sh accepts flags before or after the image and rejects unknown ones Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Sixth round. Both reviews ran against d63926e, one commit behind. Several of their items were already fixed in 42bf40c: the pin gate across all four Docker packages, the unconditional fail2ban restart, the The rest is addressed in 66b14f5:
These stay as they are, and the README records why:
|
Jessie-QingYu
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR adds infra/vm: a single definition of the Rome host layer (pinned Docker, fail2ban, metadata-egress block, the /opt/rome compose project, optional rome-hostd) with three runners — build.sh (offline virt-customize into a qcow2), apply.sh (same steps over SSH on a live host), and dev/boot.sh (KVM + NoCloud seed). The core design is sound and unusually well thought through: one idempotent step set shared by all three runners, every input pinned and verified before use (base image sha256, Docker apt key sha256, Rome image manifest digest recomputed from skopeo inspect --raw), install-vs-enable kept as two acts for the host helper, identity scrubbing isolated to a build-only step, and a /run/rome-host-changes marker scheme so a converged apply restarts nothing.
I verified beyond the diff: read all 20 new files in full, cross-checked files/*.service and the generated config.json against packages/host-helper/internal/hosthelper/{config,server,listener}.go (hostId regex, protectedFile/protectedPath mode requirements, socket creation and chown — the chmod -R go-w + install -m 0644 in 30-rome.sh/40-wechat.sh do satisfy the helper's checks), compared files/docker-compose.yml against the root docker-compose.yml and compose.dev.yml, confirmed python3-systemd is a hard Depends on noble (so --no-install-recommends fail2ban + backend = systemd is fine), confirmed bash -n passes on all ten scripts and that set -e does not trip on the bare a && b lines, and confirmed scripts/lint-shell.sh will pick these files up in CI. The blockers below are in the apply.sh ↔ run.sh ↔ 40-wechat.sh seam, where the runner duplicates state changes that the steps already own.
Verdict: REQUEST_CHANGES — Two concrete bugs on documented flows (apply.sh --wechat --hostd aborts, and --no-wechat silently skips the container recreate that revokes host-execution env) plus a design gap around build-time seeding of the tenant compose file.
10 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | error-handling | infra/vm/provision/run.sh |
apply.sh --wechat --hostd <path> always aborts on a host that has no helper yet |
| P1 | security | infra/vm/apply.sh |
apply.sh --no-wechat deletes the override itself, so the running container keeps host-execution and root mode |
| P2 | architecture | infra/vm/provision/30-rome.sh |
Build-time seeding means every image ships /opt/rome/docker-compose.yml, inverting the "never overwrite a tenant bundle" rule |
| P2 | design | infra/vm/files/docker-compose.yml |
Third copy of the Rome service definition, already drifting from the root compose |
| P2 | error-handling | infra/vm/dev/boot.sh |
OVMF firmware discovery can pair a 4M CODE with a 2M VARS |
| P2 | code-quality | infra/vm/provision/20-harden.sh |
Change-marker names are a stringly-typed contract with an unconsumed producer |
| P3 | error-handling | infra/vm/apply.sh |
apply.sh --hostd skips the ELF/arch validation build.sh performs |
| P3 | code-quality | infra/vm/apply.sh |
Non-atomic tree replacement can leave a host with no provision tree |
| P3 | code-quality | infra/vm/dev/boot.sh |
Stale references: a script that does not exist and a marker path that is not used |
| P3 | code-quality | infra/vm/build.sh |
Build preflight and cleanup gaps |
| . "$root/provision/lib.sh" | ||
| rm -rf /run/rome-host-changes | ||
|
|
||
| if [[ -e "$root/wechat" && ! -x /usr/local/bin/rome-hostd ]]; then |
There was a problem hiding this comment.
[P1] error-handling — apply.sh --wechat --hostd <path> always aborts on a host that has no helper yet
This guard only looks at /usr/local/bin/rome-hostd, but apply.sh stages the new binary at /etc/rome-host/rome-hostd and lets 40-wechat.sh install it. So the documented first-time enable — apply.sh --wechat --hostd packages/host-helper/dist/rome-hostd host — touches the marker, uploads the binary, then aborts with "no helper is installed; apply with --hostd PATH" even though --hostd was passed. The operator has to run apply twice. (The test plan exercised --hostd and --wechat separately, which is why this slipped through.)
| if [[ -e "$root/wechat" && ! -x /usr/local/bin/rome-hostd ]]; then | |
| if [[ -e "$root/wechat" && ! -x /usr/local/bin/rome-hostd && ! -f "$root/rome-hostd" ]]; then |
| run 'sudo rm -rf /etc/rome-host/provision /etc/rome-host/files && sudo mkdir -p /etc/rome-host && sudo tar -x -C /etc/rome-host --no-same-owner' | ||
| case "$wechat" in | ||
| on) run 'sudo touch /etc/rome-host/wechat' ;; | ||
| off) run 'sudo rm -f /etc/rome-host/wechat /opt/rome/docker-compose.override.yml' ;; |
There was a problem hiding this comment.
[P1] security — apply.sh --no-wechat deletes the override itself, so the running container keeps host-execution and root mode
40-wechat.sh already owns /opt/rome/docker-compose.override.yml — it removes the file and calls mark_changed docker-compose.override.yml, which is what makes run.sh re-run docker compose up -d. Because apply.sh pre-deletes the file over SSH, 40-wechat.sh's [[ -f ... ]] is already false, no mark is set, and the stack is never recreated. Net effect: the helper is stopped and disabled, but the live container keeps ROME_HOST_EXECUTION_ENABLED=true, ROME_DOCKER_USER_MODE=root and the /run/rome-host bind mount until something else restarts it — an incomplete revocation of a privilege path. It also blindly deletes an override a tenant's Rome Cloud bundle may own for unrelated reasons. Let the step own the file:
| off) run 'sudo rm -f /etc/rome-host/wechat /opt/rome/docker-compose.override.yml' ;; | |
| off) run 'sudo rm -f /etc/rome-host/wechat' ;; |
| set -euo pipefail | ||
| files=/etc/rome-host/files | ||
| mkdir -p /opt/rome /var/lib/rome | ||
| [[ -f /opt/rome/docker-compose.yml ]] || install -m 0644 "$files/docker-compose.yml" /opt/rome/docker-compose.yml |
There was a problem hiding this comment.
[P2] architecture — Build-time seeding means every image ships /opt/rome/docker-compose.yml, inverting the "never overwrite a tenant bundle" rule
run.sh build runs this step inside the image, so /opt/rome is never empty on a machine booted from the artifact. The "seed only when absent" guard therefore protects the image's compose file on every tenant, and the Rome Cloud bundle now has to clobber it at first boot to take effect — the opposite of the invariant the header comment states. Worth either skipping the seed in build mode (leave /opt/rome empty; dev's seed writes it) or recording provenance (e.g. a /etc/rome-host/compose-seeded marker) so apply.sh can tell a host-layer seed from a tenant bundle and the follow-up vultr.ts change has a deterministic contract to target.
| rome: | ||
| image: ${ROME_DOCKER_IMAGE} | ||
| ports: | ||
| - "8080:8080" |
There was a problem hiding this comment.
[P2] design — Third copy of the Rome service definition, already drifting from the root compose
This is now the third compose definition of the rome service (root docker-compose.yml, compose.dev.yml, this one) and it has already diverged: no start_period: 30s, so with interval: 30s / retries: 3 a container that takes >90s to boot reports unhealthy on a fresh host; ROME_HOST_EXECUTION_SOCKET_DIR and the ROME_DOCKER_USER_MODE default are gone. Separately, "8080:8080" publishes on 0.0.0.0 and Docker's publish path bypasses ufw/nftables user rules, so an internet-facing self-hosted box is exposed the moment docker compose up runs — the file comment acknowledges this but nothing in 20-harden.sh closes it. Consider at minimum adding start_period: 30s and documenting a 127.0.0.1-bound publish option, and noting in the README which file is authoritative for the service shape.
| ovmf_dir="${OVMF_DIR:-$(dirname "$(dirname "$(command -v qemu-system-x86_64)")")/share/OVMF}" | ||
| [[ -d "$ovmf_dir" ]] || ovmf_dir="${OVMF_FD:?set OVMF_DIR or OVMF_FD to the OVMF firmware dir}" | ||
| # The plain firmware, not a secure-boot build: an unsigned guest would not boot. | ||
| code="$(find "$ovmf_dir" -name 'OVMF_CODE.fd' -o -name 'OVMF_CODE_4M.fd' | head -1)" |
There was a problem hiding this comment.
[P2] error-handling — OVMF firmware discovery can pair a 4M CODE with a 2M VARS
code and vars_src are resolved by two independent find ... -o ... | head -1 calls. On a Debian/Ubuntu host /usr/share/OVMF contains OVMF_CODE.fd, OVMF_CODE_4M.fd, OVMF_VARS.fd and OVMF_VARS_4M.fd; readdir order decides which each head -1 wins, so a _4M CODE can be paired with a 2M VARS and the guest fails to boot or silently loses variable persistence. Resolve them as a pair (prefer _4M for both, else the plain pair) rather than independently. Minor second issue on the same lines: find | head -1 under set -o pipefail returns 141 if find gets SIGPIPE, which would abort the script under set -e.
| rm -rf /var/lib/apt/lists/* | ||
| fi | ||
| install -d /etc/systemd/system/docker.service.d | ||
| install_if_changed "$files/docker-rome-block-metadata.conf" /etc/systemd/system/docker.service.d/rome-block-metadata.conf |
There was a problem hiding this comment.
[P2] code-quality — Change-marker names are a stringly-typed contract with an unconsumed producer
install_if_changed derives the marker from basename "$dst", so this call marks rome-block-metadata.conf — a name run.sh never checks (it checks rome-block-metadata.service, rome-sshd.local, rome-hostd, docker-compose.override.yml). Nothing acts when the docker.service drop-in changes. The impact today is small because the drop-in only matters at the next Docker start, but the contract is invisible: producer and consumer agree by string across three files with no failure mode when they disagree. Either pass an explicit marker name to install_if_changed (install_if_changed SRC DST MODE MARK) or have run.sh iterate /run/rome-host-changes against an explicit name→action table so an unknown mark is loud.
| off) run 'sudo rm -f /etc/rome-host/wechat /opt/rome/docker-compose.override.yml' ;; | ||
| esac | ||
| if [[ -n "$hostd" ]]; then | ||
| run 'sudo tee /etc/rome-host/rome-hostd >/dev/null' <"$hostd" |
There was a problem hiding this comment.
[P3] error-handling — apply.sh --hostd skips the ELF/arch validation build.sh performs
build.sh checks the magic and e_machine before staging the binary; apply.sh pipes whatever the path points at straight into /etc/rome-host/rome-hostd. Pushing a darwin or wrong-arch build to a live tenant installs it and only fails when systemd tries to start it. Factor the ELF check into a small shared helper (or provision/lib.sh) and call it from both runners.
| # The tree replaces what the host had, so a file removed from the tree is | ||
| # removed from the host too. Per-host state (the wechat marker, applied, | ||
| # config.json) lives beside the tree and is kept. | ||
| tar -C "$here" -c pins.env provision files | |
There was a problem hiding this comment.
[P3] code-quality — Non-atomic tree replacement can leave a host with no provision tree
sudo rm -rf /etc/rome-host/{provision,files} && ... tar -x deletes first and extracts second over the same SSH session. A dropped connection or a tar error leaves the host with a half-populated or empty tree, and run.sh is then unrunnable until the next successful apply. Extracting into /etc/rome-host/.new and swapping the two directories afterwards would make the replacement atomic-ish for the same handful of lines.
| - $pubkey | ||
| write_files: | ||
| # Dev loop: the guest pulls worktree builds from the host registry on the | ||
| # slirp gateway over plain HTTP, the same registry scripts/vm/vm.sh pushes to. |
There was a problem hiding this comment.
[P3] code-quality — Stale references: a script that does not exist and a marker path that is not used
This comment points at scripts/vm/vm.sh, which is not in the tree (the PR description says that script is untracked in a few worktrees). Same class of issue in infra/vm/provision/40-wechat.sh:13, whose header says it "Writes /run/rome-host/changed" when the actual marker is /run/rome-host-changes/rome-hostd. Both are the kind of comment a reader trusts and then cannot verify.
| # target arch with: CGO_ENABLED=0 GOOS=linux GOARCH=<arch> go build -o | ||
| # rome-hostd ./cmd/rome-hostd from packages/host-helper. | ||
| # | ||
| # Needs on PATH: virt-customize virt-resize qemu-img skopeo curl sha256sum. |
There was a problem hiding this comment.
[P3] code-quality — Build preflight and cleanup gaps
Three small things on the build path: (1) the required-tools list lives only in this comment — a missing skopeo or virt-resize surfaces as a bare command not found partway through a multi-GB download, and the flake devShell deliberately ships none of them while the README says "All from nixpkgs"; a for t in virt-customize virt-resize qemu-img skopeo curl; do command -v ...; done preflight is a few lines. (2) A failed virt-customize leaves a multi-GB $image.part behind (the EXIT trap only covers $stage). (3) A cached $rome_tar short-circuits the digest verification entirely — the filename embeds the digest, but the bytes are never re-checked.
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This is a well-structured, carefully documented PR that consolidates three ad-hoc host-provisioning paths into one idempotent tree with three runners over the same steps. The design (host layer vs. Rome image separation, install-vs-enable split for the helper, pin-and-verify every input, change-marker driven restarts) is sound, and the scripts are defensive (arch checks, digest verification, tag/digest drift check, offline sealing). The change-detection mechanism (install_if_changed → mark_changed → changed) is a clean way to make a converged apply a no-op.\n\nThe main issue is an asymmetry in the WeChat enable/disable flow: apply.sh --no-wechat removes /opt/rome/docker-compose.override.yml itself, which pre-empts 40-wechat.sh's change detection and prevents run.sh from recreating the container. Enabling works (the override is created by the provision step, which marks the change), but disabling leaves the container running with ROME_DOCKER_USER_MODE=root and the host socket bind-mount until some other change triggers a recreate — the opposite of what the operator asked for. A couple of minor idempotency nits are noted as well.
Verdict: REQUEST_CHANGES — The WeChat disable path (apply.sh --no-wechat) deletes the compose override before the provision step can detect it, so the running container is never recreated and keeps its elevated (root + socket) configuration until an unrelated recreate.
3 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | error-handling | infra/vm/apply.sh |
apply.sh --no-wechat deletes the override before provision can react, so the container is never recreated |
| P3 | code-quality | infra/vm/provision/30-rome.sh |
30-rome.sh reinstalls rome-load-image.service unconditionally on every apply |
| P3 | error-handling | infra/vm/apply.sh |
apply.sh installs --hostd binary without an arch check |
| run 'sudo rm -rf /etc/rome-host/provision /etc/rome-host/files && sudo mkdir -p /etc/rome-host && sudo tar -x -C /etc/rome-host --no-same-owner --no-same-permissions' | ||
| case "$wechat" in | ||
| on) run 'sudo touch /etc/rome-host/wechat' ;; | ||
| off) run 'sudo rm -f /etc/rome-host/wechat /opt/rome/docker-compose.override.yml' ;; |
There was a problem hiding this comment.
[P1] error-handling — apply.sh --no-wechat deletes the override before provision can react, so the container is never recreated
On disable, apply.sh removes /opt/rome/docker-compose.override.yml itself. When run.sh apply then runs 40-wechat.sh, the file is already gone, so the disable branch's [[ -f /opt/rome/docker-compose.override.yml ]] check is false and mark_changed docker-compose.override.yml never fires. Consequently run.sh's reconcile (changed docker-compose.override.yml && ... docker compose up -d) does not run, and the already-running container keeps ROME_DOCKER_USER_MODE=root, the WeChat env, and the /run/rome-host bind mount until an unrelated recreate. Compare the enable path, which works precisely because 40-wechat.sh creates the override via install_if_changed and thus marks the change.
Let the provision step own the override in both directions — drop the file removal here so 40-wechat.sh can detect and mark it:
| off) run 'sudo rm -f /etc/rome-host/wechat /opt/rome/docker-compose.override.yml' ;; | |
| off) run 'sudo rm -f /etc/rome-host/wechat' ;; |
(40-wechat.sh already removes the override and calls mark_changed docker-compose.override.yml when the marker is absent, which will trigger the recreate.)
| files=/etc/rome-host/files | ||
| mkdir -p /opt/rome /var/lib/rome | ||
| [[ -f /opt/rome/docker-compose.yml ]] || install -m 0644 "$files/docker-compose.yml" /opt/rome/docker-compose.yml | ||
| install -m 0644 "$files/rome-load-image.service" /etc/systemd/system/rome-load-image.service |
There was a problem hiding this comment.
[P3] code-quality — 30-rome.sh reinstalls rome-load-image.service unconditionally on every apply
install -m 0644 "$files/rome-load-image.service" ... runs unconditionally, rewriting the unit file (new inode/mtime) on every apply even when content is identical. It's harmless because run.sh always runs systemctl daemon-reload and the unit is a oneshot, but it slightly undercuts the "applying to a host built from the same tree changes nothing" invariant. Consider using install_if_changed here for consistency with the other unit installs (the load unit has no restart action, so no mark_changed handling is needed).
| off) run 'sudo rm -f /etc/rome-host/wechat /opt/rome/docker-compose.override.yml' ;; | ||
| esac | ||
| if [[ -n "$hostd" ]]; then | ||
| run 'sudo tee /etc/rome-host/rome-hostd >/dev/null' <"$hostd" |
There was a problem hiding this comment.
[P3] error-handling — apply.sh installs --hostd binary without an arch check
build.sh validates that --hostd is a linux/ ELF (e_machine check), but apply.sh tees the binary to the host with no such guard. A wrong-arch binary would install cleanly and only fail at rome-hostd start time, which is harder to diagnose over SSH. Optional: mirror build.sh's ELF magic/e_machine check, or at least verify against the target's uname -m before installing.
Jessie-QingYu
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR introduces infra/vm: one idempotent definition of the Rome host layer (pinned Docker, fail2ban, the DOCKER-USER metadata block, the /opt/rome compose project, optional rome-hostd) driven by three runners — build.sh (offline virt-customize), apply.sh (over SSH), and dev/boot.sh (KVM with a NoCloud seed). The direction is good and better than the alternatives argued in the description: collapsing three divergent provisioning paths into one tree, pinning every input, keeping install-vs-enable separate for host execution, and making dev and production boot the identical image are all the right calls. The scripts are careful (digest-verified base image and Rome image, apt pin + hold, ELF arch check on the helper binary, change-tracking so a converged apply restarts nothing), and they pass the repo's own shellcheck -x --severity=warning gate. I read every file in the diff in full plus the surrounding context (docker-compose.yml, docker-entrypoint.sh, packages/host-helper/*, docs/architecture/host-execution.md, docs/wechat-personal.md, scripts/lint-shell.sh, scripts/ci-changed-areas.mjs).
My blocking concern is the apply.sh path against existing hosts, which is the PR's headline benefit. provision/40-wechat.sh claims ownership of /etc/rome-host/config.json, rome-hostd.service, and the compose override for any host that merely has /usr/local/bin/rome-hostd present — but packages/host-helper/README.md says Rome Cloud manages exactly those through its deployment bundle. A plain apply.sh against a tenant with host execution enabled rewrites its hostId/socketGid, then stops and disables the helper. 30-rome.sh deliberately refuses to overwrite a tenant's compose bundle; the helper's config deserves the same protection. The second bug is that apply.sh --no-wechat deletes the compose override itself, which defeats the change detection in 40-wechat.sh and so skips the docker compose up -d — the container keeps WECHAT_USER_ENABLED=true, ROME_HOST_EXECUTION_ENABLED=true and ROME_DOCKER_USER_MODE=root until someone recreates it by hand. Everything else is P2/P3.
Verdict: REQUEST_CHANGES — The design is sound and unusually well documented, but two apply-path bugs make apply.sh unsafe on the existing tenants it is meant to converge: it silently rewrites/disables a Rome-Cloud-managed host helper, and --no-wechat leaves the container running with WeChat + root mode.
9 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | architecture | infra/vm/provision/40-wechat.sh |
apply.sh rewrites and disables a Rome-Cloud-managed host helper on an existing tenant |
| P1 | error-handling | infra/vm/apply.sh |
apply.sh --no-wechat leaves the container running with WeChat and root mode |
| P2 | security | infra/vm/provision/run.sh |
run.sh blanket-chmods every file under /etc/rome-host to 0644 |
| P2 | architecture | infra/vm/files/docker-compose.yml |
A second compose definition of the rome service that will drift from the production one |
| P2 | error-handling | infra/vm/apply.sh |
apply.sh does not validate the --hostd binary the way build.sh does |
| P2 | error-handling | infra/vm/apply.sh |
Tree replacement on the host is not atomic |
| P3 | code-quality | infra/vm/dev/boot.sh |
Comment points at a script that does not exist in the repo |
| P3 | design | infra/vm/files/rome-load-image.service |
The preload accelerator depends on unstated seed ordering |
| P3 | security | infra/vm/files/docker-compose.yml |
Seed compose publishes 8080 on every interface with no host firewall |
| fi | ||
| rm -f "$root/rome-hostd" | ||
| fi | ||
| [[ -x /usr/local/bin/rome-hostd ]] || exit 0 |
There was a problem hiding this comment.
[P1] architecture — apply.sh rewrites and disables a Rome-Cloud-managed host helper on an existing tenant
This gate makes the host layer the owner of rome-hostd.service, /etc/rome-host/config.json and /opt/rome/docker-compose.override.yml for any host that already has the binary installed — but packages/host-helper/README.md states that "Rome Cloud manages the binary, systemd unit, socket mount, and instance policy through its deployment bundle". On an existing tenant with host execution enabled and no /etc/rome-host/wechat marker (no old tenant has one), a plain apply.sh will: overwrite the unit, rewrite config.json with hostId=$(cut -c1-12 /etc/machine-id) and socketGid: 0 (clobbering a Cloud-chosen host identity and the socket group a non-root container needs), set enabled:false, remove any existing override, and systemctl disable --now rome-hostd.service. That silently changes the execution target and revokes host execution — the opposite of docs/architecture/host-execution.md's "The helper never silently changes the execution target" and "The installer keeps state ... across helper and Rome upgrades".
30-rome.sh already gets this right for the compose file ("A tenant's Rome Cloud bundle is never overwritten"). Apply the same rule here: only manage the helper when this layer installed it (e.g. a /etc/rome-host/hostd-owned marker written when $root/rome-hostd is staged), and on first apply derive the marker from the existing state (enabled:true in a pre-existing config.json ⇒ create /etc/rome-host/wechat) rather than resetting it. At minimum, preserve hostId and socketGid from an existing config.json instead of regenerating them.
| run 'sudo rm -rf /etc/rome-host/provision /etc/rome-host/files && sudo mkdir -p /etc/rome-host && sudo tar -x -C /etc/rome-host --no-same-owner --no-same-permissions' | ||
| case "$wechat" in | ||
| on) run 'sudo touch /etc/rome-host/wechat' ;; | ||
| off) run 'sudo rm -f /etc/rome-host/wechat /opt/rome/docker-compose.override.yml' ;; |
There was a problem hiding this comment.
[P1] error-handling — apply.sh --no-wechat leaves the container running with WeChat and root mode
Deleting /opt/rome/docker-compose.override.yml here defeats the change detection that is supposed to recreate the container. 40-wechat.sh:63 only calls mark_changed docker-compose.override.yml when it finds the file still present, so after this pre-deletion the mark is never set and run.sh:68 skips docker compose up -d. The helper is stopped, but the running rome container keeps WECHAT_USER_ENABLED=true, ROME_HOST_EXECUTION_ENABLED=true and ROME_DOCKER_USER_MODE=root (every process in the container stays root, per docs/wechat-personal.md) until someone recreates it by hand — which contradicts the README's "--no-wechat turns both off". The override is documented as owned by 40-wechat.sh ("written on enable and removed on disable"), so apply.sh should only flip the marker:
| off) run 'sudo rm -f /etc/rome-host/wechat /opt/rome/docker-compose.override.yml' ;; | |
| off) run 'sudo rm -f /etc/rome-host/wechat' ;; |
| # developer's checkout had. Root owns it and nobody else writes it. | ||
| chown -R root:root "$root" | ||
| find "$root" -type d -exec chmod 755 {} + | ||
| find "$root" -type f -exec chmod 644 {} + |
There was a problem hiding this comment.
[P2] security — run.sh blanket-chmods every file under /etc/rome-host to 0644
/etc/rome-host is not exclusively this tree's directory — it is also the host helper's protected config directory, and on a Cloud-provisioned tenant it may hold other per-host state. A recursive chmod 644 over the whole directory widens anything mode-0600 that lives there to world-readable, and the comment above it ("The tree arrived with whatever owner and modes the build host or the developer's checkout had") only justifies normalizing the shipped tree. Scope the normalization to what was just extracted:
| find "$root" -type f -exec chmod 644 {} + | |
| find "$root/provision" "$root/files" "$root/pins.env" -type d -exec chmod 755 {} + | |
| find "$root/provision" "$root/files" "$root/pins.env" -type f -exec chmod 644 {} + |
(and narrow the chown -R on line 25 the same way).
| # here restricts inbound traffic; an internet-facing host needs a firewall or | ||
| # the Rome Cloud bundle. | ||
| name: rome | ||
| services: |
There was a problem hiding this comment.
[P2] architecture — A second compose definition of the rome service that will drift from the production one
This is now the third in-repo definition of the rome service (root docker-compose.yml, scripts/setup.sh, this one), and it has already diverged: no otel-collector (so the ClickStack credentials the seed writes have no consumer), no start_period on the healthcheck, no ROME_HOST_EXECUTION_SOCKET_DIR bind that the root file mounts unconditionally, and a different volume set (rome-memory:/app/memory, ssh-keys vs. the root file's host bind). Since both use name: rome, a host that later receives a bundle without rome-memory silently loses /app/memory into the app volume. Please either derive this file from the root compose (or vice versa) or add a comment stating explicitly which knobs are intentionally absent and why, so the next edit to the production compose has a reason to visit this one.
| off) run 'sudo rm -f /etc/rome-host/wechat /opt/rome/docker-compose.override.yml' ;; | ||
| esac | ||
| if [[ -n "$hostd" ]]; then | ||
| run 'sudo tee /etc/rome-host/rome-hostd >/dev/null' <"$hostd" |
There was a problem hiding this comment.
[P2] error-handling — apply.sh does not validate the --hostd binary the way build.sh does
build.sh:118-130 checks that the staged rome-hostd is an ELF for the target architecture before copying it in; apply.sh pipes whatever path it is given straight onto the host, where 40-wechat.sh installs it 0755 and run.sh restarts the service. Pushing an amd64 binary to an arm64 tenant (or a truncated file, since tee gives no integrity check) leaves the helper crash-looping. Factor the ELF check out of build.sh into a shared helper and call it here too, and consider verifying a sha256 on the far side after tee.
| # config.json) lives beside the tree and is kept. Modes are not the | ||
| # developer's: run.sh normalizes the tree before anything runs from it. | ||
| tar -C "$here" -c pins.env provision files | | ||
| run 'sudo rm -rf /etc/rome-host/provision /etc/rome-host/files && sudo mkdir -p /etc/rome-host && sudo tar -x -C /etc/rome-host --no-same-owner --no-same-permissions' |
There was a problem hiding this comment.
[P2] error-handling — Tree replacement on the host is not atomic
rm -rf /etc/rome-host/provision /etc/rome-host/files runs before the new tree lands, in the same command as the extraction. A dropped SSH connection or a truncated tar stream leaves the host with a partial or empty provision tree while /etc/rome-host/applied still advertises the previous version — and the next apply starts from that broken state. Extract into /etc/rome-host/.staging and swap with mv once the stream completes (rm -rf the old dirs only after the new ones are in place).
| - $pubkey | ||
| write_files: | ||
| # Dev loop: the guest pulls worktree builds from the host registry on the | ||
| # slirp gateway over plain HTTP, the same registry scripts/vm/vm.sh pushes to. |
There was a problem hiding this comment.
[P3] code-quality — Comment points at a script that does not exist in the repo
"the same registry scripts/vm/vm.sh pushes to" — there is no scripts/vm/ in this tree (the PR description itself calls the old local-VM script "an untracked script in a few worktrees"). A reference that is already dangling at birth will send the next reader looking for a file that was never committed. Either drop the path or describe the registry by what it is (docker push 127.0.0.1:5000/rome:dev on the host), as the README already does.
| Type=oneshot | ||
| # Loading a multi-GB archive on a slow disk outweighs the default 90s. | ||
| TimeoutStartSec=30min | ||
| ExecStart=/usr/bin/docker load -i /var/lib/rome/rome.tar |
There was a problem hiding this comment.
[P3] design — The preload accelerator depends on unstated seed ordering
Nothing orders docker compose up -d after this unit; dev/boot.sh's seed happens to systemctl start rome-load-image.service before it, but Rome Cloud's seed (out of scope for this PR) has no stated contract to do the same. If compose starts first, the tenant pulls the multi-GB image over the network while docker load is unpacking the same tag — the accelerator is lost and the two racing writes are at best wasted work. Worth either a Before= on whatever starts the stack, or an explicit line in the README stating the ordering requirement the Cloud seed must honor.
| rome: | ||
| image: ${ROME_DOCKER_IMAGE} | ||
| ports: | ||
| - "8080:8080" |
There was a problem hiding this comment.
[P3] security — Seed compose publishes 8080 on every interface with no host firewall
A published Docker port bypasses ufw/nftables rules a self-hosted operator might add later, so a box booted from this image with the seed compose has the dashboard on the public internet the moment it gets an address. The header comment does warn ("an internet-facing host needs a firewall or the Rome Cloud bundle"), but this layer otherwise owns the host's security posture (fail2ban, metadata block). Consider making the bind address a variable (${ROME_BIND_ADDR:-0.0.0.0}:8080:8080) so a seed can narrow it, and mention in "Known boundaries" that the port is not covered by a host firewall.
What this PR does
A Rome host today is provisioned three different ways: Rome Cloud runs a cloud-init script on stock Ubuntu at every tenant's first boot, the desktop ships a hand-built Alpine qcow2, and a local production-shaped VM is an untracked script in a few worktrees. Nothing pins the host layer, and a change to it has no path to an existing machine other than editing the cloud-init script and hoping.
This adds
infra/vm: one definition of the host layer (Docker at a pinned version, fail2ban, the metadata-endpoint block, the compose project, optionally the WeChat host helper) with three runners over the same idempotent steps.build.shturns a pinned Ubuntu cloud image into a bootable qcow2 offline withvirt-customize, with the Rome image preloaded by digest throughskopeo. No VM boots during the build.apply.shruns the same steps over SSH on a live host, so an existing tenant is brought up to the tree instead of re-provisioned.dev/boot.shboots a built image under KVM with a dev seed, so a local VM is the same image a tenant gets.Design & Invariants
apply.sh --wechat.pins.envand verified before use: base image serial and sha256, Docker apt versions, Rome image digest.HOST_LAYER_VERSIONis recorded on every host in/etc/rome-host/applied..env, Rome Cloud writes the instance token, Pantheon origin, and ClickStack credentials.Packer was the first candidate. It lost because the one step that needed a booted VM, pulling the Rome image, is replaced by
skopeointo adocker-archive, so a boot-and-SSH tool adds ceremony without value. NixOS as the guest OS was rejected as too large a change to the tenant fleet.Test plan
build.sh --arch amd64on devbox: 57s customize on a cached base, image inspected offline withvirt-cat/virt-lsdev/boot.shboots under KVM, SSH in 6s,docker loadof the preloaded image,Rome startedin the container log, dashboard answers 200 on the forwarded portapply.shagainst the freshly booted VM is a no-op in 2sbuild.sh --hostdinstalls the helper disabled with an empty hostId;dev/boot.sh --wechatenables it at first boot with a hostId from the machine, and a root job submitted from inside the container through the socket succeedsapply.shagainst a live VM with an edited compose file leaves the file alone and does not restartrome-hostd;apply.sh --no-wechatdisables and stops the helperdeployloopqemu-img convert -O rawproduces the artifact shape Vultr's snapshot-from-URL acceptspins.envNot in this PR
vultr.tsfromos_idto a snapshot and shrinking its cloud-init to the seed; needs the raw image published first.guestfs-tools,skopeo,cloud-utils,OVMF; kept out so the flake change reviews on its own.🤖 Generated with Claude Code