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")