Repository navigation
VM_CONTACT: upgrade path for VMs that already have vm-mcp - #85
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 030bab3d81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| B=$(sudo -n cat /root/vm-mcp-upgrade.latest) | ||
| eval "$(sudo -n cat "$B/target.env")" | ||
| sudo -n test -d "$TARGET_ROOT" | ||
| TOKEN_FILE=${TOKEN_FILE:-/root/vm-mcp.token} |
There was a problem hiding this comment.
When an existing installation reuses /etc/credstore/vm-mcp-token as instructed, U4 ignores VM_MCP_AUTH_TOKEN_FILE: it initializes the separate TOKEN_FILE variable to /root/vm-mcp.token and then overwrites VM_MCP_AUTH_TOKEN_FILE with that value. If step 1 was skipped, the installer exits 3 only after both services have been stopped; even if the operator discovers and exports TOKEN_FILE instead, check e still reads /root/vm-mcp.token. Carry one selected token path through both installation and verification.
Useful? React with 👍 / 👎.
| # start what was running before | ||
| if [ "$(sudo -n cat "$B/broker-was" 2>/dev/null)" = active ]; then sudo -n systemctl start vm-mcp-admin.service; fi | ||
| sudo -n systemctl start vm-mcp.service |
There was a problem hiding this comment.
When either service was inactive or disabled before the upgrade, U6 does not restore that state: the pinned installer enables the main unit and conditionally the broker, U1 records only whether the broker was active, and this block always starts vm-mcp.service without restoring either unit's enablement. The local systemctl --help confirms that start activates a unit and enable changes its unit-file state, so rollback can activate and persist a legacy, potentially unauthenticated endpoint that was deliberately off; snapshot and restore both active and enabled states for both units.
Useful? React with 👍 / 👎.
| port = env.get("VM_MCP_PORT", "8765") | ||
| with open(f"{B}/target.env", "w") as f: | ||
| os.fchmod(f.fileno(), 0o600) | ||
| f.write(f"TARGET_ROOT={root}\nTARGET_PROFILE={prof}\nTARGET_PORT={port}\n") |
There was a problem hiding this comment.
For an existing valid VM_MCP_ROOT containing whitespace or shell metacharacters, this emits an unquoted shell assignment and U4 later evaluates it with eval. A root such as /srv/my workspace therefore tries to execute workspace and aborts, while command substitutions embedded in a path are executed; the pinned installer itself permits every absolute path without newlines. Serialize these values with shell-safe quoting or consume the file without eval.
Useful? React with 👍 / 👎.
| | b | as written | broker unit `inactive`, `/run/vm-mcp/admin.sock` absent; all other lines as written | | ||
| | c, d, e | as written | as written (bearer auth is required in every profile) | | ||
| | f | as written | the call in f must be refused by policy (`ok: false`) and no broker receipt may appear | |
There was a problem hiding this comment.
When the existing installation uses a non-default VM_MCP_PORT, U4 correctly preserves it as TARGET_PORT, but running check b and checks e/f “as written” still polls and curls port 8765. The upgraded service can therefore be healthy on its preserved port while the required acceptance sequence waits, fails, and probes an unrelated listener; parameterize those checks with the preserved target port.
Useful? React with 👍 / 👎.
| if [ -f '$B/opt-vm-mcp.tar' ]; then rm -rf /opt/vm-mcp.rollback-old; mv /opt/vm-mcp /opt/vm-mcp.rollback-old 2>/dev/null || true; tar -C /opt -xpf '$B/opt-vm-mcp.tar'; fi | ||
| [ -f '$B/vm-mcp.env' ] && install -o root -g root -m 0600 '$B/vm-mcp.env' /etc/vm-mcp.env |
There was a problem hiding this comment.
The upgrade path is explicitly selected when either /opt/vm-mcp or /etc/vm-mcp.env exists, but U6 only restores these paths when their corresponding backup exists and never removes a newly created counterpart. Thus, for a partial pre-upgrade installation, rollback can leave the new /opt/vm-mcp tree or generated environment file behind instead of returning to the inventoried state; record absence and delete the artifact on rollback when it was originally absent.
Useful? React with 👍 / 👎.
| cp -a /etc/systemd/system/vm-mcp.service.d '$B/vm-mcp.service.d' 2>/dev/null || true | ||
| cp -a /etc/credstore/vm-mcp-token '$B/credstore-token' 2>/dev/null || true | ||
| cp -a /etc/sysctl.d/99-zz-vm-mcp-ptrace.conf '$B/' 2>/dev/null || true | ||
| tar -C /opt -cpf '$B/opt-vm-mcp.tar' vm-mcp 2>/dev/null || true |
There was a problem hiding this comment.
If tar fails after creating a partial archive—for example because the filesystem fills—|| true lets U1 report success and U6 later treats the mere existence of that archive as a valid backup. U6 then moves the working /opt/vm-mcp tree aside before extracting the truncated archive, leaving rollback and the service broken; ignore only a confirmed absent source and otherwise require the archive operation and an integrity check to succeed.
Useful? React with 👍 / 👎.
| | b | as written | broker unit `inactive`, `/run/vm-mcp/admin.sock` absent; all other lines as written | | ||
| | c, d, e | as written | as written (bearer auth is required in every profile) | | ||
| | f | as written | the call in f must be refused by policy (`ok: false`) and no broker receipt may appear | |
There was a problem hiding this comment.
For the explicitly supported workspace and read-only upgrades, the table changes the prose expectation but not the commands: check b still expects the broker to be active and its socket present, while check f still expects a successful broker receipt. Correctly disabled broker behavior therefore prints FAIL, contradicting the runbook's statement that check blocks identify success with PASS and making the required a–f acceptance sequence unusable for these preserved profiles; provide profile-aware expected values or separate executable blocks.
Useful? React with 👍 / 👎.
| Restores the env, units, drop-ins, sysctl file and code exactly as backed up in U1. | ||
| **This returns the VM to its pre-upgrade code, so endpoints that were not | ||
| token-protected before are unprotected again.** Prefer fixing forward; use this only | ||
| to recover service. For a profile-only rollback, use the `sed` rollback below instead. |
There was a problem hiding this comment.
When the operator uses the explicitly sanctioned target.env edit to change TARGET_ROOT, the installer runs usermod --home for vmmcp, but U6 restores only files and never restores the passwd entry recorded during inventory. Rolling back after that upgrade leaves the old service running with the new home and can redirect its HOME-dependent state away from the restored workspace; save the original home in machine-readable form and restore it before restarting the old unit.
Useful? React with 👍 / 👎.
| port = env.get("VM_MCP_PORT", "8765") | ||
| with open(f"{B}/target.env", "w") as f: | ||
| os.fchmod(f.fileno(), 0o600) | ||
| f.write(f"TARGET_ROOT={root}\nTARGET_PROFILE={prof}\nTARGET_PORT={port}\n") |
There was a problem hiding this comment.
When an existing installation has a non-default VM_MCP_ADMIN_SOCKET, U2 omits it from target.env and U4 passes no corresponding variable, so the installer rewrites the environment with /run/vm-mcp/admin.sock. Earlier or locally customized units and drop-ins can therefore stop agreeing with the main service, breaking admin_exec despite the stated promise to preserve privileged components; inventory and pass the existing socket alongside the root, profile, and port.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57e0ca6600
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| echo | ||
| SHA=$(printf %s 'id -u' | sha256sum | cut -d' ' -f1) | ||
| ok "broker receipt for id -u since $START" 1 "$(sudo -n journalctl -u vm-mcp-admin.service --since "$START" --no-pager | grep -F '"command_sha256":"'"$SHA"'"' | grep -F '"mode":"admin"' | grep -F '"run_as":"root"' | grep -cF '"exit_code":0' | sed 's/^[1-9][0-9]*$/1/' || true)" | ||
| ok "broker receipt for id -u since $START" 1 "$(sudo -n journalctl -u vm-mcp-admin.service --since "$START" --no-pager | grep -F '"command_sha256":"'"$SHA"'"' | grep -cF '"exit_code":0' | sed 's/^[1-9][0-9]*$/1/' || true)" |
There was a problem hiding this comment.
When a concurrent or same-second user_exec runs id -u, this pipeline accepts its receipt because the broker records the same command digest and exit code for both user and admin modes, while the preceding curl response is never asserted. Step f can therefore report every check as PASS even if the selected privileged call failed or never ran as root; retain the mode/run_as predicates or correlate the returned request ID with the receipt.
AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.
| # printed inventory (no secrets) | ||
| sudo -n sh -c "{ | ||
| echo '--- /etc/vm-mcp.env (token values blanked)' | ||
| sed -E 's/^([A-Z_]*TOKEN[A-Z_]*)=.*/\1=<blanked>/' /etc/vm-mcp.env 2>/dev/null || echo 'absent' |
There was a problem hiding this comment.
When an existing environment file contains an indented assignment such as VM_MCP_AUTH_TOKEN=..., this expression does not match because it requires the variable name at column zero, even though U2 explicitly parses assignments with leading whitespace. U1 then prints the live bearer credential into the SSH terminal and any captured run log, contrary to the runbook's secret-handling guarantees; allow leading whitespace when identifying token keys before emitting the inventory.
Useful? React with 👍 / 👎.
| sudo -n env -u VM_MCP_AUTH_TOKEN -u VM_MCP_AUTH_TOKEN_STDIN \ | ||
| VM_MCP_PROFILE="$TARGET_PROFILE" VM_MCP_ROOT="$TARGET_ROOT" VM_MCP_PORT="$TARGET_PORT" \ | ||
| VM_MCP_AUTH_TOKEN_FILE="$TOKEN_FILE" \ | ||
| bash "$ROOT_SRC/skill-lib/vm-mcp/install.sh" || rc=$? |
There was a problem hiding this comment.
When the interactive shell already has a nonzero rc variable, the subshell inherits it and a successful installer invocation leaves it unchanged because this assignment runs only on failure. The following status check then reports the stale value and exits even though the upgrade and service restart succeeded, potentially prompting an unnecessary rollback; initialize rc=0 before the invocation or assign the success status explicitly.
Useful? React with 👍 / 👎.
…-preservation Repair VM upgrade preservation and verification after #85
1 participant
Adds an upgrade section (U1–U6) to backend/deploy/VM_CONTACT.md for VMs with an existing vm-mcp install:
Goal: preserve the existing profile and privileged components by default and token-protect all local endpoints. Docs only; installer follow-ups go to skill-lib separately. Not for merge without Erin.