fix(host): keep the session workspace off the runner's sys.path (OMNI-2963) (#4688)
* fix(host): keep the session workspace off the runner's sys.path (OMNI-2963)
Opening a session inside an omnigent checkout ran a different omnigent than
the installed one. Runners are spawned with `python -m`, which prepends the
process cwd to sys.path, and since 3419de8d the runner's cwd is the session
workspace, so a workspace that is itself a checkout won over site-packages.
A long-lived daemon plus a mid-flight `git pull` then left the host and the
zygote on different code, surfacing as "runner fork request requires a cwd".
Spawn the runner, the zygote and the harness runner with -P so cwd never
lands on sys.path, and re-add the workspace in the runner entry once the real
omnigent is imported (and so can no longer be shadowed), keeping
spec-declared local tools importable by dotted path. Also pass -I to the
hermes MCP bridge, the only native bridge that was missing it.
Co-authored-by: Isaac
Signed-off-by: Dhruv Gupta <dhruv0811@gmail.com>
* chore(tests): reword the shadowing docstrings
Co-authored-by: Isaac
Signed-off-by: Dhruv Gupta <dhruv0811@gmail.com>
* fix(tests): hand spawned harness children the project root via PYTHONPATH
Harnesses now spawn with -P, so a directly-exec'd harness no longer inherits
the repo root through its cwd. Tests that register a fixture harness module
(tests._fixtures.runner_test_harness) must pass that path in the environment,
which is what tests/runtime/harnesses/conftest.py already does; mirror that
fixture for tests/runner. Also update the hermes MCP-config assertion for the
added -I, matching the qwen bridge test.
Co-authored-by: Isaac
Signed-off-by: Dhruv Gupta <dhruv0811@gmail.com>
* fix(cli): spawn the local runner with -P too
The CLI's own runner spawn inherits the CLI's cwd, so running omnigent from
inside a checkout shadowed the installed package exactly as the daemon path
did. Raised by review; the earlier audit missed it because this argv sits on
one line.
Co-authored-by: Isaac
Signed-off-by: Dhruv Gupta <dhruv0811@gmail.com>
* fix(codex-native): pass -I to the codex serve-mcp bridge
codex_mcp_config_overrides built its own args list without -I, so the one
bridge codex launches stayed open to the workspace shadowing that every other
bridge already blocks. Raised by review, which also caught that the PR
description wrongly claimed codex already had it.
Co-authored-by: Isaac
Signed-off-by: Dhruv Gupta <dhruv0811@gmail.com>
---------
Signed-off-by: Dhruv Gupta <dhruv0811@gmail.com>
This commit is contained in:
+5
-1
@@ -3441,7 +3441,11 @@ def _start_cli_runner_process(
|
||||
try:
|
||||
with child_logging_popen_kwargs(env) as logging_kwargs:
|
||||
runner_proc: subprocess.Popen[bytes] = subprocess.Popen(
|
||||
[sys.executable, "-m", "omnigent.runner._entry"],
|
||||
# This runner inherits the CLI's cwd, so -P is what stops a
|
||||
# checkout you launched from shadowing the installed omnigent
|
||||
# (the daemon and zygote spawns do the same). _entry re-adds the
|
||||
# cwd afterwards, keeping spec-declared local tools importable.
|
||||
[sys.executable, "-P", "-m", "omnigent.runner._entry"],
|
||||
env=env,
|
||||
stdout=log_fh,
|
||||
stderr=log_fh,
|
||||
|
||||
@@ -188,8 +188,11 @@ def codex_mcp_config_overrides(
|
||||
``['mcp_servers.omnigent.command="python"', ...]``.
|
||||
"""
|
||||
python = python_executable or sys.executable
|
||||
# -I: codex launches this MCP server in the workspace, so cwd must stay off
|
||||
# sys.path or a workspace that is an omnigent checkout shadows the installed
|
||||
# package. Matches every other bridge's serve-mcp invocation.
|
||||
args_toml = json.dumps(
|
||||
["-m", "omnigent.claude_native_bridge", "serve-mcp", "--bridge-dir", str(bridge_dir)]
|
||||
["-I", "-m", "omnigent.claude_native_bridge", "serve-mcp", "--bridge-dir", str(bridge_dir)]
|
||||
)
|
||||
return [
|
||||
f'mcp_servers.omnigent.command="{python}"',
|
||||
|
||||
@@ -380,6 +380,10 @@ def write_policy_hook_config(
|
||||
"omnigent": {
|
||||
"command": sys.executable,
|
||||
"args": [
|
||||
# hermes launches MCP servers in the workspace; -I keeps that
|
||||
# cwd off sys.path so a workspace that is an omnigent checkout
|
||||
# can't shadow the installed package (as every other bridge does).
|
||||
"-I",
|
||||
"-m",
|
||||
"omnigent.claude_native_bridge",
|
||||
"serve-mcp",
|
||||
|
||||
@@ -1457,7 +1457,10 @@ class HostProcess:
|
||||
|
||||
with child_logging_popen_kwargs(env) as logging_kwargs:
|
||||
proc = subprocess.Popen(
|
||||
[sys.executable, "-m", "omnigent.runner._entry"],
|
||||
# -P keeps cwd off sys.path: a workspace that is itself an
|
||||
# omnigent checkout would otherwise shadow the installed
|
||||
# package. _entry re-adds it for spec-declared local tools.
|
||||
[sys.executable, "-P", "-m", "omnigent.runner._entry"],
|
||||
env=env,
|
||||
# A daemon may outlive the checkout it started from.
|
||||
cwd=str(workspace),
|
||||
|
||||
@@ -227,7 +227,10 @@ class ZygoteManager:
|
||||
tty_kwargs = stack.enter_context(child_logging_popen_kwargs(env))
|
||||
pass_fds = (child_fd, *tty_kwargs.get("pass_fds", ()))
|
||||
return subprocess.Popen(
|
||||
[self._python, "-m", "omnigent.runner._zygote"],
|
||||
# -P keeps the daemon's cwd off sys.path, so a daemon started
|
||||
# inside an omnigent checkout can't hand forked runners a
|
||||
# different omnigent than the installed one.
|
||||
[self._python, "-P", "-m", "omnigent.runner._zygote"],
|
||||
env=env,
|
||||
pass_fds=pass_fds,
|
||||
stdin=subprocess.DEVNULL,
|
||||
|
||||
@@ -1742,6 +1742,13 @@ def main() -> None:
|
||||
"""
|
||||
from omnigent.process_logging import configure_process_logging
|
||||
|
||||
# Spawned with -P, so the workspace is not on sys.path. Re-add it now that
|
||||
# the real omnigent is imported (it can no longer be shadowed) so
|
||||
# spec-declared local tools living in the workspace still import.
|
||||
cwd = os.getcwd()
|
||||
if cwd not in sys.path:
|
||||
sys.path.insert(0, cwd)
|
||||
|
||||
configure_process_logging("runner", force=True)
|
||||
_install_crash_logging()
|
||||
try:
|
||||
|
||||
@@ -1240,6 +1240,9 @@ class HarnessProcessManager:
|
||||
self._harness_zygote_disabled = True
|
||||
return await asyncio.create_subprocess_exec(
|
||||
sys.executable,
|
||||
# -P keeps the inherited workspace cwd off sys.path so it can't
|
||||
# shadow the installed omnigent the harness module comes from.
|
||||
"-P",
|
||||
"-m",
|
||||
"omnigent.runtime.harnesses._runner",
|
||||
*runner_argv,
|
||||
|
||||
@@ -1641,6 +1641,9 @@ def test_start_cli_runner_process_uses_token_bound_runner_id(
|
||||
assert env[RUNNER_TUNNEL_BINDING_TOKEN_ENV_VAR] == "bind-token"
|
||||
assert env[RUNNER_PARENT_PID_ENV_VAR] == str(os.getpid())
|
||||
assert env[RUNNER_WORKSPACE_ENV_VAR] == str(workspace.resolve())
|
||||
# -P: this runner inherits the CLI's cwd, so launching from inside an
|
||||
# omnigent checkout must not shadow the installed package.
|
||||
assert captured["args"] == [sys.executable, "-P", "-m", "omnigent.runner._entry"]
|
||||
|
||||
|
||||
def test_start_cli_runner_process_binds_stable_local_runner_to_generated_token(
|
||||
|
||||
@@ -4147,3 +4147,21 @@ def test_zygote_start_failure_disables_it(
|
||||
assert host._zygote_disabled is True
|
||||
assert zygote.stop_calls == 0
|
||||
assert len(popen_argvs) == 1
|
||||
|
||||
|
||||
def test_direct_spawn_keeps_the_workspace_off_sys_path(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""The runner is spawned with ``-P`` so its workspace cannot shadow omnigent.
|
||||
|
||||
The runner's cwd is the session workspace, and ``python -m`` would otherwise
|
||||
prepend it to ``sys.path``, so a session opened *in an omnigent checkout*
|
||||
imports that checkout instead of the installed package, and a long-lived
|
||||
daemon plus a mid-flight ``git pull`` then skews the two.
|
||||
"""
|
||||
zygote = _FakeZygote(fail_at="start", running=False)
|
||||
|
||||
_host, popen_argvs = _spawn_with_fake_zygote(monkeypatch, tmp_path, zygote)
|
||||
|
||||
assert popen_argvs[0][1:] == ["-P", "-m", "omnigent.runner._entry"]
|
||||
|
||||
@@ -341,6 +341,30 @@ def test_operator_malloc_override_wins_at_the_zygote_exec(monkeypatch, tmp_path)
|
||||
assert captured["MALLOC_ARENA_MAX"] == "16"
|
||||
|
||||
|
||||
def test_zygote_boots_from_inside_an_omnigent_checkout(monkeypatch, tmp_path) -> None:
|
||||
"""A daemon whose cwd holds an ``omnigent/`` package still starts a zygote.
|
||||
|
||||
The zygote inherits the daemon's cwd, which ``python -m`` would prepend to
|
||||
``sys.path``, so a daemon started inside an omnigent checkout would import
|
||||
that checkout instead of the installed package. Booting against a poisoned
|
||||
package proves the spawn keeps cwd off ``sys.path``.
|
||||
|
||||
:param monkeypatch: Fixture used to run from the poisoned directory.
|
||||
:param tmp_path: Temp dir holding the poisoned package and the zygote log.
|
||||
"""
|
||||
package = tmp_path / "omnigent"
|
||||
package.mkdir()
|
||||
(package / "__init__.py").write_text('raise ImportError("poisoned omnigent")\n')
|
||||
monkeypatch.chdir(tmp_path)
|
||||
|
||||
mgr = ZygoteManager(log_path=tmp_path / "zygote.log")
|
||||
mgr.start()
|
||||
try:
|
||||
assert mgr.is_running()
|
||||
finally:
|
||||
mgr.stop()
|
||||
|
||||
|
||||
# ── Harness-fork path (fork_harness) ──────────────────────────────
|
||||
#
|
||||
# The zygote also forks HARNESS children on request, sharing the harness import
|
||||
|
||||
@@ -184,7 +184,12 @@ class TestSetupHermesHome:
|
||||
key: a headless Hermes agent had zero Omnigent tools."""
|
||||
home, bridge_dir = setup
|
||||
omnigent_mcp = json.loads((home / "config.yaml").read_text())["mcp_servers"]["omnigent"]
|
||||
assert omnigent_mcp["args"][:2] == ["-m", "omnigent.claude_native_bridge"]
|
||||
assert omnigent_mcp["args"][:4] == [
|
||||
"-I",
|
||||
"-m",
|
||||
"omnigent.claude_native_bridge",
|
||||
"serve-mcp",
|
||||
]
|
||||
assert "serve-mcp" in omnigent_mcp["args"]
|
||||
assert str(bridge_dir) in omnigent_mcp["args"] # serve-mcp --bridge-dir <bridge_dir>
|
||||
|
||||
|
||||
@@ -5,6 +5,7 @@ from __future__ import annotations
|
||||
import asyncio
|
||||
import contextlib
|
||||
import json
|
||||
import os
|
||||
from collections.abc import AsyncIterator
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
@@ -19,6 +20,27 @@ from omnigent.runner.mcp_manager import McpSchemasResult
|
||||
from omnigent.spec.types import AgentSpec, ExecutorSpec, MCPServerConfig
|
||||
from tests.runner.helpers import NullServerClient
|
||||
|
||||
# Project root: two parents up from this conftest (tests/runner/ → repo root).
|
||||
_PROJECT_ROOT = Path(__file__).resolve().parents[2]
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _ensure_subprocess_pythonpath(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Put the project root on ``PYTHONPATH`` for spawned harness children.
|
||||
|
||||
Harnesses are spawned with ``-P``, so the child's cwd is deliberately kept
|
||||
off ``sys.path`` (a workspace that is an omnigent checkout must not shadow
|
||||
the installed package). Tests that register a fixture harness module such as
|
||||
``tests._fixtures.runner_test_harness`` therefore have to hand the child that
|
||||
path through the environment, exactly as ``tests/runtime/harnesses/conftest``
|
||||
already does. Prepend rather than overwrite so a developer-set value stays.
|
||||
|
||||
:param monkeypatch: Pytest monkeypatch fixture, scoping this to one test.
|
||||
"""
|
||||
existing = os.environ.get("PYTHONPATH", "")
|
||||
new_path = f"{_PROJECT_ROOT}{os.pathsep}{existing}" if existing else str(_PROJECT_ROOT)
|
||||
monkeypatch.setenv("PYTHONPATH", new_path)
|
||||
|
||||
|
||||
def _drain_session_event_queue(queue: asyncio.Queue[Any] | None) -> list[dict[str, Any]]:
|
||||
"""
|
||||
|
||||
@@ -2167,6 +2167,39 @@ def test_main_configures_runner_process_logging(
|
||||
assert captured == {"destination": "runner", "force": True}
|
||||
|
||||
|
||||
def test_main_makes_the_workspace_importable(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""``main`` puts the workspace back on ``sys.path`` for local tools.
|
||||
|
||||
The runner is spawned with ``-P`` so its workspace cannot shadow the
|
||||
installed omnigent. Spec-declared local tools are still imported by dotted
|
||||
path, so the workspace has to be restored once omnigent itself is imported.
|
||||
|
||||
:param monkeypatch: Pytest monkeypatch fixture.
|
||||
:param tmp_path: Stands in for the session workspace.
|
||||
:returns: None.
|
||||
"""
|
||||
|
||||
async def _stop_immediately() -> None:
|
||||
"""Let ``main`` return once the path is set up.
|
||||
|
||||
:returns: None.
|
||||
"""
|
||||
|
||||
monkeypatch.setattr(
|
||||
"omnigent.runner._entry._run_tunnel_from_env",
|
||||
_stop_immediately,
|
||||
)
|
||||
monkeypatch.chdir(tmp_path)
|
||||
monkeypatch.setattr(sys, "path", [p for p in sys.path if p != str(tmp_path)])
|
||||
|
||||
main()
|
||||
|
||||
assert str(tmp_path) in sys.path
|
||||
|
||||
|
||||
def test_main_preserves_unexpected_runtime_errors(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
capsys: pytest.CaptureFixture[str],
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
@@ -12,6 +13,7 @@ from omnigent.codex_native_bridge import (
|
||||
clear_active_turn_id_if_matches,
|
||||
clear_bridge_state,
|
||||
codex_home_for_bridge_dir,
|
||||
codex_mcp_config_overrides,
|
||||
mcp_startup_waiting_detail,
|
||||
pending_mcp_servers,
|
||||
prepare_bridge_dir,
|
||||
@@ -29,6 +31,22 @@ from omnigent.codex_native_bridge import (
|
||||
)
|
||||
|
||||
|
||||
def test_codex_mcp_config_overrides_isolate_the_bridge_interpreter(tmp_path: Path) -> None:
|
||||
"""codex launches serve-mcp with ``-I`` so the workspace can't shadow omnigent.
|
||||
|
||||
The MCP server starts in the session workspace, and without ``-I`` python puts
|
||||
that cwd on ``sys.path``, so a workspace that is an omnigent checkout supplies
|
||||
the bridge's own package. Every other native bridge passes ``-I`` here.
|
||||
|
||||
:param tmp_path: Stands in for the per-session bridge dir.
|
||||
"""
|
||||
overrides = codex_mcp_config_overrides(tmp_path)
|
||||
|
||||
prefix = "mcp_servers.omnigent.args="
|
||||
raw = next(o[len(prefix) :] for o in overrides if o.startswith(prefix))
|
||||
assert json.loads(raw)[:4] == ["-I", "-m", "omnigent.claude_native_bridge", "serve-mcp"]
|
||||
|
||||
|
||||
def _seed_active_turn(bridge_dir: Path, active_turn_id: str | None) -> None:
|
||||
"""
|
||||
Write bridge state with a given active turn id.
|
||||
|
||||
Reference in New Issue
Block a user