From 5d9bbbeea07c2e1ee1640cc455aaeb23d9f75768 Mon Sep 17 00:00:00 2001 From: nangsontay <143306990+nangsontay@users.noreply.github.com> Date: Tue, 14 Jul 2026 22:53:24 +0700 Subject: [PATCH] =?UTF-8?q?fix(codex):=20rewrite=20config.toml=20properly?= =?UTF-8?q?=20so=20Codex=20will=20route=20through=20=E2=80=A6=20(#2102)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- headroom/providers/codex/install.py | 53 +++++++++++++++++++++++----- tests/test_install/test_providers.py | 48 +++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 8 deletions(-) diff --git a/headroom/providers/codex/install.py b/headroom/providers/codex/install.py index 30059866b..2567d6636 100644 --- a/headroom/providers/codex/install.py +++ b/headroom/providers/codex/install.py @@ -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. diff --git a/tests/test_install/test_providers.py b/tests/test_install/test_providers.py index 6de73fa76..837847150 100644 --- a/tests/test_install/test_providers.py +++ b/tests/test_install/test_providers.py @@ -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")