Skip to content

security: use explicit outer-container isolation for Codex - #39

Merged
eXPerience83 merged 56 commits into
mainfrom
security/no-bubblewrap-outer-isolation
Jul 30, 2026
Merged

security: use explicit outer-container isolation for Codex#39
eXPerience83 merged 56 commits into
mainfrom
security/no-bubblewrap-outer-isolation

Conversation

@eXPerience83

@eXPerience83 eXPerience83 commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Summary

Make the supported TrueNAS isolation model explicit and deterministic:

  • remove the system bubblewrap package and executable from the default image;
  • start Codex through one shared run-codex launcher with --sandbox danger-full-access --ask-for-approval untrusted;
  • route menu start, resume and START_MODE=codex through that same launcher;
  • avoid Codex's unsupported packaged Bubblewrap fallback and its warning instead of depending on sandbox failure and retry;
  • report the fixed launch policy and outer-container boundary in diagnostics;
  • fail build/runtime validation if the system Bubblewrap executable is unexpectedly present;
  • document that approvals are a control, not a sandbox or a substitute for narrow mounts.

Security constraints preserved

  • no privileged mode;
  • no SYS_ADMIN;
  • no unconfined AppArmor or seccomp profile;
  • no Docker socket;
  • no host security changes;
  • no-new-privileges:true remains enabled;
  • only explicitly mounted paths are reachable from the agent service.

danger-full-access refers only to the disabled Codex inner sandbox. It does not add Docker privileges or host access. The outer container remains the supported TrueNAS isolation boundary.

Automated validation

  • repository configuration and Bash syntax validation;
  • base and final AMD64 image builds;
  • absence of the system Bubblewrap executable;
  • shared launcher argument and policy assertions;
  • rejection of sandbox, approval, bypass and configuration overrides;
  • direct execution of the pinned /usr/local/bin/codex;
  • real Codex compatibility through run-codex --version and run-codex resume --help;
  • build-only policy tests supplied through a read-only BuildKit bind mount rather than persisted image layers;
  • non-destructive launcher mocks that never replace /usr/local/bin/codex;
  • embedded image identity checks;
  • direct-session status and credential hardening;
  • ttyd/tmux runtime smoke tests;
  • diagnostics assertions for the explicit policy;
  • Trivy reports and fixable-critical gate.

Manual TrueNAS validation completed on the first candidate

  • exact candidate revision and image metadata verified;
  • Codex and GitHub authentication persisted;
  • workspace and credential paths remained writable;
  • auth.json retained mode 600;
  • the system Bubblewrap executable was absent;
  • removing only the package still triggered Codex's packaged-Bubblewrap warning, so relying on fallback failure was rejected;
  • manual launch with codex --sandbox danger-full-access --ask-for-approval untrusted produced no Bubblewrap warning;
  • pwd and git status --short ran without unnecessary approval;
  • a shell command that created, read and removed a file requested explicit one-time approval and completed successfully.

Audit status

The complete 18-file PR diff, official Codex 0.146.0 CLI option definitions, all supported entry paths, test cleanup behavior, image-layer contents, documentation, related issues and the exact candidate-publication workflow were reviewed. The current PR merge result passed the full AMD64 build, runtime smoke tests and vulnerability gate. The exact head still requires the independent candidate workflow before TrueNAS validation. Earlier candidate images are superseded and must not be used for final validation.

Final validation still required

This PR remains a draft until a new exact candidate for the final reviewed head is tested on TrueNAS for:

  • menu option 1 with no Bubblewrap warning;
  • menu option 2 / resume with the same policy;
  • write-command approval;
  • diagnostics output;
  • browser reconnect and tmux persistence;
  • stop/start and recreation;
  • workspace and credential persistence.

Progresses #36.
Tracked by #31.

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@eXPerience83, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 37 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ea65f1d1-81b4-4380-ad15-652db0275cd8

📥 Commits

Reviewing files that changed from the base of the PR and between 56b1778 and 5d2af76.

📒 Files selected for processing (1)
  • scripts/smoke-test.sh

Summary by CodeRabbit

  • New Features
    • Added a unified Codex launcher used across start, resume, and direct paths, with policy reporting and diagnostics.
    • Improved terminal experience persistence and exposed build/runtime metadata in the menu/diagnostics.
    • Added Trivy reporting/gating for image promotion and ensured multi-arch runtime configuration is committed with validation.
  • Security
    • Untrusted commands now require explicit approval within the supported outer-container isolation boundary.
    • Removed Bubblewrap from the default image; hardened verification to prevent nested/unsupported sandboxing.
  • Documentation
    • Expanded TrueNAS isolation guidance and clarified approval prompts vs sandbox behavior.
  • Tests
    • Strengthened smoke/doctor verification and added dedicated launcher policy override tests.

Walkthrough

The PR removes Bubblewrap from the default image and standardizes Codex startup through run-codex, enforcing disabled inner sandboxing with untrusted-command approvals. Diagnostics, smoke tests, documentation, and release validation now describe the outer container as the isolation boundary.

Changes

Codex outer-isolation policy

Layer / File(s) Summary
Unified Codex launcher and policy checks
scripts/run-codex.sh, scripts/test-run-codex-policy.sh, images/codex/Dockerfile, scripts/codex-menu.sh, scripts/attach-remote-dev-tmux.sh
Adds a shared launcher, rejects policy overrides, routes supported entrypoints through it, and validates invocation behavior during builds and tests.
Bubblewrap removal and image validation
images/base/Dockerfile, scripts/base-verify.sh, scripts/validate-version-pins.sh
Removes Bubblewrap and rejects its installation or reference.
Runtime policy diagnostics and smoke tests
scripts/codex-doctor.sh, scripts/runtime-smoke-test.sh, scripts/smoke-test.sh
Validates launcher compatibility, policy output, Codex version consistency, and Bubblewrap absence.
Isolation contract and release documentation
CHANGELOG.md, PROJECT_STATUS.md, README*.md, docs/*.md
Documents the outer-container boundary, disabled inner sandbox, approval behavior, image contents, and release validation requirements.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Possibly related issues

Possibly related PRs

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant CodexMenu
  participant RunCodex
  participant CodexCLI
  User->>CodexMenu: choose start or resume
  CodexMenu->>RunCodex: invoke command
  RunCodex->>CodexCLI: pass fixed sandbox and approval policy
  CodexCLI-->>User: run commands with untrusted-command approval prompts
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately captures the main change: making Codex use explicit outer-container isolation.
Description check ✅ Passed The description covers the change, impact, validation, safety constraints, and follow-up, even if it doesn't match the template headings exactly.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch security/no-bubblewrap-outer-isolation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Owner Author

/publish-candidate 8c13e65

1 similar comment

Copy link
Copy Markdown
Owner Author

/publish-candidate 8c13e65

Copy link
Copy Markdown
Owner Author

Manual TrueNAS validation of candidate 8c13e654bf107da363c81eb57886d2da45300e2f:

  • Candidate image starts correctly and preserves Codex/GitHub authentication and 0600 permissions on auth.json.
  • Diagnostics correctly report Inner sandbox: unavailable / not installed and Isolation boundary: outer container.
  • Codex can create, read, delete and verify a workspace file through explicit approvals.
  • However, Codex 0.146.0 emits Codex could not find bubblewrap on PATH ... Codex will use the bundled bubblewrap in the meantime.
  • The bundled Bubblewrap path still cannot provide a usable nested sandbox on this TrueNAS host; shell commands continue through explicit out-of-sandbox approvals.

Therefore the runtime is functional, but the current PR does not yet provide the intended clean explicit outer-isolation mode. Keep the PR in draft while testing an explicit danger-full-access Codex sandbox policy combined with the stricter untrusted approval policy. Do not use on-request with danger-full-access, because upstream tests show that combination permits writes and network access without approval.

Copy link
Copy Markdown
Owner Author

Final audit request for current head 7a05401bf5158bd3688be29e71e21ea301cf0685.

The two prior findings are fixed and confirmed:

  • the build-only policy test is read-only bind-mounted and never persisted in an image layer;
  • policy and tmux tests use temporary launcher copies and never mutate /usr/local/bin/codex.

Additional audit hardening on this head:

  • no temporary set +e in the security-policy or runtime tests;
  • cleanup covers EXIT, INT and TERM;
  • the real pinned launcher is checked with run-codex --version;
  • the resume route is parsed through run-codex resume --help;
  • documentation distinguishes the absent system Bubblewrap executable from Codex's packaged fallback;
  • the complete 18-file diff, entry paths, related issues and candidate workflow were reviewed;
  • exact-head AMD64 build, runtime smoke tests, Trivy reports and the fixable-critical gate are green.

Please review the complete current head and report any remaining actionable issue before a new candidate is published.

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== HEAD and target commit =='
git rev-parse HEAD
git show -s --format='%H%n%s%n%P' 7a05401bf5158bd3688be29e71e21ea301cf0685
echo
echo '== Changed files vs first parent =='
git diff-tree --no-commit-id --name-status -r 7a05401bf5158bd3688be29e71e21ea301cf0685
echo
echo '== Aggregate PR diff stat =='
git diff --stat "$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"..HEAD 2>/dev/null || true
echo
echo '== Candidate file outlines =='
for f in \
  scripts/run-codex.sh \
  scripts/codex-menu.sh \
  scripts/attach-remote-dev-tmux.sh \
  scripts/codex-doctor.sh \
  scripts/test-run-codex-policy.sh \
  scripts/smoke-test.sh \
  scripts/runtime-smoke-test.sh \
  images/codex/Dockerfile \
  images/base/Dockerfile \
  scripts/base-verify.sh \
  scripts/validate-version-pins.sh; do
  if [ -f "$f" ]; then
    echo "--- $f"
    ast-grep outline "$f" --items all 2>/dev/null || true
  fi
done
echo
echo '== Launcher and test scripts =='
for f in scripts/run-codex.sh scripts/test-run-codex-policy.sh scripts/smoke-test.sh scripts/runtime-smoke-test.sh; do
  if [ -f "$f" ]; then
    echo "--- $f"
    cat -n "$f"
  fi
done
echo
echo '== Entry points, build, verification, and diagnostics =='
for f in scripts/codex-menu.sh scripts/attach-remote-dev-tmux.sh scripts/codex-doctor.sh images/codex/Dockerfile images/base/Dockerfile scripts/base-verify.sh scripts/validate-version-pins.sh; do
  if [ -f "$f" ]; then
    echo "--- $f"
    cat -n "$f"
  fi
done
echo
echo '== Repository-wide Codex invocation and launcher-routing evidence =='
rg -n -i -C 3 --glob '!*.md' --glob '!CHANGELOG.md' \
  '(?:^|[[:space:];|&()])codex(?:[[:space:];|&()]|$)|run-codex|START_MODE' . || true
echo
echo '== Bubblewrap and policy documentation claims =='
rg -n -i -C 2 \
  'bubblewrap|bwrap|danger-full-access|ask-for-approval|untrusted|approval.*sandbox|sandbox.*approval' \
  README.md README.es.md PROJECT_STATUS.md docs CHANGELOG.md 2>/dev/null || true
echo
echo '== Exact target commit diff =='
git show --format=fuller --find-renames --find-copies --stat --patch \
  7a05401bf5158bd3688be29e71e21ea301cf0685

Length of output: 100861


🏁 Script executed:

#!/bin/bash
set -euo pipefail

base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
echo "== Merge base: $base =="
echo
echo '== Complete changed-file patch (no unrelated history) =='
git diff --find-renames --find-copies --unified=35 "$base"..HEAD -- \
  CHANGELOG.md PROJECT_STATUS.md README.md README.es.md \
  docs/releases.md docs/security.md docs/tool-matrix.md \
  images/base/Dockerfile images/codex/Dockerfile \
  scripts/attach-remote-dev-tmux.sh scripts/base-verify.sh \
  scripts/codex-doctor.sh scripts/codex-menu.sh scripts/run-codex.sh \
  scripts/runtime-smoke-test.sh scripts/smoke-test.sh \
  scripts/test-run-codex-policy.sh scripts/validate-version-pins.sh

echo
echo '== Entrypoint, state wrapper, and deployment security configuration =='
for f in scripts/start-codex-web.sh scripts/run-direct-session.sh compose/truenas.yml compose/docker-compose.yml .github/workflows/publish-edge-amd64.yml; do
  if [ -f "$f" ]; then
    echo "--- $f"
    cat -n "$f"
  fi
done

echo
echo '== All executable scripts that invoke Codex or launcher-related commands =='
rg -n -i -C 4 --glob '*.sh' \
  '(?:^|[[:space:];|&()])(?:/usr/local/bin/)?codex(?:[[:space:];|&()]|$)|(?:^|[[:space:];|&()])(?:/usr/local/bin/)?run-codex(?:[[:space:];|&()]|$)' \
  scripts

echo
echo '== Launcher option/config parsing tests and config policy references =='
rg -n -i -C 3 \
  'sandbox_mode|approval_policy|ask_for_approval|--config|-c[[:space:]]|dangerously|full-auto|yolo' \
  scripts docs README.md README.es.md

echo
echo '== GitHub PR metadata, checks, and review comments =='
gh pr view 39 --repo eXPerience83/remote-dev-containers \
  --json number,state,isDraft,headRefOid,baseRefName,mergeStateStatus,reviewDecision,statusCheckRollup,url
gh api repos/eXPerience83/remote-dev-containers/pulls/39/comments --paginate \
  --jq '.[] | {path:.path,line:.line,side:.side,user:.user.login,body:.body,url:.html_url}'

Length of output: 177533


@eXPerience83 Final audit complete for 7a05401bf5158bd3688be29e71e21ea301cf0685. I found no remaining actionable issues in the 18-file change set.

Verified:

  • fixed launcher policy, pinned executable, override rejection, and argv behavior;
  • menu start, resume, and START_MODE=codex routing through run-codex;
  • non-persistent build-only test mount and non-destructive temporary launcher mocks;
  • EXIT/INT/TERM cleanup;
  • Bubblewrap absence checks and diagnostics;
  • documentation correctly treating approvals as controls rather than sandboxing;
  • successful AMD64 build and CodeRabbit checks on the exact PR head.

The PR should remain draft until the newly published exact candidate completes the documented manual TrueNAS validation: launch paths, approval behavior, diagnostics, persistence/permissions, and lifecycle behavior. Static review does not replace that environment-specific gate.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Owner Author

Final audit checkpoint for current head 7a05401bf5158bd3688be29e71e21ea301cf0685:

  • reviewed the complete 18-file PR diff and the 14-commit delta since the prematurely published 69b3b408... candidate;
  • verified the two later findings are addressed: the build-only policy test uses a read-only BuildKit bind mount, and launcher tests no longer replace /usr/local/bin/codex;
  • reviewed launcher override rejection, pinned binary execution, menu/resume/direct-start routing, diagnostics, cleanup traps, runtime smoke tests and documentation;
  • checked the exact Codex 0.146.0 CLI definitions for --sandbox, --ask-for-approval, --config, profile and resume handling;
  • audited the exact-SHA candidate workflow, artifact separation, digest publication and vulnerability gate;
  • corrected the PR description so it no longer claims a ShellCheck CI step or describes the merge-result build as an exact-head build;
  • current PR merge result has a green AMD64 build, runtime smoke tests and fixable-critical gate;
  • no new candidate will be requested until this review completes with no unresolved findings.

Please perform a full review of the current head and all changes since 69b3b408f758cef2af176bc7eb3bd173e807d5f9, with special attention to the non-destructive test indirection, BuildKit bind mount, real Codex compatibility checks, fixed-policy enforcement and documentation accuracy.

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

@eXPerience83 I’ll perform a full review of the current head and the complete delta since 69b3b408f758cef2af176bc7eb3bd173e807d5f9, with focused verification of test non-destructiveness, the BuildKit bind mount, Codex 0.146.0 compatibility, fixed-policy enforcement, routing, and documentation accuracy.

✅ Action performed

Full review finished.

eXPerience83 commented Jul 30, 2026

Copy link
Copy Markdown
Owner Author

CANCELLED / INVALIDATED: candidate publication was requested while the final full CodeRabbit review was still processing. Do not use any image produced by this trigger. A fresh candidate will be requested only after that review completes and all resulting findings are resolved.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@scripts/codex-menu.sh`:
- Around line 86-89: Update both run_interactive_and_harden invocations in the
Codex menu cases to call /usr/local/bin/run-codex instead of the PATH-resolved
run-codex command, including the resume argument in the second case.

In `@scripts/smoke-test.sh`:
- Around line 286-301: Update the launcher-generation logic around the sed
commands to safely escape temporary paths before embedding them in generated
shell scripts. Use shell-escaped values (for example via printf '%q') for
assignments involving fake_codex and test_run_codex, and separately escape each
value for use in sed replacement expressions so whitespace, &, |, and
backslashes remain valid. Keep the existing launcher validation checks
unchanged.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1597e1c9-0711-451d-b779-e4fceed68b0a

📥 Commits

Reviewing files that changed from the base of the PR and between daf64cd and 7a05401.

📒 Files selected for processing (18)
  • CHANGELOG.md
  • PROJECT_STATUS.md
  • README.es.md
  • README.md
  • docs/releases.md
  • docs/security.md
  • docs/tool-matrix.md
  • images/base/Dockerfile
  • images/codex/Dockerfile
  • scripts/attach-remote-dev-tmux.sh
  • scripts/base-verify.sh
  • scripts/codex-doctor.sh
  • scripts/codex-menu.sh
  • scripts/run-codex.sh
  • scripts/runtime-smoke-test.sh
  • scripts/smoke-test.sh
  • scripts/test-run-codex-policy.sh
  • scripts/validate-version-pins.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
scripts/**/*.sh

⚙️ CodeRabbit configuration file

scripts/**/*.sh: Review as production Bash. Check quoting, set -euo pipefail behavior, cleanup traps,
bounded retries and timeouts, non-interactive execution, idempotency and useful failure messages.
Flag commands that can hang, leak credentials, silently ignore failures or mutate persistent data unexpectedly.

Files:

  • scripts/base-verify.sh
  • scripts/codex-menu.sh
  • scripts/validate-version-pins.sh
  • scripts/codex-doctor.sh
  • scripts/attach-remote-dev-tmux.sh
  • scripts/run-codex.sh
  • scripts/test-run-codex-policy.sh
  • scripts/runtime-smoke-test.sh
  • scripts/smoke-test.sh
**/*.md

⚙️ CodeRabbit configuration file

**/*.md: Check that documentation matches the implemented behavior and clearly distinguishes experimental edge builds
from stable releases. Flag outdated versions, unsafe deployment guidance and claims not proven by CI or testing.

Files:

  • docs/tool-matrix.md
  • docs/releases.md
  • PROJECT_STATUS.md
  • docs/security.md
  • README.es.md
  • README.md
  • CHANGELOG.md
images/**/Dockerfile

⚙️ CodeRabbit configuration file

images/**/Dockerfile: Review for reproducibility, supply-chain security and minimal image growth.
Require pinned upstream versions and checksum or digest verification for downloaded binaries.
Flag secrets, floating base tags, unsafe remote-script execution, broken multi-architecture logic,
cache misuse, unnecessary packages and changes that weaken the secure-by-default runtime.

Files:

  • images/base/Dockerfile
  • images/codex/Dockerfile
🔇 Additional comments (17)
CHANGELOG.md (1)

29-29: LGTM!

Also applies to: 42-44, 64-66

PROJECT_STATUS.md (1)

21-23: LGTM!

Also applies to: 33-33, 58-60

README.es.md (1)

23-30: LGTM!

README.md (1)

26-32: LGTM!

Also applies to: 34-40, 125-126

docs/releases.md (1)

21-23: LGTM!

Also applies to: 55-55

docs/security.md (1)

9-18: LGTM!

Also applies to: 32-32

docs/tool-matrix.md (1)

26-26: LGTM!

scripts/run-codex.sh (1)

1-81: LGTM!

scripts/test-run-codex-policy.sh (1)

1-152: LGTM!

images/codex/Dockerfile (1)

55-55: LGTM!

Also applies to: 64-79

scripts/attach-remote-dev-tmux.sh (1)

19-19: LGTM!

images/base/Dockerfile (1)

54-60: LGTM!

scripts/base-verify.sh (1)

4-20: LGTM!

Also applies to: 36-36

scripts/validate-version-pins.sh (1)

175-176: LGTM!

scripts/codex-doctor.sh (1)

27-27: LGTM!

Also applies to: 45-57

scripts/runtime-smoke-test.sh (1)

14-23: LGTM!

Also applies to: 53-92

scripts/smoke-test.sh (1)

40-41: LGTM!

Also applies to: 109-115, 165-172

Comment thread scripts/codex-menu.sh Outdated
Comment thread scripts/smoke-test.sh

Copy link
Copy Markdown
Owner Author

Final reviewed head checkpoint: 56b1778957c06bf2e9de3571c15095906e63eca7.

Since the full review of 7a05401bf5158bd3688be29e71e21ea301cf0685, the final delta is limited to three scripts:

  • menu start and resume invoke /usr/local/bin/run-codex directly;
  • the tmux wrapper pins that launcher in a readonly variable and shell-quotes it with printf '%q' when constructing the session command;
  • the runtime smoke test shell-quotes temporary executable paths, separately escapes sed replacement metacharacters, substitutes only exact pinned assignments, and deliberately exercises spaces, &, | and backslashes.

The first path-escaping implementation failed the runtime smoke test and was replaced rather than waived. Build AMD64 run 259 for this exact head now passes the complete build, runtime tmux/Codex smoke path and vulnerability gate. Both review threads are confirmed as addressed and resolved.

Please perform one final incremental review of all changes from 7a05401bf5158bd3688be29e71e21ea301cf0685 through the current head, particularly the readonly tmux launcher indirection and generated-script quoting. No candidate will be requested until this review has completed without new actionable findings.

@coderabbitai review

Copy link
Copy Markdown
Owner Author

/publish-candidate 56b1778

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

base='7a05401bf5158bd3688be29e71e21ea301cf0685'
head='56b1778957c06bf2e9de3571c15095906e63eca7'

echo '== Commit identity and range =='
git show -s --format='base: %H%nsubject: %s' "$base"
git show -s --format='head: %H%nsubject: %s' "$head"
git log --oneline --decorate "${base}..${head}"
echo

echo '== Changed files and complete focused patch =='
git diff --name-status "$base..$head"
git diff --check "$base..$head"
git diff --find-renames --find-copies --unified=50 "$base..$head" -- \
  scripts/codex-menu.sh \
  scripts/attach-remote-dev-tmux.sh \
  scripts/runtime-smoke-test.sh
echo

echo '== AST outlines =='
for file in scripts/codex-menu.sh scripts/attach-remote-dev-tmux.sh scripts/runtime-smoke-test.sh; do
  echo "--- ${file}"
  ast-grep outline "$file" --items all 2>/dev/null || true
done
echo

echo '== Focused source contexts for routing, shell quoting, and generated launcher rewriting =='
rg -n -C 12 \
  'readonly .*run_codex|run-codex|printf.*%q|sed .*[-]e|sed_escape|replacement|fake.*codex|temp.*launcher|mktemp|START_MODE|codex_binary' \
  scripts/codex-menu.sh scripts/attach-remote-dev-tmux.sh scripts/runtime-smoke-test.sh
echo

echo '== Relevant full files with line numbers =='
for file in scripts/codex-menu.sh scripts/attach-remote-dev-tmux.sh scripts/runtime-smoke-test.sh; do
  echo "--- ${file}"
  cat -n "$file"
done
echo

echo '== PR metadata and exact head checks =='
gh pr view 39 --repo eXPerience83/remote-dev-containers \
  --json number,isDraft,headRefOid,mergeCommit,statusCheckRollup,url

Length of output: 25833


🏁 Script executed:

#!/bin/bash
set -euo pipefail

base='7a05401bf5158bd3688be29e71e21ea301cf0685'
head='56b1778957c06bf2e9de3571c15095906e63eca7'

echo '== Complete smoke-test patch in requested range =='
git diff --check "$base..$head" -- scripts/smoke-test.sh
git diff --unified=100 "$base..$head" -- scripts/smoke-test.sh
echo

echo '== Current smoke-test source =='
cat -n scripts/smoke-test.sh
echo

echo '== Pre-range smoke-test source for comparison =='
git show "${base}:scripts/smoke-test.sh" | cat -n
echo

echo '== Exact assignment and rewriting sites =='
rg -n -C 15 \
  'readonly (codex_binary|run_codex_binary)=|sed_escape|sed .*[-]e|fake_codex|fake_run_codex|fake_attach|mktemp|printf.*%q|START_MODE=codex|TMUX' \
  scripts/smoke-test.sh scripts/attach-remote-dev-tmux.sh scripts/run-codex.sh
echo

echo '== All range commits with per-commit patches =='
git log --format='commit %H%n%s%n' --patch --find-renames --find-copies \
  "${base}..${head}" -- \
  scripts/smoke-test.sh \
  scripts/attach-remote-dev-tmux.sh \
  scripts/codex-menu.sh
echo

echo '== Static validation: shell syntax and changed-line search =='
bash -n scripts/smoke-test.sh
bash -n scripts/attach-remote-dev-tmux.sh
bash -n scripts/codex-menu.sh
git diff --unified=0 "$base..$head" -- \
  scripts/smoke-test.sh scripts/attach-remote-dev-tmux.sh scripts/codex-menu.sh

Length of output: 68385


@eXPerience83 Final incremental review complete for 7a05401bf5158bd3688be29e71e21ea301cf0685..56b1778957c06bf2e9de3571c15095906e63eca7. I found no remaining actionable issues.

Verified:

  • menu start and resume use the pinned /usr/local/bin/run-codex path;
  • the direct tmux route stores the launcher in a readonly variable and safely shell-quotes it with printf '%q' before tmux evaluates the session command;
  • the smoke test uses isolated temporary launcher copies, targets only exact readonly assignments, and escapes backslashes, &, and the sed delimiter;
  • the generated-path test deliberately covers spaces, &, |, and backslashes, while preserving the installed launchers;
  • git diff --check and Bash syntax validation pass for all three changed scripts.

One terminology note only: the modified test is scripts/smoke-test.sh, rather than scripts/runtime-smoke-test.sh; the implementation and coverage described are otherwise consistent.

The PR remains appropriately draft pending the separate manual validation of the exact published TrueNAS candidate.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

base='7a05401bf5158bd3688be29e71e21ea301cf0685'
target='56b1778957c06bf2e9de3571c15095906e63eca7'

echo '== Revision verification =='
git rev-parse HEAD
git show -s --format='target=%H%nparents=%P%nsubject=%s' "$target"
git merge-base --is-ancestor "$base" "$target"
echo "base is an ancestor of target: $?"

echo
echo '== Changed files in requested range =='
git diff --name-status "$base..$target"
echo
echo '== Exact requested-range patch =='
git diff --find-renames --find-copies --unified=50 "$base..$target" -- \
  scripts/codex-menu.sh \
  scripts/attach-remote-dev-tmux.sh \
  scripts/runtime-smoke-test.sh

echo
echo '== Compact structures of affected scripts =='
for f in scripts/codex-menu.sh scripts/attach-remote-dev-tmux.sh scripts/runtime-smoke-test.sh; do
  echo "--- $f"
  ast-grep outline "$f" --items all 2>/dev/null || true
done

echo
echo '== Relevant definitions and invocations with context =='
rg -n -C 8 --glob '*.sh' \
  'readonly[[:space:]]+codex_launcher|run-codex|run-direct-session|printf[[:space:]]+.*%q|sed[[:space:]].*(-e|s[|/])|escape.*sed|shell_quote|REMOTE_DEV_CODEX_ARGS_FILE|fake_codex|fake-bin' \
  scripts

echo
echo '== All direct Codex executable calls and launch routes =='
rg -n -C 4 --glob '*.sh' \
  '(?:^|[[:space:];|&()])(?:/usr/local/bin/)?codex(?:[[:space:];|&()]|$)|(?:^|[[:space:];|&()])(?:/usr/local/bin/)?run-codex(?:[[:space:];|&()]|$)|START_MODE' \
  scripts

echo
echo '== PR metadata and current checks =='
gh pr view 39 --repo eXPerience83/remote-dev-containers \
  --json headRefOid,isDraft,mergeStateStatus,reviewDecision,statusCheckRollup,url

Length of output: 52038


🏁 Script executed:

#!/bin/bash
set -euo pipefail

base='7a05401bf5158bd3688be29e71e21ea301cf0685'
target='56b1778957c06bf2e9de3571c15095906e63eca7'

echo '== Commit sequence in review range =='
git log --reverse --format='%H %s' "$base..$target"

echo
echo '== Complete smoke-test patch in review range =='
git diff --unified=80 "$base..$target" -- scripts/smoke-test.sh

echo
echo '== Full relevant smoke-test regions =='
sed -n '1,115p' scripts/smoke-test.sh
echo '--- tmux smoke region ---'
sed -n '190,365p' scripts/smoke-test.sh

echo
echo '== Exact generated-launcher references =='
rg -n -C 5 --glob 'smoke-test.sh' \
  'escape_sed_replacement|fake_codex|test_run_codex|test_attach_tmux|readonly codex_binary=|readonly run_codex_binary=|sed[[:space:]]' \
  scripts

echo
echo '== Static checks for direct invocation/path interpolation in affected scripts =='
rg -n -C 3 --glob 'attach-remote-dev-tmux.sh' --glob 'codex-menu.sh' --glob 'smoke-test.sh' \
  '(^|[[:space:]])(/usr/local/bin/run-codex|run-codex|codex)([[:space:];]|$)|session_command=|printf -v|"\$test_(run_codex|attach_tmux)"|>\s*"\$test_' \
  scripts

echo
echo '== File modes and changed-file summary =='
git diff --summary "$base..$target"
git ls-tree "$target" scripts/attach-remote-dev-tmux.sh scripts/codex-menu.sh scripts/smoke-test.sh

Length of output: 35656


@eXPerience83 Final incremental review complete for 7a05401bf5158bd3688be29e71e21ea301cf0685..56b1778957c06bf2e9de3571c15095906e63eca7. I found no remaining actionable issues.

Verified:

  • menu start and resume now call the pinned /usr/local/bin/run-codex;
  • the direct tmux route pins and %q-quotes the launcher before embedding it in the session command;
  • generated launchers are isolated under the test’s temporary directory—installed launchers remain unchanged;
  • sed replacement values correctly escape \, &, and |, while substitutions target only the exact readonly assignments;
  • the smoke test exercises spaces, &, |, and backslashes, then verifies the generated scripts and direct Codex route;
  • cleanup covers temporary state and tmux servers on EXIT, INT, and TERM.

GitHub also reports successful Build AMD64 and CodeRabbit checks for this exact draft head. The remaining gate is the documented manual TrueNAS validation of a newly published exact-head candidate.

You are interacting with an AI system.

@eXPerience83
eXPerience83 marked this pull request as ready for review July 30, 2026 12:20

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
scripts/smoke-test.sh (1)

116-117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Pin both binaries in the installed-launcher assertion.

Line 117 still resolves run-codex through PATH; a shadowing pair can make this comparison pass without exercising the installed policy launcher. Use the fixed absolute paths.

Proposed fix
-codex_version="$(codex --version)"
-launcher_version="$(run-codex --version)"
+codex_version="$(/usr/local/bin/codex --version)"
+launcher_version="$(/usr/local/bin/run-codex --version)"
🤖 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 `@scripts/smoke-test.sh` around lines 116 - 117, Update the installed-launcher
assertion around codex_version and launcher_version to invoke both binaries
using their fixed absolute paths, replacing PATH-based resolution of run-codex.
Preserve the existing version comparison while ensuring it exercises the
installed policy launcher.
🤖 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.

Outside diff comments:
In `@scripts/smoke-test.sh`:
- Around line 116-117: Update the installed-launcher assertion around
codex_version and launcher_version to invoke both binaries using their fixed
absolute paths, replacing PATH-based resolution of run-codex. Preserve the
existing version comparison while ensuring it exercises the installed policy
launcher.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a4554bfa-378e-40a8-8a37-bdddd27ee92f

📥 Commits

Reviewing files that changed from the base of the PR and between 7a05401 and 56b1778.

📒 Files selected for processing (3)
  • scripts/attach-remote-dev-tmux.sh
  • scripts/codex-menu.sh
  • scripts/smoke-test.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
scripts/**/*.sh

⚙️ CodeRabbit configuration file

scripts/**/*.sh: Review as production Bash. Check quoting, set -euo pipefail behavior, cleanup traps,
bounded retries and timeouts, non-interactive execution, idempotency and useful failure messages.
Flag commands that can hang, leak credentials, silently ignore failures or mutate persistent data unexpectedly.

Files:

  • scripts/codex-menu.sh
  • scripts/attach-remote-dev-tmux.sh
  • scripts/smoke-test.sh
🔇 Additional comments (3)
scripts/codex-menu.sh (1)

86-89: LGTM!

scripts/attach-remote-dev-tmux.sh (1)

7-7: LGTM!

Also applies to: 20-21

scripts/smoke-test.sh (1)

90-96: LGTM!

Also applies to: 271-315

@eXPerience83
eXPerience83 merged commit a60670b into main Jul 30, 2026
2 checks passed
@eXPerience83
eXPerience83 deleted the security/no-bubblewrap-outer-isolation branch July 30, 2026 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant