mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix: brace-group the filtered export dump so $BASHPID expands in the parent shell
Follow-up for salvaged PR #69380. The snippet 'export -p | grep -vE ... > $tmp.$BASHPID || true' attaches the redirect to the grep pipeline segment, so $BASHPID expands inside grep's pipeline subshell — a DIFFERENT pid than the parent shell that expands the follow-up 'mv $tmp' operand. The dump landed in an orphaned temp file, mv failed silently (2>/dev/null), and the shared snapshot never updated again: exported env / venv activation stopped persisting between commands (tests/tools/test_local_shell_init.py TestSnapshotEndToEnd caught it). Wrap the pipeline in a brace group and attach the redirect to the group so the expansion happens in the current shell, matching mv's expansion. Also rename the shell-init probe var HERMES_SESSION_ENV_PROBE -> HERMES_STICKY_ENV_PROBE: it matched the HERMES_SESSION_ prefix the salvaged fix now intentionally strips from snapshots, and the snippet-shape test is updated to pin the brace-group contract.
This commit is contained in:
parent
50e1d7e3ae
commit
95a566b1e7
3 changed files with 29 additions and 6 deletions
|
|
@ -194,12 +194,12 @@ class TestSnapshotEndToEnd:
|
|||
env = LocalEnvironment(cwd=str(tmp_path), timeout=15)
|
||||
try:
|
||||
first = env.execute(
|
||||
'export HERMES_SESSION_ENV_PROBE="sticky"; '
|
||||
'export HERMES_STICKY_ENV_PROBE="sticky"; '
|
||||
'export PATH="/tmp/hermes-session-bin:$PATH"; '
|
||||
'echo "first=$HERMES_SESSION_ENV_PROBE"'
|
||||
'echo "first=$HERMES_STICKY_ENV_PROBE"'
|
||||
)
|
||||
second = env.execute(
|
||||
'echo "second=$HERMES_SESSION_ENV_PROBE"; echo "PATH=$PATH"'
|
||||
'echo "second=$HERMES_STICKY_ENV_PROBE"; echo "PATH=$PATH"'
|
||||
)
|
||||
finally:
|
||||
env.cleanup()
|
||||
|
|
|
|||
|
|
@ -58,7 +58,14 @@ def test_export_snippet_shape():
|
|||
assert "export -p" in snippet
|
||||
assert "grep -vE" in snippet
|
||||
assert "/tmp/snap.tmp.$BASHPID" in snippet
|
||||
assert snippet.rstrip().endswith("|| true")
|
||||
# The redirection must be attached to a brace group wrapping the pipeline,
|
||||
# NOT to the grep segment: a redirect on grep expands $BASHPID inside
|
||||
# grep's pipeline subshell (a different PID than the parent shell that
|
||||
# expands the follow-up ``mv`` operand), silently orphaning the dump and
|
||||
# breaking snapshot env persistence entirely.
|
||||
assert snippet.lstrip().startswith("{ ")
|
||||
assert "|| true; }" in snippet
|
||||
assert snippet.rstrip().endswith("> /tmp/snap.tmp.$BASHPID")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -412,9 +412,20 @@ def _export_dump_excluding_session_vars(tmp_path: str) -> str:
|
|||
persist across sessions in the shared snapshot. ``grep -vE`` returns exit 1
|
||||
when it filters everything, so ``|| true`` keeps the pipeline's success
|
||||
contract intact for the callers that chain on it.
|
||||
|
||||
The pipeline MUST be wrapped in a brace group with the redirection applied
|
||||
to the group, not to the last pipeline segment. *tmp_path* typically embeds
|
||||
``$BASHPID`` for concurrency-safe temp names; a redirection attached
|
||||
directly to ``grep`` is expanded inside grep's own pipeline subshell, where
|
||||
``$BASHPID`` resolves to the grep subshell's PID — while the caller's
|
||||
follow-up ``mv $tmp`` expands in the parent shell to a DIFFERENT PID. The
|
||||
dump then lands in an orphaned temp file and the snapshot silently never
|
||||
updates (all exported-env persistence breaks). The brace-group redirect is
|
||||
expanded in the current shell, keeping both expansions consistent.
|
||||
"""
|
||||
return (
|
||||
f"export -p | grep -vE '{_SNAPSHOT_EXCLUDED_ENV_REGEX}' > {tmp_path} || true"
|
||||
f"{{ export -p | grep -vE '{_SNAPSHOT_EXCLUDED_ENV_REGEX}' || true; }} "
|
||||
f"> {tmp_path}"
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -678,9 +689,14 @@ class BaseEnvironment(ABC):
|
|||
# Chain mv on the export succeeding so a failed/partial dump never
|
||||
# replaces a good snapshot; drop the temp on failure so it isn't
|
||||
# orphaned (cleaned up wholesale in LocalEnvironment.cleanup too).
|
||||
# NOTE: the redirection must be attached to a brace group, not to the
|
||||
# grep pipeline segment — ``_snap_tmp`` embeds ``$BASHPID``, and a
|
||||
# redirect on grep is expanded inside grep's pipeline subshell (a
|
||||
# different PID than the parent shell that expands the ``mv`` operand),
|
||||
# silently orphaning the dump. See _export_dump_excluding_session_vars.
|
||||
if self._snapshot_ready:
|
||||
parts.append(
|
||||
f"{{ export -p | grep -vE '{_SNAPSHOT_EXCLUDED_ENV_REGEX}' > {_snap_tmp} "
|
||||
f"{{ {_export_dump_excluding_session_vars(_snap_tmp)} "
|
||||
f"&& mv -f {_snap_tmp} {_quoted_snap}; }} "
|
||||
f"2>/dev/null || rm -f {_snap_tmp} 2>/dev/null || true"
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue