mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(codex): rewrite config.toml properly so Codex will route through … (#2102)
## Description `headroom install apply --providers manual --target codex --scope provider` silently failed to route Codex through the proxy whenever `~/.codex/config.toml` already had a `[table]` section (e.g. `[features]`, `[mcp_servers.*]`). `apply_provider_scope` appended the managed `model_provider = "headroom"` block after the last existing table, so TOML scoped the bare key into that table instead of the document root — Codex silently ignored it and kept routing through its default provider. The same code path never overrode a pre-existing top-level `model_provider` assignment either, so a user's `model_provider = "openai"` kept winning even when Headroom's block was appended elsewhere in the file. This mirrors a bug already fixed in the `headroom init` path (`_ensure_codex_provider`, #260) that was never ported to the persistent-install path. Closes: reported via user session (no tracked issue number yet). ## Type of Change - [x] Bug fix (non-breaking change which fixes an issue) ## Changes Made - **`headroom/providers/codex/install.py`**: Added `_insert_block_at_root()`, which walks the document line-by-line and inserts the managed marker block immediately above the first `[table]`/`[[array-of-tables]]` header, falling back to end-of-file append only when no table exists. Mirrors the root-insertion logic already used by `cli/init.py:_ensure_codex_provider`. - Added `_ANY_MODEL_PROVIDER` / `_ANY_OPENAI_BASE_URL` patterns (match any value, not just `"headroom"`) so `apply_provider_scope` strips **any** prior top-level `model_provider` / `openai_base_url` assignment before re-inserting the managed block — the managed keys now override the user's config outright instead of losing to it. - `apply_provider_scope` merge order is now: strip old managed block → strip prior top-level assignments → insert fresh block at document root. ## Testing - [x] **New regression test**: `test_apply_codex_provider_scope_lands_model_provider_at_root` (`tests/test_install/test_providers.py`) — asserts `model_provider = "headroom"` lands before `[features]`, overrides a prior `"openai"` value, and the user's own table content survives. - [x] **Existing tests**: `tests/test_install/test_providers.py` — 42/42 pass (includes prior codex apply/revert/replace/orphan-cleanup coverage). - [x] **Adversarial (ad-hoc, not committed)**: 6-case TOML round-trip proof — parses output with `tomllib` (not substring matching) across: prior provider before a table, no prior provider, empty file, scalars-only (no tables), CRLF line endings, multiple tables. All 6 pass after the fix; first pass caught a false failure from a stale globally pip-installed `headroom` copy shadowing the repo source when tests run outside the project directory — re-verified from inside the repo to confirm the fix itself is correct. - [x] **Lint**: `ruff check` and `ruff format --check` pass on both changed files. ```text $ uv run pytest tests/test_install/test_providers.py -q 42 passed in 0.21s $ uv run --with ruff ruff check headroom/providers/codex/install.py tests/test_install/test_providers.py All checks passed! $ uv run --with ruff ruff format --check headroom/providers/codex/install.py tests/test_install/test_providers.py 2 files already formatted ``` ## Real Behavior Proof - Environment: macOS 26.4.1 (arm64), Python 3.13.14, headroom branch `patch/install-codex` - Exact command / steps: constructed a temp `config.toml` with `[features]\nweb_search = true` (no existing Headroom block), invoked `apply_provider_scope(manifest)` against it with `codex_config_path` patched to the temp file, then parsed the result with `tomllib.loads()`. - Observed result: before the fix, `tomllib.loads(result)["model_provider"]` raised `KeyError` — the key was nested inside `[features]` due to end-of-file append. After the fix, `parsed["model_provider"] == "headroom"` and `parsed["features"]["web_search"] is True` — both the managed key and the user's table are present and correctly scoped. Revert removes `model_provider` and preserves the user's table. - Tested local build and behavior is correct as expected of this patch. ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review
This commit is contained in:
parent
5fb449e90b
commit
5d9bbbeea0
2 changed files with 93 additions and 8 deletions
|
|
@ -31,6 +31,10 @@ _ORPHAN_HEADROOM_TABLE = re.compile(
|
|||
r"(?=^\[|\Z)"
|
||||
)
|
||||
|
||||
_TOML_TABLE_HEADER_RE = re.compile(r"^[ \t]*(?:\[\[[^\]\r\n]+\]\]|\[[^\]\r\n]+\])[ \t]*(?:#.*)?$")
|
||||
_ROOT_MODEL_PROVIDER_RE = re.compile(r"^[ \t]*model_provider[ \t]*=")
|
||||
_ROOT_OPENAI_BASE_URL_RE = re.compile(r"^[ \t]*openai_base_url[ \t]*=")
|
||||
|
||||
|
||||
def codex_uses_chatgpt_auth(auth_path: Path) -> bool:
|
||||
"""Whether Codex authenticated via ChatGPT OAuth (vs an OpenAI API key).
|
||||
|
|
@ -92,6 +96,41 @@ def build_install_env(*, port: int, backend: str) -> dict[str, str]:
|
|||
return {"OPENAI_BASE_URL": proxy_base_url(port)}
|
||||
|
||||
|
||||
def _insert_block_at_root(content: str, block: str) -> str:
|
||||
"""Place a marker block carrying top-level keys above the first TOML table.
|
||||
|
||||
Codex scopes bare keys under the preceding ``[table]`` header, so a
|
||||
``model_provider`` appended after a table (e.g. ``[features]``) is silently
|
||||
ignored and routing never switches (#260). Land the block at the document
|
||||
root instead.
|
||||
"""
|
||||
block = block.strip()
|
||||
lines = content.splitlines()
|
||||
for index, line in enumerate(lines):
|
||||
if _TOML_TABLE_HEADER_RE.search(line):
|
||||
head = "\n".join(lines[:index]).rstrip()
|
||||
tail = "\n".join(lines[index:]).lstrip("\n")
|
||||
prefix = f"{head}\n\n" if head else ""
|
||||
return (f"{prefix}{block}\n\n{tail}").rstrip() + "\n"
|
||||
return (content.rstrip() + "\n\n" + block + "\n").lstrip()
|
||||
|
||||
|
||||
def _strip_root_provider_assignments(content: str) -> str:
|
||||
"""Remove root provider assignments without touching table-scoped settings."""
|
||||
lines = content.splitlines(keepends=True)
|
||||
kept: list[str] = []
|
||||
in_root = True
|
||||
for line in lines:
|
||||
if in_root and _TOML_TABLE_HEADER_RE.search(line):
|
||||
in_root = False
|
||||
if in_root and (
|
||||
_ROOT_MODEL_PROVIDER_RE.match(line) or _ROOT_OPENAI_BASE_URL_RE.match(line)
|
||||
):
|
||||
continue
|
||||
kept.append(line)
|
||||
return "".join(kept)
|
||||
|
||||
|
||||
def apply_provider_scope(manifest: DeploymentManifest) -> ManagedMutation | None:
|
||||
"""Apply Codex provider-scope configuration when requested."""
|
||||
if manifest.scope != ConfigScope.PROVIDER.value:
|
||||
|
|
@ -111,14 +150,12 @@ def apply_provider_scope(manifest: DeploymentManifest) -> ManagedMutation | None
|
|||
)
|
||||
+ f"{_CODEX_MARKER_END}\n"
|
||||
)
|
||||
if path.exists():
|
||||
existing = path.read_text(encoding="utf-8")
|
||||
if _CODEX_MARKER_START in existing:
|
||||
merged = _CODEX_PATTERN.sub(section, existing)
|
||||
else:
|
||||
merged = existing.rstrip() + "\n\n" + section + "\n"
|
||||
else:
|
||||
merged = section + "\n"
|
||||
existing = path.read_text(encoding="utf-8") if path.exists() else ""
|
||||
# Drop our previous block and any prior top-level provider assignment so the
|
||||
# managed keys override the user's, then land them at the document root.
|
||||
existing = _CODEX_PATTERN.sub("", existing)
|
||||
existing = _strip_root_provider_assignments(existing)
|
||||
merged = _insert_block_at_root(existing, section)
|
||||
path.write_text(merged, encoding="utf-8")
|
||||
# Pull existing native threads into the headroom-provider menu so Codex's
|
||||
# history list stays whole once it routes through Headroom. Best-effort.
|
||||
|
|
|
|||
|
|
@ -100,6 +100,54 @@ def test_apply_codex_provider_scope_emits_flag_for_chatgpt_auth(
|
|||
assert "requires_openai_auth = true" in config_path.read_text()
|
||||
|
||||
|
||||
def test_apply_codex_provider_scope_lands_model_provider_at_root(
|
||||
monkeypatch, tmp_path: Path
|
||||
) -> None:
|
||||
"""model_provider must sit above the first [table] and override any prior value.
|
||||
|
||||
Codex scopes bare keys under the preceding table header, so appending the
|
||||
provider block after an existing [table] (e.g. [features]) leaves
|
||||
model_provider ignored and routing never switches (#260).
|
||||
"""
|
||||
config_path = tmp_path / "config.toml"
|
||||
config_path.write_text('model_provider = "openai"\n\n[features]\nweb_search = true\n')
|
||||
monkeypatch.setattr("headroom.providers.codex.install.codex_config_path", lambda: config_path)
|
||||
manifest = _manifest(tmp_path)
|
||||
|
||||
apply_codex_provider_scope(manifest)
|
||||
|
||||
content = config_path.read_text()
|
||||
# Prior "openai" assignment is overridden, not duplicated.
|
||||
assert content.count("model_provider =") == 1
|
||||
assert 'model_provider = "headroom"' in content
|
||||
# The managed key must land before the first table header.
|
||||
assert content.index('model_provider = "headroom"') < content.index("[features]")
|
||||
# The user's own table survives.
|
||||
assert "web_search = true" in content
|
||||
|
||||
|
||||
def test_apply_codex_provider_scope_preserves_table_scoped_provider_keys(
|
||||
monkeypatch, tmp_path: Path
|
||||
) -> None:
|
||||
config_path = tmp_path / "config.toml"
|
||||
config_path.write_text(
|
||||
'model_provider = "openai"\n\n'
|
||||
"[profiles.work]\n"
|
||||
'model_provider = "native"\n'
|
||||
'openai_base_url = "https://example.invalid/v1"\n'
|
||||
)
|
||||
monkeypatch.setattr("headroom.providers.codex.install.codex_config_path", lambda: config_path)
|
||||
manifest = _manifest(tmp_path)
|
||||
|
||||
apply_codex_provider_scope(manifest)
|
||||
|
||||
content = config_path.read_text()
|
||||
assert content.count('model_provider = "headroom"') == 1
|
||||
assert 'model_provider = "openai"' not in content
|
||||
assert 'model_provider = "native"' in content
|
||||
assert 'openai_base_url = "https://example.invalid/v1"' in content
|
||||
|
||||
|
||||
def test_codex_build_install_env_returns_proxy_base_url() -> None:
|
||||
env = build_codex_install_env(port=5566, backend="ignored")
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue