mirrors/hermes-agent
1
0
Fork 0
mirror of https://github.com/NousResearch/hermes-agent.git synced 2026-07-31 19:16:29 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

fix(terminal): atomic env-snapshot replacement to prevent PATH corruption (#38249)

Browse source 
Concurrent terminal calls in one session both source AND rewrite the shared
env snapshot. The per-command re-dump used `export -p > snap` — a non-atomic
truncate-then-write in place (the code even noted "last-writer-wins"). A
concurrent `source snap` could read a half-written file and embed literal
`declare -x` / `export` fragments into PATH, breaking `ls`/`git`/`tr` with
command-not-found until PATH was manually repaired. The corruption persisted
because the malformed env got saved back into the snapshot.

Write to a unique temp file then `mv -f` over the snapshot. `mv`/rename is
atomic on POSIX (same filesystem), so a reader always sees the old-complete or
new-complete file — never a torn one. Applied at both the init_session
bootstrap and the per-command re-dump. `$$` (bash PID) makes the temp name
unique per concurrent process so their temp writes can't collide before the mv.

Salvaged from #38279 by @kyssta-exe (authorship preserved via cherry-pick).
On top of the original I hardened the temp-path quoting: the static path is now
shlex-quoted with `$$` left outside the quotes to expand, so a snapshot path
with a space or a Windows `C:/Users/...` drive letter doesn't break the shell
(matching the existing quoting care for the snapshot path itself; `bash -n`
verified on a spaced path). Added a regression test class (atomic temp+mv used,
not in-place write; per-process-unique temp; static part quoted; bootstrap also
atomic). 22 tests pass, mutation-verified (reverting to the in-place write fails
the atomic tests), ruff clean.

Closes #38249. Supersedes #38267 (serialize-execution lock — heavier and
serializes the spawn-per-call concurrency this fix preserves).
This commit is contained in:
kyssta-exe 2026-06-26 21:47:22 +05:30 • committed by kshitijk4poor
parent 5038678647
commit a54be30b6e
4 changed files with 183 additions and 9 deletions
Download patch file Download diff file Expand all files Collapse all files

1
scripts/release.py
View file

@ -259,7 +259,6 @@ AUTHOR_MAP = {
"9592417+adam91holt@users.noreply.github.com": "adam91holt",
"kchuang1015@users.noreply.github.com": "kchuang1015",
"maheshthedev@gmail.com": "MaheshtheDev",
"kyssta-exe@users.noreply.github.com": "kyssta-exe",
"shriganesh.patel@gmail.com": "ashishpatel26",
"45688690+fujinice@users.noreply.github.com": "fujinice",
"276689385+carltonawong@users.noreply.github.com": "carltonawong",

136
tests/tools/test_base_environment.py
View file

@ -4,6 +4,7 @@ Tests _wrap_command(), _extract_cwd_from_output(), _embed_stdin_heredoc(),
init_session() failure handling, and the CWD marker contract.
"""
import subprocess
from unittest.mock import MagicMock
from tools.environments.base import BaseEnvironment
@ -90,6 +91,70 @@ class TestWrapCommand:
assert "exit 126" in wrapped
class TestAtomicSnapshotWrite:
"""Regression for #38249: concurrent terminal calls in one session both
source AND rewrite the shared env snapshot. A non-atomic ``export -p >
snap`` truncates-then-writes in place, so a concurrent ``source snap`` can
read a half-written file and embed ``declare -x``/``export`` fragments into
PATH, breaking ``ls``/``git``/``tr`` with command-not-found. The write must
be atomic: ``export -p > snap.tmp.$$`` then ``mv -f snap.tmp.$$ snap`` (mv
is atomic on POSIX same-fs), so a reader sees the old-or-new complete file,
never a torn one."""
def test_wrap_command_uses_atomic_temp_then_mv(self):
env = _TestableEnv()
env._snapshot_ready = True
wrapped = env._wrap_command("echo hi", "/tmp")
# Writes go to a temp file, not directly over the live snapshot.
assert "export -p > " in wrapped
assert ".tmp." in wrapped
# Then an atomic rename onto the real snapshot path.
assert "mv -f " in wrapped
# The env-dump must NOT write the live snapshot in place (the bug).
snap = env._snapshot_path
assert f"export -p > {snap} " not in wrapped
assert f"export -p > '{snap}'" not in wrapped
def test_temp_path_is_per_process_unique(self):
"""``$$`` (bash PID) makes concurrent processes use distinct temp files
so their temp writes can't clobber each other before the mv."""
env = _TestableEnv()
env._snapshot_ready = True
wrapped = env._wrap_command("echo hi", "/tmp")
assert ".tmp.$$" in wrapped or ".tmp.'$$'" in wrapped or "'.tmp.'$$" in wrapped
def test_temp_path_static_part_is_quoted(self):
"""The static path portion must be shlex-quoted (Windows/Git-Bash
``C:/Users/...`` or spaces) while ``$$`` stays outside the quotes so
it still expands. The bug-prone form is a bare unquoted temp path."""
env = _TestableEnv()
env._snapshot_ready = True
env._snapshot_path = "/tmp/has space/hermes-snap-x.sh"
wrapped = env._wrap_command("echo hi", "/tmp")
# The space in the path must be quoted, not left bare in the temp name.
assert "/tmp/has space/hermes-snap-x.sh.tmp.$$" not in wrapped
# $$ must remain shell-expandable (not swallowed inside quotes as a literal).
assert "$$" in wrapped
def test_init_session_bootstrap_also_atomic(self):
"""The init_session bootstrap (first snapshot write) must be atomic too
— it's the same shared file a concurrent command could source."""
env = _TestableEnv()
captured = {}
def fake_run_bash(cmd_string, *, login=False, timeout=120, stdin_data=None):
captured["cmd"] = cmd_string
raise RuntimeError("stop after capture") # we only need the script
env._run_bash = fake_run_bash # type: ignore[assignment]
try:
env.init_session()
except Exception:
pass
boot = captured.get("cmd", "")
assert ".tmp." in boot and "mv -f " in boot, boot
class TestExtractCwdFromOutput:
def test_happy_path(self):
env = _TestableEnv()
@ -194,3 +259,74 @@ class TestCwdMarker:
env1 = _TestableEnv()
env2 = _TestableEnv()
assert env1._cwd_marker != env2._cwd_marker
class TestAtomicSnapshotConcurrencyBehavioral:
"""Behavioral regression for #38249 — actually EXECUTES the generated
snapshot write concurrently and asserts the file never tears.
The string-inspection tests prove the right script is emitted; this proves
the emitted script's guarantee holds: N concurrent writers + readers, and
the snapshot is ALWAYS a complete, parseable env dump — never truncated
mid-line with a `declare -x` / `export` fragment that would corrupt PATH.
"""
def _run(self, script, env=None):
return subprocess.run(["/bin/bash", "-c", script], capture_output=True,
text=True, env=env)
def test_concurrent_writes_never_tear_the_snapshot(self, tmp_path):
import shutil
if not shutil.which("bash"):
import pytest; pytest.skip("bash required")
snap = str(tmp_path / "hermes-snap-x.sh")
_q = __import__("shlex").quote
_snap_tmp = _q(snap + ".tmp.") + "$$"
# One writer iteration = the exact atomic sequence _wrap_command emits.
writer = (
"for i in $(seq 1 40); do "
f"export BIG_$i=$(head -c 400 /dev/zero | tr '\\0' x); "
f"{{ export -p > {_snap_tmp} && mv -f {_snap_tmp} {_q(snap)}; }} "
f"2>/dev/null || rm -f {_snap_tmp} 2>/dev/null || true; "
"done"
)
# Reader: repeatedly source the snapshot and check PATH never absorbs
# an `export `/`declare -x` fragment (the corruption signature).
reader = (
f"export PATH=/usr/bin:/bin; "
"for i in $(seq 1 80); do "
f"( source {_q(snap)} >/dev/null 2>&1 || true; "
"case \"$PATH\" in *'declare -x'*|*'export '*) echo CORRUPT; ;; esac ); "
"done"
)
# seed a valid snapshot first
self._run(f"export -p > {_q(snap)}")
# launch 6 concurrent writers + 6 readers
procs = [self._run(f"{writer} & {writer} & {reader} & {reader} & wait")
for _ in range(3)]
corrupt = any("CORRUPT" in p.stdout for p in procs)
assert not corrupt, "snapshot tore — PATH absorbed a declare-x/export fragment"
# final snapshot must still be a complete, sourceable env dump
final = self._run(f"source {_q(snap)} >/dev/null 2>&1 && echo OK || echo BROKEN")
assert "OK" in final.stdout, f"final snapshot not sourceable: {final.stdout} {final.stderr}"
def test_failed_export_does_not_destroy_good_snapshot(self, tmp_path):
"""If `export -p` fails, the `&&`-chained mv must NOT clobber the
existing good snapshot (the failure-path fix)."""
import shutil
if not shutil.which("bash"):
import pytest; pytest.skip("bash required")
snap = str(tmp_path / "snap.sh")
_q = __import__("shlex").quote
self._run(f"echo 'export GOOD=1' > {_q(snap)}") # seed good snapshot
_snap_tmp = _q(snap + ".tmp.") + "$$"
# Simulate a failed dump: redirect export into an unwritable temp dir so
# the export side fails; mv must then NOT run (&&) and not clobber snap.
bad_tmp = _q("/nonexistent-dir/snap.tmp.") + "$$"
script = (
f"{{ export -p > {bad_tmp} && mv -f {bad_tmp} {_q(snap)}; }} "
f"2>/dev/null || rm -f {bad_tmp} 2>/dev/null || true"
)
self._run(script)
out = self._run(f"cat {_q(snap)}")
assert "export GOOD=1" in out.stdout, "good snapshot was destroyed by a failed export"

44
tools/environments/base.py
View file

@ -371,13 +371,31 @@ class BaseEnvironment(ABC):
# backends) into every terminal-tool response.
_quoted_snap = shlex.quote(self._snapshot_path)
_quoted_cwd_file = shlex.quote(self._cwd_file)
# Use atomic file replacement: write to a temp file, then mv to the
# final path. This prevents concurrent source() calls from reading a
# half-written snapshot when another terminal command finishes and
# rewrites the env vars (issue #38249). `mv` is atomic on POSIX
# when src and dest are on the same filesystem, so source() will
# either see the old complete snapshot or the new complete one —
# never a partial/truncated file.
#
# Quote the static path portion and leave ``$$`` (the bash PID, which
# makes the temp name unique per concurrent process) outside the
# quotes so it still expands. Matches the Windows/Git-Bash quoting
# care taken for ``_quoted_snap`` — an unquoted temp path with a
# ``C:/Users/...`` drive letter or a space would otherwise break.
_snap_tmp = shlex.quote(self._snapshot_path + ".tmp.") + "$$"
bootstrap = (
f"export -p > {_quoted_snap}\n"
f"declare -f | grep -vE '^_[^_]' >> {_quoted_snap}\n"
f"alias -p >> {_quoted_snap}\n"
f"echo 'shopt -s expand_aliases' >> {_quoted_snap}\n"
f"echo 'set +e' >> {_quoted_snap}\n"
f"echo 'set +u' >> {_quoted_snap}\n"
f"export -p > {_snap_tmp}\n"
f"declare -f | grep -vE '^_[^_]' >> {_snap_tmp}\n"
f"alias -p >> {_snap_tmp}\n"
f"echo 'shopt -s expand_aliases' >> {_snap_tmp}\n"
f"echo 'set +e' >> {_snap_tmp}\n"
f"echo 'set +u' >> {_snap_tmp}\n"
# Atomically publish the assembled snapshot. mv only fires if the
# export succeeded; on failure the partial temp is removed rather
# than left to be sourced or orphaned.
f"mv -f {_snap_tmp} {_quoted_snap} || rm -f {_snap_tmp}\n"
f"builtin cd {_quoted_cwd} 2>/dev/null || true\n"
f"pwd -P > {_quoted_cwd_file} 2>/dev/null || true\n"
f"printf '\\n{self._cwd_marker}%s{self._cwd_marker}\\n' \"$(pwd -P)\"\n"
@ -427,6 +445,10 @@ class BaseEnvironment(ABC):
# :meth:`init_session` for the same fix on the bootstrap block.
_quoted_snap = shlex.quote(self._snapshot_path)
_quoted_cwd_file = shlex.quote(self._cwd_file)
# Atomic env-snapshot replacement (issue #38249) — write to a unique
# temp file, then mv over the snapshot so a concurrent source() never
# reads a truncated file. Static path quoted; ``$$`` left to expand.
_snap_tmp = shlex.quote(self._snapshot_path + ".tmp.") + "$$"
parts = []
@ -451,9 +473,15 @@ class BaseEnvironment(ABC):
parts.append(f"eval '{escaped}'")
parts.append("__hermes_ec=$?")
# Re-dump env vars to snapshot (last-writer-wins for concurrent calls)
# Re-dump env vars to snapshot (atomic replacement to avoid races).
# Chain mv on the export succeeding (``&&``) so a failed/partial dump
# never replaces a good snapshot — and clean up the temp on failure so
# it isn't orphaned.
if self._snapshot_ready:
parts.append(f"export -p > {_quoted_snap} 2>/dev/null || true")
parts.append(
f"{{ export -p > {_snap_tmp} && mv -f {_snap_tmp} {_quoted_snap}; }} "
f"2>/dev/null || rm -f {_snap_tmp} 2>/dev/null || true"
)
# Write CWD to file (local reads this) and stdout marker (remote parses this)
parts.append(f"pwd -P > {_quoted_cwd_file} 2>/dev/null || true")

11
tools/environments/local.py
View file

@ -875,3 +875,14 @@ class LocalEnvironment(BaseEnvironment):
os.unlink(f)
except OSError:
pass
# Remove any orphaned atomic-write temp snapshots (snap.tmp.<pid>) that
# a failed mv could have left behind (#38249).
try:
import glob
for tmp in glob.glob(f"{self._snapshot_path}.tmp.*"):
try:
os.unlink(tmp)
except OSError:
pass
except Exception:
pass