Skip to content

fix(engines): validate mktemp output and set trap before temp creation in opencode sandbox - #108

Open
JackReis wants to merge 7 commits into
NateBJones-Projects:mainfrom
JackReis:fix/opencode-sandbox-cleanup-and-validation
Open

fix(engines): validate mktemp output and set trap before temp creation in opencode sandbox#108
JackReis wants to merge 7 commits into
NateBJones-Projects:mainfrom
JackReis:fix/opencode-sandbox-cleanup-and-validation

Conversation

@JackReis

Copy link
Copy Markdown

Bugs Found During Audit Two related issues in engines/opencode-sandboxed.sh were discovered while auditing the script for macOS-specific problems. ### 1. Sandbox bypass on mktemp failure (security) Line 46 (original): bash SCRATCH=$(cd $(mktemp -d -t ringer-opencode-scratch) && pwd -P) Bash disables set -e inside command substitutions. If mktemp -d fails (disk full, permissions, path too long), the inner substitution returns empty, cd falls through to the current directory, and SCRATCH is set to $HOME or PWD. The sandbox then allows writes to the entire home directory. Reproduced on macOS: $ bash -c set -euo pipefail; OUTER=$(cd $(false) && pwd -P); echo OUTER=$OUTER OUTER=/Users/hermes ### 2. Profile file leak on early termination Lines 47-49 (original): bash PROFILE=$(mktemp -t ringer-opencode-prof) cleanup() { rm -rf $SCRATCH $PROFILE; } trap cleanup EXIT If the script receives a signal or crashes between mktemp and trap cleanup EXIT, the profile file is never cleaned up. Over many runs this accumulates files in /var/folders/.../T/. ## Fix - Move trap cleanup EXIT before any temp file creation. - Define cleanup() early with guards against uninitialized SCRATCH/PROFILE (so it is safe to call before those vars are set). - Separate mktemp -d from cd/pwd -P and explicitly validate the result: exit with an error message if the directory was not created. - Same validation for the profile mktemp. ## Verification - bash -n syntax check passes. - The script still runs correctly in --no-sandbox mode (verified live with opencode on macOS). - No other tracked files in the repo use mktemp, so this is a complete fix for the pattern. ## Files Changed - engines/opencode-sandboxed.sh (+23 / −3)

…+ routing helper

Resolves a 2026-08-16 false-negative where 'which codex' missed
/Users/hermes/.local/bin/codex (off default PATH), wrongly forcing a
delegate_task fallback on Aegis. New tools resolve each [engines.NAME].bin
directly from config.toml (os.path.exists / shutil.which), never 'which'.
Scripts are self-locating (resolve paths relative to their own dir), so the
canonical clone may live anywhere (e.g. ~/Projects/ringer-fleet-swarm).
- scripts/ring-engine-probe.py: per-engine AVAILABLE/MISSING report, exit 0/1/2
- checks/engine-bin-probe.py: lint-gate (PASS/FAIL) for pre-dispatch
- scripts/verify-ringer-engines.sh: preflight wrapper, aborts on MISSING
- scripts/ring-route.sh: host+engine router, failing closed on probe
- README: Preflight engine-bin gate section
@JackReis

Copy link
Copy Markdown
Author

Post-review sanity check: passed ✅

This PR went through two independent AI-assisted review passes after the initial submission, with all findings fixed and re-verified:

Round 1 — Gemini 3.1 Pro review flagged:

  • Temp-dir leak: a failed canonicalizing cd left the mktemp dir trap-unreachable (cleanup only checked $SCRATCH, unset at that point)
  • Under set -e, bare VAR="$(mktemp ...)" assignments exited silently, bypassing the friendly diagnostics

→ Fixed in 54660e8 and 0305e18 (SCRATCH pre-assignment, if ! guards on all mktemp calls).

Round 2 — Claude Code sanity check flagged:

  • The failed-cd path still clobbered the pre-set SCRATCH with an empty string, defeating the round-1 fix on paper
  • A failing rm inside the EXIT trap could abort the trap mid-run and replace the script's exit status

→ Fixed in ad86682 (canonical path resolves into SCRATCH_REAL and is promoted only on success; cleanup is now fault-tolerant).

Re-verification: all findings confirmed resolved, including empirical failure-injection tests (failed cd → trap still removes the temp dir; failing rm → PROFILE cleanup completes and exit status is preserved). Success-path behavior is unchanged; BSD/macOS mktemp usage verified correct.

From a shell-safety standpoint this is merge-ready on my end.

@JackReis

Copy link
Copy Markdown
Author

hey nate and team hope this is useful

JackReis and others added 5 commits August 21, 2026 13:20
Running pytest from the repo root had no config, so it collected 258
tests by sweeping test_*.py under swarms/*/work/... and templates/
(transient swarm work-dirs). Add pytest.ini with `testpaths = tests`
to restrict default collection to the real suite (258 -> 175, all
under tests/).

Pre-existing tests/ failures (foreign hardcoded path, stale date
assertion, lint template) are independent of this change.

Reviewed-by: qwen2.5-coder:14b (local alternate-brain review: APPROVE)
Authored-by: Zatara (session:437bb9f9)

Co-authored-by: hermes <hermes@Aegis.local>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@JackReis
JackReis force-pushed the fix/opencode-sandbox-cleanup-and-validation branch from ad86682 to 49e59dd Compare August 21, 2026 18:21
@JackReis

Copy link
Copy Markdown
Author

Smoke-tested the rebased branch (now on current upstream main) on macOS:

Path Result
Sandboxed run (opencode --version under Seatbelt, opencode 1.17.14) ✅ exit 0
Temp cleanup after success ✅ no leftover ringer-opencode-* in /tmp
mktemp failure (stubbed) ✅ clean diagnostic + exit 1, no leaks
--no-sandbox passthrough ✅ exit 0

Both the silent set -e death and the temp-file leak paths are fixed and verified behaviorally.

The opencode sandbox fix branch accidentally included two JackReis/ringer
main-only commits (engine-bin probe + pytest testpaths). Strip those so
this PR diffs only engines/opencode-sandboxed.sh against upstream main.

Co-authored-by: Cursor <cursoragent@cursor.com>
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