ci: move trusted sandbox runtime to dedicated prefix - #331
Conversation
📝 WalkthroughWalkthroughThe CI ChangesCI sandbox runtime
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merge Protections🔴 1 of 2 protections blocking · waiting on 🙋 you
🔴 🚦 Auto-queueWaiting for
This rule is failing.When all merge protections are satisfied and these conditions match, this pull request will be queued automatically.
Show 1 satisfied protection🟢 mergeable shapeThe pull request is ready to be considered for merging.
|
Revert the global /usr/local ancestor mutation from #313 and install SRT plus every discovered support executable under a root-owned /opt/looper-srt tree.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 76: Update the global sandbox-runtime installation command in the
workflow to preserve the setup-node toolchain: invoke the resolved npm binary
from command -v npm and pass the current PATH explicitly through sudo env, while
retaining the existing install prefix and package version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5203a025-437a-42ef-809e-a8c397506919
📒 Files selected for processing (1)
.github/workflows/ci.yml
| sudo chmod 0755 /usr/local /usr/local/lib | ||
| sudo setfacl -b /usr/local /usr/local/lib | ||
| sudo install -d -o root -g root -m 0555 /opt/looper-srt/runtime /opt/looper-srt/bin | ||
| sudo npm install --global --prefix /opt/looper-srt/runtime @anthropic-ai/sandbox-runtime@0.0.67 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== workflow excerpt ==\n'
nl -ba .github/workflows/ci.yml | sed -n '30,90p'
printf '\n== sudo availability / behavior ==\n'
command -v sudo || true
sudo -V 2>/dev/null | sed -n '1,80p' || true
printf '\n== node/npm paths in current shell ==\n'
command -v node || true
command -v npm || true
node -v 2>/dev/null || true
npm -v 2>/dev/null || trueRepository: MumuTW/looper
Length of output: 215
🌐 Web query:
sudo default secure_path PATH npm setup-node ubuntu-latest GitHub Actions
💡 Result:
When using GitHub Actions ubuntu-latest runners, issues involving sudo, npm, and secure_path typically occur because sudo restricts the PATH environment variable for security reasons, often excluding directories where Node.js or npm binaries are installed [1][2][3]. ### Understanding the Conflict On Ubuntu systems, the /etc/sudoers file defines a secure_path [1][3]. When you run a command with sudo, the shell resets the PATH environment variable to only include these predefined, trusted directories [1][3]. If the Node.js binary (installed by actions/setup-node or pre-installed) resides outside of this secure_path, sudo will fail to find it, resulting in a command not found error [2][3][4]. ### Recommended Solutions 1. Use Absolute Paths (Best Practice) If you must run a command with sudo, explicitly provide the absolute path to the binary [2]. You can resolve this path dynamically in your workflow step [2]: yaml - name: Run command with sudo run: | node_bin=$(command -v node) sudo "$node_bin" your-script.js 2. Configure npm for User-Space Operations (Preferred) In GitHub Actions, you should generally avoid sudo for npm operations entirely [5]. If you need to install packages globally or update npm, configure npm to use a directory in your home folder instead of trying to write to system directories [6]: yaml - name: Setup npm prefix run: | mkdir -p ~/.npm-global npm config set prefix '~/.npm-global' echo "$HOME/.npm-global/bin" >> $GITHUB_PATH 3. Explicitly Specify Node Version Always use actions/setup-node to install a specific Node.js version [7][8]. This ensures that your workflow does not rely on the pre-installed system Node.js, which is often the source of permission conflicts [6]. ### Why setup-node Helps The actions/setup-node action installs Node.js versions into a tool cache directory [4]. When you run your workflow without sudo, the action correctly adds this directory to your PATH [9][8]. Problems only arise when you introduce sudo, as it intentionally strips these custom additions from the environment unless you explicitly account for them [2][4]. Avoid modifying /etc/sudoers or secure_path directly, as this is unnecessary and brittle in ephemeral runner environments [1].
Citations:
- 1: Sudo improperly configured in ubuntu-latest actions/runner#2458
- 2: fix: use absolute node path in awf wrapper to survive sudo PATH stripping github/gh-aw#25341
- 3: npm doesn't work with sudo nodejs/snap#44
- 4: fix: correctly recover runner tool on PATH (after sudo w/ secure_path). remove incorrect reading from GITHUB_PATH github/gh-aw-firewall#5144
- 5: [Ubuntu] Fail to install Node.js package globally for default Node.js actions/runner-images#3727
- 6: Cannot update NPM on Linux runners unless node_version is explicitly set actions/setup-node#1409
- 7: https://github.com/actions/setup-node
- 8: https://github.com/actions/setup-node/blob/v6/README.md
- 9: https://github.com/marketplace/actions/setup-node-js-environment
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, shutil, subprocess, json, sys
print("PATH=", os.environ.get("PATH"))
for exe in ["sudo", "node", "npm"]:
print(exe, "=>", shutil.which(exe))
PYRepository: MumuTW/looper
Length of output: 403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk 'NR>=30 && NR<=90 { printf "%5d %s\n", NR, $0 }' .github/workflows/ci.ymlRepository: MumuTW/looper
Length of output: 2871
Keep the setup-node npm toolchain under sudo. sudo npm can drop the toolcache PATH added by actions/setup-node, so this can invoke the system npm/node pair instead of Node 22. Use the resolved npm binary with an explicit PATH, e.g. sudo env "PATH=$PATH" "$(command -v npm)" ....
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 76, Update the global sandbox-runtime
installation command in the workflow to preserve the setup-node toolchain:
invoke the resolved npm binary from command -v npm and pass the current PATH
explicitly through sudo env, while retaining the existing install prefix and
package version.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 112c7e6ccc
ℹ️ 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".
| sudo install -o root -g root -m 0555 "$(command -v node)" /opt/looper-srt/bin/node | ||
| sudo install -o root -g root -m 0555 "$(command -v rg)" /opt/looper-srt/bin/rg | ||
| sudo install -o root -g root -m 0555 "$(command -v bwrap)" /opt/looper-srt/bin/bwrap | ||
| sudo install -o root -g root -m 0555 "$(command -v socat)" /opt/looper-srt/bin/socat |
There was a problem hiding this comment.
On the hosted image this change targets, /usr is writable by the runner, but copying these dynamically linked executables into /opt only relocates their main files: Node, ripgrep, bubblewrap, and socat still load their ELF interpreter and shared libraries from /lib//usr/lib, and the npm srt JavaScript launcher runs through /usr/bin/env, all before the sandbox exists. Consequently requireTrustedPath accepts the copied paths while the runner can still replace an ancestor of code involved in starting the sandbox, so LOOPER_REQUIRE_TRUSTED_SRT=1 no longer exercises the claimed trusted-runtime invariant. Use a runner with trusted system paths or place the complete interpreter/dependency chain under the protected prefix.
Useful? React with 👍 / 👎.
| sudo chown root:root /usr/local /usr/local/lib | ||
| sudo chmod 0755 /usr/local /usr/local/lib | ||
| sudo setfacl -b /usr/local /usr/local/lib | ||
| sudo install -d -o root -g root -m 0555 /opt/looper-srt/runtime /opt/looper-srt/bin |
There was a problem hiding this comment.
install -d protects only /opt/looper-srt; requireTrustedPath also walks /opt, and this PR\u2019s verify run proves /opt is runner-writable (untrusted srt installation: /opt is writable by the daemon user). Thus every support binary is staged successfully but the required trust contract still rejects SRT, so verify remains red. Place the dedicated tree directly below a non-writable ancestor (for example /looper-srt, assuming / is checked) or establish a trusted ancestor without reverting to a global writable path mutation; then re-run the required job.
|
Closing because the hosted invariant disproved this redesign. Verify still fails because /opt itself is writable by the daemon user. More fundamentally, copying the ELF entrypoints under /opt does not move their dynamic linker/shared libraries or the srt launcher interpreter (/usr/bin/env), so the current path probe could accept a prefix while executing replaceable code from writable /usr. The npm-under-sudo toolchain is also not pinned. Do not stack another path/permission patch. Redesign around a root-owned container/runner trust root that includes the complete interpreter/dependency chain, or revise the runtime trust authority explicitly; retain #313 global mutations reverted in that focused design. |
Outcome
Restores the required
verifyjob after main began rejecting the hosted runner because/usris writable. This reverts the global/usr/localpermission/ACL mutation from #313 and installs SRT plus the exact support executables under a dedicated root-owned/opt/looper-srttree.Authority
The authority is the executable paths resolved from the CI package installation before the private prefix is added to
GITHUB_PATH; it is not agent output.Trade-off
The dedicated prefix adds one CI-only install location and copies four binaries, so version/path drift must remain aligned with the package-manager install step. It removes the broader failure modes from changing ownership, modes, and ACLs on shared
/usr/localancestors. A plain deletion of #313 is insufficient because current hosted runners make/usrwritable and the trusted-path invariant would still reject tools installed below it.Validation
git diff --checkverifyjob as the contract test:LOOPER_REQUIRE_TRUSTED_SRT=1 go test ./...must resolve SRT, node, rg, bwrap, and socat entirely under the dedicated trusted tree.Replaces #313/#323.
Summary by CodeRabbit