diff --git a/AGENTS.md b/AGENTS.md index d64e75a0dc..07ca9b1c52 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -86,7 +86,7 @@ All pull requests must comply with [`.agents/skills/custom-codereview-guide.md`] - `LookupSecret` normalizes hostless URLs against `OH_INTERNAL_SERVER_URL` (set by `openhands-agent-server.__main__` from the bound host/port, rewriting wildcard binds to loopback) and otherwise falls back to `http://127.0.0.1:8000`, so relative secret URLs can safely target the current agent-server instance. - read_only ACP session-mode confirmation (`_apply_acp_session_mode`) waits at most `_ACP_SESSION_MODE_CONFIRM_TIMEOUT` (5s) on a per-session `asyncio.Event` in `_OpenHandsACPBridge.wait_for_current_mode` when `set_mode` returned with no observed mode and the advertised current mode differs; a different observed mode or a timeout still raises `ACPSessionModeError` (fail-closed, no provider/CLI version conditions). `_verify_read_only_mode_after_config` is deliberately unchanged (sync, generation-based, post-config), so the wait applies only to the initial `session/set_mode` confirmation. -- `.aidw/validation.json` (schema `aidw.repository-validation.v1`, only command `make validate`) is the canonical validation the ai-dev runtime VERIFY step trusts from the baseline SHA; changing it in a candidate yields `HUMAN_REQUIRED`, so it must land on main first. `make validate` unsets `CLAUDE_CODE_OAUTH_TOKEN`/`CLAUDE_AUTH_CODE` (the workflow environment exports them and they make `TestACPEnvConflictSuppression` fail) and the agent-server variables `OPENHANDS_CODEX_AUTH_SOURCE`, `OPENHANDS_AGENT_SERVER_CONFIG_PATH`, `OH_PERSISTENCE_DIR`, `OH_SECRETS_DIR`, `CLOUD_AGENT_INJECTED_SECRET_NAMES`, `OH_INTERNAL_SERVER_URL` first (Builders run it in an agent-server shell; otherwise tests read host Codex auth and write into the real `secrets.json`), via `VALIDATE_UNSET_ENV` in the Makefile; `tests/conftest.py` `pytest_configure` strips the agent-server ones for direct `uv run pytest` too (tests that need them must `monkeypatch.setenv`), then runs `uv sync --dev --frozen`, then the `tests/sdk`, `tests/agent_server`, `tests/workspace`, `tests/cross` and `tests/tools` suites as separate pytest invocations (one combined run is order-dependent under xdist), `pyright`, `pre-commit run --all-files`, `git diff --check` and `git diff --exit-code`. `tests/examples` and `tests/integration` are deliberately excluded because they need a live LLM (not part of the `tests.yml` unit jobs either); the DNS-dependent `test_openai_chat_completions_gateway_over_real_server` is deselected. A cold-worktree run took 16m11s (limit 31 min). The `.github/workflows/validate.yml` job named exactly `validate` runs `make validate` on `pull_request`; the merge coordinator matches the Actions job name, so do not rename it or give it a display `name:`. `make validate` must not modify tracked files, `uv.lock`, or the ACP version pins. +- `.aidw/validation.json` (schema `aidw.repository-validation.v1`, only command `make validate`) is the canonical validation the ai-dev runtime VERIFY step trusts from the baseline SHA; changing it in a candidate yields `HUMAN_REQUIRED`, so it must land on main first. `make validate` runs its steps (`make validate-steps`) through `scripts/validate-env.sh`, i.e. under `env -i` with an allowlist only (`PATH`, `LANG`, `LC_*`, `CI=true`, `UV_CACHE_DIR`/`UV_PYTHON_INSTALL_DIR`/`PRE_COMMIT_HOME` defaulting to the host's cache locations, and a throwaway `HOME`/`TMPDIR` created with `mktemp -d` and removed afterwards), so no inherited variable (`OPENHANDS_*`, `OH_*`, `CLAUDE_*`, `OPENAI_*`, `CODEX_*`, `CURSOR_*`, `GH_TOKEN`/`GITHUB_*`, `CLOUD_AGENT_*`, or any future name) reaches the suites (Builders run it in an agent-server shell; otherwise tests read host Codex auth, write into the real `secrets.json` and see `OPENHANDS_SESSION_API_KEY`); there is no denylist (`VALIDATE_UNSET_ENV` and the `tests/conftest.py` env strip were removed), so direct `uv run pytest` in a polluted shell is not isolated — use `make validate`; tests that need a variable `monkeypatch.setenv` it. `validate-steps` runs `uv sync --dev --frozen`, then the `tests/sdk`, `tests/agent_server`, `tests/workspace`, `tests/cross` and `tests/tools` suites as separate pytest invocations (one combined run is order-dependent under xdist), `pyright`, `pre-commit run --all-files`, `git diff --check` and `git diff --exit-code`. `tests/examples` and `tests/integration` are deliberately excluded because they need a live LLM (not part of the `tests.yml` unit jobs either); the DNS-dependent `test_openai_chat_completions_gateway_over_real_server` is deselected. A cold-worktree run took 16m11s (limit 31 min). The `.github/workflows/validate.yml` job named exactly `validate` runs `make validate` on `pull_request`; the merge coordinator matches the Actions job name, so do not rename it or give it a display `name:`. `make validate` must not modify tracked files, `uv.lock`, or the ACP version pins. Acceptance decision: direct-pytest isolation is intentionally not claimed or kept (the single allowlist lives in `scripts/validate-env.sh`); `TERM`, `SHELL`, `USER` and `VIRTUAL_ENV` are not passed because no suite needs them. diff --git a/Makefile b/Makefile index eae030f572..14181cda07 100644 --- a/Makefile +++ b/Makefile @@ -57,18 +57,16 @@ pre-commit: @$(ECHO) "$(GREEN)Pre-commit run successfully.$(RESET)" # Canonical validation for the ai-dev runtime VERIFY step and the `validate` CI job. -# The OAuth variables make TestACPEnvConflictSuppression fail, and the -# agent-server variables point tests at the host's real persistence, secrets, -# Codex auth and config. All of them are unset first; tests/conftest.py strips -# the agent-server ones again for direct pytest runs. +# `validate` re-runs `validate-steps` through scripts/validate-env.sh, which starts +# it under `env -i` with an allowlisted environment and a throwaway HOME/TMPDIR, +# so no inherited credential or agent-server variable reaches the suites. # Scope: tests/sdk, tools, workspace, agent_server and cross, as in tests.yml. # tests/examples and tests/integration need a live LLM and are excluded. -VALIDATE_UNSET_ENV := CLAUDE_CODE_OAUTH_TOKEN CLAUDE_AUTH_CODE \ - OPENHANDS_CODEX_AUTH_SOURCE OPENHANDS_AGENT_SERVER_CONFIG_PATH \ - OH_PERSISTENCE_DIR OH_SECRETS_DIR CLOUD_AGENT_INJECTED_SECRET_NAMES \ - OH_INTERNAL_SERVER_URL validate: - @unset $(VALIDATE_UNSET_ENV); \ + @sh scripts/validate-env.sh make --no-print-directory validate-steps + +.PHONY: validate-steps +validate-steps: uv sync --dev --frozen && \ CI=true uv run python -m pytest -n auto -p no:cacheprovider tests/sdk && \ CI=true uv run python -m pytest -n auto -p no:cacheprovider tests/agent_server && \ diff --git a/scripts/validate-env.sh b/scripts/validate-env.sh new file mode 100755 index 0000000000..4231ca6ca9 --- /dev/null +++ b/scripts/validate-env.sh @@ -0,0 +1,27 @@ +#!/bin/sh +# Runs "$@" in a clean environment (env -i) holding only the allowlist below +# and throwaway HOME/TMPDIR directories. Nothing else is inherited. +set -eu + +cache_home="${XDG_CACHE_HOME:-$HOME/.cache}" +data_home="${XDG_DATA_HOME:-$HOME/.local/share}" +sandbox="$(mktemp -d "${TMPDIR:-/tmp}/oh-validate.XXXXXX")" +trap 'rm -rf "$sandbox"' EXIT +mkdir "$sandbox/home" "$sandbox/tmp" + +# LANG and LC_* are the only allowlisted names matched by prefix. +locale_vars="$(env | sed -n 's/^\(LC_[A-Za-z_]*=.*\)$/\1/p' | tr '\n' ' ')" + +# $locale_vars is deliberately unquoted: one word per NAME=value pair. +# shellcheck disable=SC2086 +env -i \ + PATH="$PATH" \ + LANG="${LANG:-C.UTF-8}" \ + $locale_vars \ + HOME="$sandbox/home" \ + TMPDIR="$sandbox/tmp" \ + UV_CACHE_DIR="${UV_CACHE_DIR:-$cache_home/uv}" \ + UV_PYTHON_INSTALL_DIR="${UV_PYTHON_INSTALL_DIR:-$data_home/uv/python}" \ + PRE_COMMIT_HOME="${PRE_COMMIT_HOME:-$cache_home/pre-commit}" \ + CI=true \ + "$@" diff --git a/tests/conftest.py b/tests/conftest.py index 9df7bcce14..fe7dc99b7a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,6 +1,5 @@ """Common test fixtures and utilities.""" -import os import uuid from pathlib import Path from unittest.mock import MagicMock @@ -23,29 +22,6 @@ ) -# Inherited from an agent-server shell, these point tests at the host's real -# persistence, secrets, Codex auth and server config. -AGENT_SERVER_ENV_VARS = ( - "OPENHANDS_CODEX_AUTH_SOURCE", - "OPENHANDS_AGENT_SERVER_CONFIG_PATH", - "OH_PERSISTENCE_DIR", - "OH_SECRETS_DIR", - "CLOUD_AGENT_INJECTED_SECRET_NAMES", - "OH_INTERNAL_SERVER_URL", -) - - -def strip_agent_server_env() -> None: - for name in AGENT_SERVER_ENV_VARS: - os.environ.pop(name, None) - - -def pytest_configure(config: pytest.Config) -> None: - # Before collection and before xdist spawns workers, so no fixture can - # capture a host value and restore it on teardown. - strip_agent_server_env() - - def pytest_addoption(parser: pytest.Parser) -> None: group = parser.getgroup("examples") group.addoption( diff --git a/tests/sdk/test_env_isolation.py b/tests/sdk/test_env_isolation.py index a171b82873..a7d7533562 100644 --- a/tests/sdk/test_env_isolation.py +++ b/tests/sdk/test_env_isolation.py @@ -1,59 +1,78 @@ -"""The test suite must never inherit the agent-server's host-side env.""" +"""`make validate` must run in an allowlisted environment, not inherit the parent's.""" import os import subprocess -import sys from pathlib import Path -from tests.conftest import AGENT_SERVER_ENV_VARS, REPO_ROOT, strip_agent_server_env +from tests.conftest import REPO_ROOT -ISOLATED_ENV_VARS = ( - "CLAUDE_CODE_OAUTH_TOKEN", - "CLAUDE_AUTH_CODE", - *AGENT_SERVER_ENV_VARS, -) +SENTINEL = "dummy-sentinel-do-not-leak" +POLLUTED_ENV = { + "OPENHANDS_SESSION_API_KEY": SENTINEL, + "OPENHANDS_CODEX_AUTH_SOURCE": SENTINEL, + "OH_PERSISTENCE_DIR": SENTINEL, + "OH_SECRETS_DIR": SENTINEL, + "CLAUDE_CODE_OAUTH_TOKEN": SENTINEL, + "OPENAI_API_KEY": SENTINEL, + "CODEX_HOME": SENTINEL, + "CURSOR_API_KEY": SENTINEL, + "GH_TOKEN": SENTINEL, + "GITHUB_TOKEN": SENTINEL, + "CLOUD_AGENT_INJECTED_SECRET_NAMES": SENTINEL, + "SOME_UNLISTED_SECRET": SENTINEL, +} +ALLOWED_NAMES = { + "PATH", + "LANG", + "HOME", + "TMPDIR", + "UV_CACHE_DIR", + "UV_PYTHON_INSTALL_DIR", + "PRE_COMMIT_HOME", + "CI", +} -def test_strip_agent_server_env_removes_inherited_values(monkeypatch): - for name in AGENT_SERVER_ENV_VARS: - monkeypatch.setenv(name, "/dummy") - strip_agent_server_env() - assert [n for n in AGENT_SERVER_ENV_VARS if n in os.environ] == [] - - -def test_pytest_run_strips_inherited_env(tmp_path: Path): - probe = tmp_path / "test_probe.py" - probe.write_text( - "import os\n" - f"NAMES = {AGENT_SERVER_ENV_VARS!r}\n" - "def test_probe():\n" - " assert [n for n in NAMES if n in os.environ] == []\n" - ) - env = {**os.environ, **{name: "/dummy" for name in AGENT_SERVER_ENV_VARS}} - result = subprocess.run( - [ - sys.executable, - "-m", - "pytest", - "-q", - "-p", - "no:cacheprovider", - "-p", - "tests.conftest", - "--rootdir", - str(tmp_path), - str(probe), - ], +def run_in_validate_env( + tmp_path: Path, *command: str +) -> subprocess.CompletedProcess[str]: + env = { + **os.environ, + **POLLUTED_ENV, + "LC_ALL": "C.UTF-8", + "TMPDIR": str(tmp_path), + "HOME": str(tmp_path / "real-home"), + } + return subprocess.run( + ["sh", "scripts/validate-env.sh", *command], cwd=REPO_ROOT, env=env, capture_output=True, text=True, + check=True, ) - assert result.returncode == 0, result.stdout + result.stderr -def test_make_validate_unsets_isolated_env(): +def test_validate_env_contains_only_allowlisted_names(tmp_path: Path): + result = run_in_validate_env(tmp_path, "env") + names = {line.split("=", 1)[0] for line in result.stdout.splitlines()} + locale_names = {name for name in names if name.startswith("LC_")} + assert names <= ALLOWED_NAMES | locale_names + assert {"PATH", "HOME", "TMPDIR", "CI", "LC_ALL"} <= names + assert SENTINEL not in result.stdout + result.stderr + + +def test_validate_env_uses_throwaway_home_and_tmpdir(tmp_path: Path): + result = run_in_validate_env(tmp_path, "sh", "-c", 'echo "$HOME" "$TMPDIR"') + home, tmpdir = (Path(p) for p in result.stdout.split()) + assert home != tmp_path / "real-home" + assert home.parent == tmpdir.parent + assert home.parent.parent == tmp_path + assert not home.parent.exists() + + +def test_make_validate_runs_steps_in_validate_env(): result = subprocess.run( ["make", "-n", "validate"], cwd=REPO_ROOT, @@ -61,9 +80,5 @@ def test_make_validate_unsets_isolated_env(): text=True, check=True, ) - unset_line = next( - line for line in result.stdout.splitlines() if line.startswith("unset ") - ) - assert set(unset_line.removeprefix("unset ").split(";")[0].split()) == set( - ISOLATED_ENV_VARS - ) + assert result.stdout.startswith("sh scripts/validate-env.sh ") + assert "validate-steps" in result.stdout