From b7f342c153a3e6e43a9d3df006bcd4dd69842d00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=9F=B3=E5=B2=B3=E5=B3=B0?= <132282304+syf2211@users.noreply.github.com> Date: Thu, 13 Aug 2026 09:52:22 -0700 Subject: [PATCH] fix(wrap): verify proxy deps before mutating Codex config (#1628) ## Description \`headroom wrap codex\` now verifies that optional proxy dependencies (\`headroom-ai[proxy]\`) are installed before mutating Codex \`config.toml\`. If the check fails, the command exits with the same error message as \`headroom proxy\` and leaves Codex config untouched. Fixes #1614 (Bug 1: config mutated before proxy dependency check). ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Performance improvement - [ ] Code refactoring (no functional changes) ## Changes Made - Extract \`ensure_proxy_dependencies()\` in \`headroom/cli/proxy.py\` (shared with \`headroom proxy\`) - Call it at the start of \`wrap codex\` when \`not no_proxy\`, before config snapshot/injection - Add regression tests for prepare-only abort, \`--no-proxy\` skip, and import failure messaging ## Testing - [x] Unit tests pass (\`pytest\`) - [x] Linting passes (\`ruff check .\`) - [ ] Type checking passes (\`mypy headroom\`) - [x] New tests added for new functionality - [x] Manual testing performed ### Test Output \`\`\`bash pytest tests/test_cli/test_wrap_codex.py::test_wrap_codex_aborts_before_mutating_config_when_proxy_deps_missing \ tests/test_cli/test_wrap_codex.py::test_wrap_codex_skips_proxy_dependency_check_with_no_proxy \ tests/test_cli/test_wrap_codex.py::test_ensure_proxy_dependencies_exits_when_server_import_fails -q # 3 passed ruff check headroom/cli/wrap.py headroom/cli/proxy.py tests/test_cli/test_wrap_codex.py ruff format --check headroom/cli/wrap.py headroom/cli/proxy.py tests/test_cli/test_wrap_codex.py \`\`\` ## Real Behavior Proof Environment: Linux (Ubuntu), Python 3.12, local checkout with \`PYTHONPATH\` pointed at patched sources. Exact command / steps: 1. Created a temp \`~/.codex/config.toml\` with \`model_provider = "openai"\`. 2. Patched \`headroom.cli.wrap.ensure_proxy_dependencies\` to raise \`SystemExit(1)\` (simulating missing \`[proxy]\` extra). 3. Ran \`headroom wrap codex --prepare-only --no-serena --port 8787\`. Observed result: exit code 1; \`config.toml\` unchanged; no \`config.toml.headroom-backup\` created; no \`[mcp_servers.headroom]\` block written. Also verified: \`headroom wrap codex --prepare-only --no-proxy ...\` does not invoke the dependency check. Not tested: Windows-specific proxy selector behavior (covered separately in #1655). ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I did not edit CHANGELOG.md; release notes are generated automatically --------- Co-authored-by: syf2211 Co-authored-by: JerrettDavis Co-authored-by: JD Davis --- headroom/cli/proxy.py | 58 +++++++++++++++++------- headroom/cli/wrap.py | 4 ++ pyproject.toml | 1 + tests/conftest.py | 16 +++++++ tests/test_cli/test_wrap_bridge.py | 3 +- tests/test_cli/test_wrap_codex.py | 67 +++++++++++++++++++++++++++ tests/test_cli/test_wrap_opencode.py | 10 ++++ tests/test_cli_proxy_improvements.py | 68 +++++++++++++++++++--------- 8 files changed, 189 insertions(+), 38 deletions(-) diff --git a/headroom/cli/proxy.py b/headroom/cli/proxy.py index e9ba52686..73e2aee8a 100644 --- a/headroom/cli/proxy.py +++ b/headroom/cli/proxy.py @@ -4,6 +4,7 @@ import logging import os import sys import warnings +from importlib import import_module from typing import Any, Literal, cast import click @@ -18,6 +19,38 @@ from headroom.proxy.modes import PROXY_MODE_CACHE, normalize_proxy_mode from .main import main + +def ensure_proxy_dependencies() -> None: + """Verify optional proxy extras are installed before starting or wrapping.""" + required_modules: list[str] = [ + "fastapi", + "uvicorn", + "httpx", + "openai", + "mcp", + "magika", + "zstandard", + "websockets", + "onnxruntime", + "transformers", + "watchdog", + ] + if sys.implementation.name != "pypy": + required_modules.append("orjson") + + try: + for module in required_modules: + import_module(module) + except ImportError as e: + click.secho( + "Error: Proxy dependencies not installed. Run: pip install headroom-ai[proxy]", + fg="red", + err=True, + ) + click.secho(f"Details: {e}", fg="red", err=True) + raise SystemExit(1) from None + + # --------------------------------------------------------------------------- # Startup log suppression. # @@ -1032,23 +1065,16 @@ def proxy( Usage with OpenAI-compatible clients: OPENAI_BASE_URL=http://localhost:8787/v1 your-app """ + ensure_proxy_dependencies() + # Import here to avoid slow startup - try: - from headroom.proxy.server import ( - ProxyConfig, - _parse_csv_tools, - _parse_exclude_tools, - _parse_tool_profiles, - run_server, - ) - except ImportError as e: - click.secho( - "Error: Proxy dependencies not installed. Run: pip install headroom-ai[proxy]", - fg="red", - err=True, - ) - click.secho(f"Details: {e}", fg="red", err=True) - raise SystemExit(1) from None + from headroom.proxy.server import ( + ProxyConfig, + _parse_csv_tools, + _parse_exclude_tools, + _parse_tool_profiles, + run_server, + ) # Warn if --learn and --no-learn are both set (--no-learn wins, per docstring) if learn and no_learn: diff --git a/headroom/cli/wrap.py b/headroom/cli/wrap.py index ef4a4a0f8..0a240c2d0 100644 --- a/headroom/cli/wrap.py +++ b/headroom/cli/wrap.py @@ -59,6 +59,7 @@ from headroom._version import normalize_release_version as _normalize_release_ve from headroom.agent_savings import ( apply_agent_savings_env_defaults, ) +from headroom.cli.proxy import ensure_proxy_dependencies from headroom.copilot_auth import ( _API_TOKEN_ENV_VARS, _API_TOKEN_EXPIRES_AT_ENV_VAR, @@ -5717,6 +5718,9 @@ def _run_codex_wrap( codex_args: tuple, ) -> None: """Execute the Codex wrap flow against the durable Codex home.""" + if not no_proxy: + ensure_proxy_dependencies() + if prepare_only: _prepare_codex_wrap_state( port=port, diff --git a/pyproject.toml b/pyproject.toml index ecee0517d..dfd51a33a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -495,6 +495,7 @@ markers = [ "slow: slow tests (model loads, large fixtures)", "real_llm: tests that hit real LLM APIs; skipped unless explicitly enabled", "live: opt-in multi-turn tests that hit real upstream APIs; require provider keys", + "proxy_dependency_gate: exercises ensure_proxy_dependencies() without mocking", ] [tool.coverage.run] diff --git a/tests/conftest.py b/tests/conftest.py index 2a75bd260..9b202b6cb 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -22,6 +22,22 @@ from tests._skip_helpers import external_model_skip_reason # those up inside CliRunner, so assertions would see the developer's proxy # config instead of the test's. Scrub them so local runs match CI; tests # that need a value set it explicitly via monkeypatch or CliRunner env. +@pytest.fixture(autouse=True) +def _skip_proxy_dependency_gate_unless_exercised( + request: pytest.FixtureRequest, monkeypatch: pytest.MonkeyPatch +) -> None: + """Most CLI tests run without headroom-ai[proxy] extras installed.""" + if request.node.get_closest_marker("proxy_dependency_gate") is not None: + return + try: + from headroom.cli import proxy + except ModuleNotFoundError: + # Native-wrapper jobs intentionally install only pytest and exercise the + # installer scripts without importing Headroom's runtime dependencies. + return + monkeypatch.setattr(proxy, "ensure_proxy_dependencies", lambda: None) + + @pytest.fixture(autouse=True) def _scrub_developer_headroom_env(monkeypatch): for key in list(os.environ): diff --git a/tests/test_cli/test_wrap_bridge.py b/tests/test_cli/test_wrap_bridge.py index a948c4df8..c6c98172d 100644 --- a/tests/test_cli/test_wrap_bridge.py +++ b/tests/test_cli/test_wrap_bridge.py @@ -42,7 +42,8 @@ def test_wrap_codex_prepare_only_updates_config(monkeypatch, tmp_path: Path) -> _set_test_home(monkeypatch, tmp_path) runner = CliRunner() - result = runner.invoke(main, ["wrap", "codex", "--prepare-only", "--port", "8787"]) + with patch("headroom.cli.wrap.ensure_proxy_dependencies", return_value=None): + result = runner.invoke(main, ["wrap", "codex", "--prepare-only", "--port", "8787"]) assert result.exit_code == 0, result.output config_file = tmp_path / ".codex" / "config.toml" diff --git a/tests/test_cli/test_wrap_codex.py b/tests/test_cli/test_wrap_codex.py index 38da91c73..bbade27ab 100644 --- a/tests/test_cli/test_wrap_codex.py +++ b/tests/test_cli/test_wrap_codex.py @@ -40,6 +40,24 @@ def runner() -> CliRunner: return CliRunner() +_PROXY_DEP_TESTS = frozenset( + { + "test_wrap_codex_aborts_before_mutating_config_when_proxy_deps_missing", + "test_wrap_codex_skips_proxy_dependency_check_with_no_proxy", + } +) + + +@pytest.fixture(autouse=True) +def _skip_wrap_proxy_dependency_gate_unless_exercised( + request: pytest.FixtureRequest, monkeypatch: pytest.MonkeyPatch +) -> None: + """Wrap-codex integration tests run in the base CI env without [proxy] extras.""" + if request.node.name in _PROXY_DEP_TESTS: + return + monkeypatch.setattr("headroom.cli.wrap.ensure_proxy_dependencies", lambda: None) + + # --------------------------------------------------------------------------- # Unit tests: helpers operating on ~/.codex/config.toml # --------------------------------------------------------------------------- @@ -990,6 +1008,55 @@ def test_wrap_codex_prepare_only_creates_backup_and_config( assert backup.read_text(encoding="utf-8") == original +def test_wrap_codex_aborts_before_mutating_config_when_proxy_deps_missing( + runner: CliRunner, monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + _set_test_home(monkeypatch, tmp_path) + config_file = tmp_path / ".codex" / "config.toml" + config_file.parent.mkdir(parents=True) + original = 'model_provider = "openai"\n' + config_file.write_text(original, encoding="utf-8") + + with patch("headroom.cli.wrap.ensure_proxy_dependencies", side_effect=SystemExit(1)): + result = runner.invoke( + main, + ["wrap", "codex", "--prepare-only", "--no-serena", "--port", "8787"], + ) + + assert result.exit_code == 1, result.output + assert config_file.read_text(encoding="utf-8") == original + assert "[mcp_servers.headroom]" not in config_file.read_text(encoding="utf-8") + assert not (tmp_path / ".codex" / "config.toml.headroom-backup").exists() + + +def test_wrap_codex_skips_proxy_dependency_check_with_no_proxy( + runner: CliRunner, monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + _set_test_home(monkeypatch, tmp_path) + config_file = tmp_path / ".codex" / "config.toml" + config_file.parent.mkdir(parents=True) + config_file.write_text('model_provider = "openai"\n', encoding="utf-8") + + with patch( + "headroom.cli.wrap.ensure_proxy_dependencies", + side_effect=AssertionError("should not run with --no-proxy"), + ): + result = runner.invoke( + main, + [ + "wrap", + "codex", + "--prepare-only", + "--no-proxy", + "--no-serena", + "--port", + "8787", + ], + ) + + assert result.exit_code == 0, result.output + + def test_wrap_codex_registers_mcp_when_codex_home_does_not_exist_yet( runner: CliRunner, monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: diff --git a/tests/test_cli/test_wrap_opencode.py b/tests/test_cli/test_wrap_opencode.py index 55914fcef..ee0bea009 100644 --- a/tests/test_cli/test_wrap_opencode.py +++ b/tests/test_cli/test_wrap_opencode.py @@ -20,6 +20,16 @@ def _no_retired_context_tool_env(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.delenv("HEADROOM_CONTEXT_TOOL", raising=False) +@pytest.fixture(autouse=True) +def _mock_ensure_proxy(monkeypatch: pytest.MonkeyPatch) -> None: + """Wrap-opencode tests should not spawn a real proxy subprocess in CI.""" + + def fake_ensure_proxy(port: int, no_proxy: bool, **kwargs): # noqa: ANN002, ANN003 + return None, port + + monkeypatch.setattr(wrap_mod, "_ensure_proxy", fake_ensure_proxy) + + @pytest.fixture def runner() -> CliRunner: return CliRunner() diff --git a/tests/test_cli_proxy_improvements.py b/tests/test_cli_proxy_improvements.py index e400ae35b..aa5d58eb8 100644 --- a/tests/test_cli_proxy_improvements.py +++ b/tests/test_cli_proxy_improvements.py @@ -299,32 +299,58 @@ class TestMemoryTopKValidation: class TestMissingProxyDepsError: """When proxy dependencies are absent the CLI should print an actionable error and exit 1.""" - def test_import_error_exits_nonzero(self, runner: CliRunner) -> None: - with patch.dict( - "sys.modules", - {"headroom.proxy.server": None}, + @pytest.mark.proxy_dependency_gate + def test_proxy_command_exits_when_mcp_missing( + self, runner: CliRunner, monkeypatch: pytest.MonkeyPatch + ) -> None: + import builtins + + real_import = builtins.__import__ + + def fake_import( + name: str, + globals: dict | None = None, + locals: dict | None = None, + fromlist: tuple = (), + level: int = 0, ): - result = runner.invoke(main, ["proxy"]) - # Click CliRunner may raise SystemExit or catch it; exit code must be non-zero - assert result.exit_code != 0 + if name == "mcp": + raise ImportError("No module named 'mcp'") + return real_import(name, globals, locals, fromlist, level) - def test_import_error_message_is_actionable(self, runner: CliRunner) -> None: - """The error message should tell the user how to fix the problem.""" - original_import = ( - __builtins__.__import__ if hasattr(__builtins__, "__import__") else __import__ - ) + monkeypatch.setattr(builtins, "__import__", fake_import) + result = runner.invoke(main, ["proxy"]) + assert result.exit_code == 1, result.output + assert "pip install headroom-ai[proxy]" in result.output + assert "No module named 'mcp'" in result.output - def patched_import(name, *args, **kwargs): - if name == "headroom.proxy.server": - raise ImportError("No module named 'headroom.proxy.server'") - return original_import(name, *args, **kwargs) + @pytest.mark.proxy_dependency_gate + def test_ensure_proxy_dependencies_exits_when_fastapi_missing( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + import builtins - with patch("builtins.__import__", side_effect=patched_import): - result = runner.invoke(main, ["proxy"]) + from headroom.cli.proxy import ensure_proxy_dependencies - # Either exit code 1 or output with actionable guidance - # (some test environments may shadow the import differently) - assert result.exit_code != 0 or "proxy" in result.output.lower() + real_import = builtins.__import__ + + def fake_import( + name: str, + globals: dict | None = None, + locals: dict | None = None, + fromlist: tuple = (), + level: int = 0, + ): + if name == "fastapi": + raise ImportError("No module named 'fastapi'") + return real_import(name, globals, locals, fromlist, level) + + monkeypatch.setattr(builtins, "__import__", fake_import) + + with pytest.raises(SystemExit) as exc_info: + ensure_proxy_dependencies() + + assert exc_info.value.code == 1 class TestKeyboardInterruptExitCode: