Conversation
scripts/vm/vm.sh boots the Ubuntu cloud image under QEMU/KVM and provisions it the way Rome Cloud provisions a hosted box. up --wechat installs the host helper built from the worktree and enables personal WeChat through a compose override. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 💬 COMMENT
This PR introduces a local, production-shaped VM harness (scripts/vm/vm.sh + cloud-init provisioning in scripts/vm/provision/rome-host.sh) that boots the Ubuntu 22.04 cloud image under QEMU/KVM, builds the Rome image from the worktree the way CI does, serves it through a shared host registry, and adds a --wechat flag that installs the host helper and writes a Compose override. It also extends the Linux devShell with qemu_kvm/cloud-utils, adds the pnpm vm script, and updates docs.
Overall the change is cohesive, self-contained, Linux-gated, and thoroughly documented. I verified the tricky parts: the worktree slug is DNS/Docker-tag safe so it is safe to interpolate into image tags and the helper config.json; the nested single-quoted heredocs inside the unquoted outer vm_ssh <<EOF expand $slug locally as intended while keeping remote-side content literal; and docker compose auto-merges docker-compose.override.yml, which is what makes WeChat survive deploy. No P0/P1 issues found — the remaining notes are hygiene and edge-case robustness.
Verdict: COMMENT — A well-structured, well-documented local dev tool with no blocking issues; only a few low/medium robustness and security-hygiene notes.
5 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P2 | security | scripts/vm/vm.sh |
Secrets are forwarded into the cloud-init seed and printed verbatim by print-user-data |
| P3 | error-handling | scripts/vm/provision/rome-host.sh |
.env is sourced with unquoted values, so a token with shell metacharacters breaks provisioning |
| P3 | error-handling | scripts/vm/vm.sh |
up --wechat guard only checks the current invocation, not the VM's provisioning state |
| P3 | design | scripts/vm/vm.sh |
rome-hostd.service ordering guarantees launch order, not socket-directory readiness before docker.service |
| P3 | error-handling | scripts/vm/vm.sh |
power_off removes the pidfile after SIGTERM without confirming the process exited |
| case "$rome_image" in | ||
| "$guest_registry"/*) echo "ROME_VM_INSECURE_REGISTRY=$guest_registry" ;; | ||
| esac | ||
| for var in PANTHEON_BASE_ORIGIN PANTHEON_DOMAIN ROME_INSTANCE_TOKEN \ |
There was a problem hiding this comment.
[P2] security — Secrets are forwarded into the cloud-init seed and printed verbatim by print-user-data
DOCKERHUB_TOKEN, CLICKHOUSE_PASSWORD, ROME_INSTANCE_TOKEN, and STATSIG_SERVER_SECRET_KEY are written into /etc/rome-vm/provision.env and embedded (base64) in the cloud-init user-data. The print-user-data command is documented as a way to "inspect the cloud-init seed without booting", so anyone who runs it and pastes output into a bug report or shares a terminal leaks live credentials in trivially-decoded base64. Consider redacting sensitive vars in print-user-data output (or printing a placeholder), and note in the README that the seed contains secrets. Local-dev scope keeps this from being critical, but the inspection command makes accidental exposure easy.
| step "docker pull ${ROME_DOCKER_IMAGE}" | ||
| set -a | ||
| # shellcheck source=/dev/null | ||
| source /opt/rome/.env |
There was a problem hiding this comment.
[P3] error-handling — .env is sourced with unquoted values, so a token with shell metacharacters breaks provisioning
.env is generated with unquoted assignments like DOCKERHUB_TOKEN=${DOCKERHUB_TOKEN:-} and then re-read via set -a; source /opt/rome/.env. A token/password containing spaces, $, backticks, #, or quotes will either corrupt the value or execute embedded shell during source, failing provisioning under set -e in a confusing way. Docker Hub tokens are usually safe, but ClickHouse passwords may not be. Prefer reading these values from the already-exported environment instead of round-tripping through a sourced file, or quote them when writing the file.
| esac | ||
| done | ||
| require_tools | ||
| if [ "$wechat" -eq 1 ] && [ "$provision" -eq 0 ]; then |
There was a problem hiding this comment.
[P3] error-handling — up --wechat guard only checks the current invocation, not the VM's provisioning state
The guard if [ "$wechat" -eq 1 ] && [ "$provision" -eq 0 ] only rejects --wechat --no-provision in the same command. When --wechat targets an already-running VM (the is_running early-return path calls install_wechat), provision defaults to 1 regardless of how the VM was originally booted. If that VM was first created with --no-provision, install_wechat's cd /opt/rome will fail with a cryptic remote error because /opt/rome never existed. Consider probing the guest (e.g. check /opt/rome/docker-compose.yml) before attempting the WeChat install, and emit a clear message.
| After=network.target | ||
| # The socket directory lives on /run. It must exist before Docker restores the | ||
| # container that mounts it. | ||
| Before=docker.service |
There was a problem hiding this comment.
[P3] design — rome-hostd.service ordering guarantees launch order, not socket-directory readiness before docker.service
The unit relies on Before=docker.service to ensure /run/rome-host exists before Docker restores the mounting container. With the default Type=simple, systemd considers the unit "started" as soon as the binary is exec'd, not when it has created the socket directory — so on a cold boot docker.service can start before rome-hostd has created /run/rome-host. Directory bind-mount propagation usually papers over this at runtime, but if you want the ordering to be real, use Type=notify (with sd_notify in the helper) or systemd socket activation for /run/rome-host/control.sock. Low severity given the observed behavior, but worth documenting the assumption.
| local deadline=$(($(date +%s) + 60)) | ||
| while kill -0 "$pid" 2>/dev/null; do | ||
| if [ "$(date +%s)" -ge "$deadline" ]; then | ||
| log "guest did not power off in 60s, terminating qemu" |
There was a problem hiding this comment.
[P3] error-handling — power_off removes the pidfile after SIGTERM without confirming the process exited
On the 60s timeout path, power_off sends kill "$pid" (SIGTERM), sleeps 1s, breaks, then unconditionally rm -f "$pidfile". If QEMU ignores or is slow to handle SIGTERM, the pidfile is deleted while the process is still alive, leaving an orphaned VM that is_running/qemu_pid can no longer track (subsequent up may then collide on ports). Consider escalating to kill -9 after a short grace check and only removing the pidfile once kill -0 confirms the process is gone.
Jessie-QingYu
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
The local KVM/cloud-init development loop is directionally appropriate, but the primary WeChat path cannot build from a clean checkout, provisioning mishandles ordinary environment values, and up can report success when Rome never becomes ready.
Verdict: REQUEST_CHANGES — The advertised VM and WeChat flows contain three blocking failures in a clean, production-like setup.
3 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | error-handling | scripts/vm/vm.sh |
Create the host-helper output directory before building |
| P1 | security | scripts/vm/vm.sh |
Escape values written into the sourced provisioning file |
| P1 | error-handling | scripts/vm/vm.sh |
Propagate dashboard readiness failures from up |
| install_wechat() { | ||
| command -v go >/dev/null 2>&1 || die "go is missing on PATH — run inside \`nix develop\`" | ||
| log "building the host helper ..." | ||
| (cd packages/host-helper && CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o dist/rome-hostd ./cmd/rome-hostd) |
There was a problem hiding this comment.
[P1] error-handling — Create the host-helper output directory before building
packages/host-helper/dist is ignored and absent in a clean checkout. go build -o dist/rome-hostd does not create its parent directory, so pnpm vm up --wechat fails before upload unless a stale local dist/ exists. Create the directory first or build to a temporary path.
| CLICKHOUSE_ENDPOINT CLICKHOUSE_USERNAME CLICKHOUSE_PASSWORD CLICKHOUSE_DATABASE \ | ||
| PANTHEON_SSH_PUBLIC_KEY; do | ||
| if [ -n "${!var:-}" ]; then | ||
| echo "$var=${!var}" |
There was a problem hiding this comment.
[P1] security — Escape values written into the sourced provisioning file
This file is later executed with source as root. A normal PANTHEON_SSH_PUBLIC_KEY contains spaces, so the raw assignment makes the shell try to execute the key body and aborts provisioning; shell metacharacters can also alter the sourced program. Render every value as a shell-escaped assignment, such as Bash printf '%s=%q\n' "$var" "${!var}".
| wait_for_cloud_init | ||
| if [ "$provision" -eq 1 ]; then | ||
| log "cloud-init done; waiting for Rome to serve ..." | ||
| wait_for_rome || true |
There was a problem hiding this comment.
[P1] error-handling — Propagate dashboard readiness failures from up
wait_for_rome fails only after its 600-second timeout, but || true discards that failure and up prints a successful VM/timing summary. This contradicts the documented readiness contract and masks failed first-boot deployments. Return the failure or explicitly handle only a known noncritical case.
What this PR does
Host-level Rome work had no local loop.
pnpm dev:allruns Rome in a container, but it has no VM underneath, so there was nowhere to exercise provisioning, systemd units, the host helper, or a feature that depends on them. Personal WeChat is one such feature: it needs an x86-64 Linux VM with the host helper installed, so it cannot run on a Mac or in the dev container. The VM script existed only as untracked files in old worktrees, and each WeChat test VM got its host helper by hand.pnpm vmboots a production-shaped VM on a Linux host with KVM.pnpm vm up --wechatalso installs the host helper and enables personal WeChat.Design & Invariants
scripts/vm/provision/rome-host.sh. That script follows Rome Cloud's hosted cloud-init script step for step, and the compose bundle comes from a rome-cloud checkout. A host-setup change is developed here and then ported to Pantheon.rome-vm-registryon the host loopback, so a rebuild moves only changed layers.~/.rome-vm/<worktree slug>, and ports are derived from the slug, so sibling worktrees get separate VMs.--wechatcovers the runtime requirements indocs/wechat-personal.mdand nothing more. It installs the host helper as a systemd unit ordered before Docker, because its socket directory lives on/runand must exist before Docker restores the container. It also writes a compose override that mounts the socket, sets the WeChat and host execution flags, runs the backend as root, and setsshm_size: 1gb. The override lives on the VM, sodeploykeeps it. The flag works on a new or a running VM.qemu_kvmandcloud-utils. macOS is unchanged, since it has no/dev/kvm.The alternative for WeChat was to bake the helper into cloud-init. Rome Cloud installs the helper from a released artifact, so a local cloud-init copy would drift from it. Installing after provisioning also lets one flag update a running VM with a helper rebuilt from the worktree.
Test plan
scripts/lint-shell.sh(shellcheck and shfmt)node scripts/check-prose.mjsvm.sh up --wechaton a fresh VM on a 32-core KVM host: Rome serves,rome-hostdis enabled and active, the container has the WeChat, host execution, and root-mode settings with 1 GB of shared memory, andGET /v1/capabilitiesover the mounted socket answers from inside the container.vm.sh up --wechatagain on the running VM: replaces the running helper binary and leaves Rome serving.vm.sh up, with the same helper and override applied by hand before--wechatexisted: personal WeChat QR login, key capture, and message history (with fix(wechat): wait for unfinished shards and route remembered sign-ins to the desktop #404 applied).--wechatitself.vm.sh deployon a--wechatVM keeps WeChat enabled.🤖 Generated with Claude Code