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 <syf2211@users.noreply.github.com> Co-authored-by: JerrettDavis <mxjerrett@gmail.com> Co-authored-by: JD Davis <jd@jds-macbook-air.tail2a279.ts.net>
This commit is contained in:
+42
-16
@@ -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:
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user