Repository navigation
Operator deploy CLI with health-checked rollback, one deploy routine - #1386
Conversation
…ed rollback sandboxed-sh deploy --service <unit> runs the same run_deploy as POST /api/system/deploy (refactored out of the SSE stream), applies the same refusals, waits for the guarded restart and reports its health check. The restart script now requires /api/health after start and restores the previous binaries otherwise, for both entry points. The API grant for missions is unchanged.
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: 916a1649b9
ℹ️ 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".
| repo_path, | ||
| git_ref: args.git_ref.clone(), | ||
| skip_build: args.skip_build, | ||
| service_name: args.service.clone(), |
There was a problem hiding this comment.
When invoking sandboxed-sh-dev deploy --service orb-core-dev.service, the service-equivalence guard accepts the alias, but passing that alias into DeployPlan changes the install destinations: service_binary_path_for_unit selects /usr/local/bin/orb-core-dev, and service_companion_binary_path only recognizes the sandboxed-sh- prefix, so it selects the unsuffixed production companions. With the checked-in deploy/systemd/orb-core-dev.service, whose ExecStart is /usr/local/bin/sandboxed-sh-dev, this both leaves the running backend binary unchanged and overwrites production MCP/palomactl binaries. Preserve the actual binary service identity for installation rather than using the user-supplied alias. This violates the dev/prod isolation requirement in AGENTS.md.
AGENTS.md reference: AGENTS.md:L364-L367
Useful? React with 👍 / 👎.
| std::env::set_var(key, value); | ||
| } | ||
| } | ||
| let config = match crate::config::Config::from_env() { |
There was a problem hiding this comment.
If WORKING_DIR is unset, copying the service environment does not reproduce its configuration: Config::from_env defaults debug builds to the current directory, but this CLI retains the SSH caller's directory instead of the service's working directory. The checked-in core units use /var/lib/sandboxed-sh or /var/lib/sandboxed-sh-dev, while an SSH invocation can start in /root. Consequently, count_live_writers_on_disk scans the wrong mission directory and returns zero, allowing a non-forced deploy to interrupt running missions; settings-based repository selection also reads the wrong state directory. Resolve the live process's cwd or the unit's configured working directory before constructing the configuration.
Useful? React with 👍 / 👎.
… environment Addresses security review: no set_var of the service environment, no /proc/<pid>/environ; PORT/HOST/WORKING_DIR/repo path are read from the unit's EnvironmentFile/Environment and passed explicitly. Test asserts LD_PRELOAD, GIT_DIR, BASH_ENV, PATH, CARGO_*, RUSTC_* are never adopted.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a82fbef81d
ℹ️ 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".
| if let Some(inline) = systemctl_show(service, "Environment").await { | ||
| env.extend(parse_environment_file( | ||
| &inline.split_whitespace().collect::<Vec<_>>().join("\n"), | ||
| )); |
There was a problem hiding this comment.
When a unit defines the same allowlisted key in both Environment= and EnvironmentFile=, this merge selects the inline value because service_deploy_env searches the pairs in reverse. That is opposite to systemd: its documentation states, “Settings from these files override settings made with Environment=.” For example, an inline PORT=3000 overridden by PORT=3100 in the environment file makes the CLI guard and health-check port 3000 while the restarted service actually listens on 3100, potentially rolling back a healthy deployment. Collect inline assignments first, then append the environment files in their configured order. This also undermines the documented requirement that both deploy entry points health-check the restarted service.
AGENTS.md reference: AGENTS.md:L420-L421
Useful? React with 👍 / 👎.
What
sandboxed-sh deploy --service <unit> [--ref] [--repo] [--skip-build] [--force]: operator deploy run as root on the host. Standard prod deploy:ssh agent-core '/usr/local/bin/sandboxed-sh-prod deploy --service sandboxed-sh-prod.service --ref origin/master'stream_deploy's body is nowrun_deploy(plan, tx)insrc/api/system.rs, reporting progress over a channel. The API wraps it in SSE, and the CLI prints the lines. There is one deploy implementation and no shell script.evaluate_deploy_request: the binary must serve--service, plus the live-writer and debounce checks (--forcebypasses the last two). It adopts the service's environment (/proc/<MainPID>/environ, or the unit's Environment/EnvironmentFile when it is down) for port, working dir and repo path. Live writers are counted from the on-disk SQLite mission stores using the samecount_live_writershelper as the API./api/healthforDEPLOY_HEALTH_TIMEOUT_SECS(default 120). If the check fails, it restores the previous binaries (backup copy, symlink retarget, or removal of a first install) and restarts again. The outcome is written to/var/lib/sandboxed-sh/last_deploy_result. The CLI waits withsystemd-run --waitand exits non-zero on rollback. The API path gets the same check and rollback. Self-update keeps the plain guarded restart.Authorization
The API path is unchanged: missions still need an active
service_deploygrant, andorb_deploy_gate_stepstill applies. The CLI requires root shell access on the host, which is already full trust.Principles traded
curlis missing on the host, the health check reportsuncheckedinstead of rolling back. A missing tool should not undo a good deploy.Tests
cargo test --lib api::system::: 61 passed. The new tests cover argument parsing, the refusals the CLI applies, the environment parsers, the health URL, the restart-outcome decision, the rollback shell commands and the health script order.cargo clippy -D clippy::allis clean on the touched code.Not deployed. The operator will run the command above.