diff --git a/src/ucode/agents/codex.py b/src/ucode/agents/codex.py index 2077bee..7c3ef73 100644 --- a/src/ucode/agents/codex.py +++ b/src/ucode/agents/codex.py @@ -27,6 +27,8 @@ CODEX_PROFILE_NAME = "ucode" CODEX_CONFIG_PATH = CODEX_CONFIG_DIR / f"{CODEX_PROFILE_NAME}.config.toml" CODEX_BACKUP_PATH = APP_DIR / "codex-ucode-config.backup.toml" +LEGACY_CODEX_CONFIG_PATH = CODEX_CONFIG_DIR / "config.toml" +LEGACY_CODEX_BACKUP_PATH = APP_DIR / "codex-config.backup.toml" CODEX_MODEL_PROVIDER_NAME = "ucode-databricks" MINIMUM_CODEX_VERSION = (0, 134, 0) MINIMUM_CODEX_VERSION_TEXT = "0.134.0" @@ -47,6 +49,13 @@ ["model_providers", CODEX_MODEL_PROVIDER_NAME, "http_headers"], ] +LEGACY_MANAGED_KEYS: list[list[str]] = [ + ["profile"], + ["profiles", CODEX_PROFILE_NAME], + ["model_providers", CODEX_MODEL_PROVIDER_NAME], + ["model_providers", CODEX_MODEL_PROVIDER_NAME, "http_headers"], +] + def is_update_available() -> tuple[str, str] | None: return available_npm_package_update(SPEC["package"]) @@ -68,60 +77,72 @@ def _installed_version_status() -> tuple[str, bool] | None: return version, parsed < MINIMUM_CODEX_VERSION -def minimum_version_error() -> str | None: - status = _installed_version_status() - if status is None: - return None - version, is_too_old = status - if not is_too_old: - return None - return ( - f"Codex CLI {version} is too old for ucode's Codex profile config. " - f"Codex CLI must be updated to {MINIMUM_CODEX_VERSION_TEXT} or newer; " - f"run `npm install -g {SPEC['package']}` or `ucode configure`." - ) +def _use_legacy_layout() -> bool: + """Return True when the installed Codex CLI predates per-profile config files. + Codex 0.134.0 introduced support for `--profile ` resolving to + `~/.codex/.config.toml`. Older releases only honor a single + `~/.codex/config.toml` with `[profiles.]` sections. When the version + is unknown we keep the new layout (matches the prior "unknown does not + block" semantic). + """ + parsed = _parse_version(agent_version(SPEC["binary"])) + if parsed is None: + return False + return parsed < MINIMUM_CODEX_VERSION -def required_update_message() -> str | None: - status = _installed_version_status() - if status is None: - return None - version, is_too_old = status - if not is_too_old: - return None - return ( - f"Codex CLI {version} is older than required {MINIMUM_CODEX_VERSION_TEXT}; " - "updating Codex is required for ucode's Codex profile config." - ) + +def _provider_block(workspace: str, databricks_profile: str | None) -> dict: + auth_command = build_auth_shell_command(workspace, databricks_profile) + base_url = build_tool_base_url("codex", workspace) + return { + "name": "Databricks AI Gateway", + "base_url": base_url, + "wire_api": "responses", + "http_headers": { + "User-Agent": f"ucode/{ucode_version()} codex/{agent_version('codex')}", + }, + "auth": { + "command": "sh", + "args": ["-c", auth_command], + "timeout_ms": 5000, + "refresh_interval_ms": 900000, + }, + } def render_overlay( workspace: str, model: str | None = None, databricks_profile: str | None = None ) -> dict: - auth_command = build_auth_shell_command(workspace, databricks_profile) - base_url = build_tool_base_url("codex", workspace) overlay: dict = {"model_provider": CODEX_MODEL_PROVIDER_NAME} if model: overlay["model"] = model overlay["model_providers"] = { - CODEX_MODEL_PROVIDER_NAME: { - "name": "Databricks AI Gateway", - "base_url": base_url, - "wire_api": "responses", - "http_headers": { - "User-Agent": f"ucode/{ucode_version()} codex/{agent_version('codex')}", - }, - "auth": { - "command": "sh", - "args": ["-c", auth_command], - "timeout_ms": 5000, - "refresh_interval_ms": 900000, - }, - } + CODEX_MODEL_PROVIDER_NAME: _provider_block(workspace, databricks_profile), } return overlay +def render_legacy_overlay( + workspace: str, model: str | None = None, databricks_profile: str | None = None +) -> dict: + """Overlay for Codex CLI < 0.134.0, which only reads `~/.codex/config.toml`. + + The shared file uses `profile = "ucode"` to select `[profiles.ucode]`, which + points at the shared `[model_providers.ucode-databricks]` block. + """ + profile_block: dict = {"model_provider": CODEX_MODEL_PROVIDER_NAME} + if model: + profile_block["model"] = model + return { + "profile": CODEX_PROFILE_NAME, + "profiles": {CODEX_PROFILE_NAME: profile_block}, + "model_providers": { + CODEX_MODEL_PROVIDER_NAME: _provider_block(workspace, databricks_profile), + }, + } + + def _legacy_config_path() -> Path: return CODEX_CONFIG_PATH.parent / "config.toml" @@ -157,11 +178,27 @@ def _remove_legacy_ucode_profile() -> None: def write_tool_config(state: dict, model: str | None = None) -> dict: + workspace = state["workspace"] + chosen_model = model or default_model(state) + databricks_profile = state.get("profile") + + if _use_legacy_layout(): + # Codex < 0.134.0 only reads ~/.codex/config.toml. Write the shared + # config with [profiles.ucode] + shared [model_providers.ucode-databricks] + # and skip the per-profile-file cleanup that would normally strip + # ucode's entry from the shared file. + backup_existing_file(LEGACY_CODEX_CONFIG_PATH, LEGACY_CODEX_BACKUP_PATH) + overlay = render_legacy_overlay(workspace, chosen_model, databricks_profile) + doc = read_toml_safe(LEGACY_CODEX_CONFIG_PATH) + deep_merge_dict(doc, overlay) + write_toml_file(LEGACY_CODEX_CONFIG_PATH, doc) + state = mark_tool_managed(state, "codex", LEGACY_MANAGED_KEYS) + save_state(state) + return state + _remove_legacy_ucode_profile() backup_existing_file(CODEX_CONFIG_PATH, CODEX_BACKUP_PATH) - overlay = render_overlay( - state["workspace"], model or default_model(state), state.get("profile") - ) + overlay = render_overlay(workspace, chosen_model, databricks_profile) doc = read_toml_safe(CODEX_CONFIG_PATH) deep_merge_dict(doc, overlay) write_toml_file(CODEX_CONFIG_PATH, doc) diff --git a/tests/test_agent_codex.py b/tests/test_agent_codex.py index 45c709f..3f6ac35 100644 --- a/tests/test_agent_codex.py +++ b/tests/test_agent_codex.py @@ -81,6 +81,7 @@ def test_writes_ucode_profile_config_file(self, tmp_path, monkeypatch): backup_path = tmp_path / "codex-ucode-config.backup.toml" monkeypatch.setattr(codex, "CODEX_CONFIG_PATH", config_path) monkeypatch.setattr(codex, "CODEX_BACKUP_PATH", backup_path) + monkeypatch.setattr(codex, "agent_version", lambda binary: "0.134.0") monkeypatch.setattr(codex, "save_state", lambda state: None) codex.write_tool_config({"workspace": WS, "codex_models": ["gpt-5"]}) @@ -107,6 +108,7 @@ def test_removes_legacy_ucode_profile_from_shared_config(self, tmp_path, monkeyp legacy_backup_path = tmp_path / "codex-legacy-config.backup.toml" monkeypatch.setattr(codex, "CODEX_CONFIG_PATH", profile_path) monkeypatch.setattr(codex, "CODEX_BACKUP_PATH", backup_path) + monkeypatch.setattr(codex, "agent_version", lambda binary: "0.134.0") monkeypatch.setattr(codex, "save_state", lambda state: None) codex.write_tool_config({"workspace": WS, "codex_models": ["gpt-5"]}) @@ -117,25 +119,71 @@ def test_removes_legacy_ucode_profile_from_shared_config(self, tmp_path, monkeyp assert doc["profiles"]["other"]["model_provider"] == "keep" assert legacy_backup_path.exists() + def test_writes_legacy_shared_config_when_codex_too_old(self, tmp_path, monkeypatch): + config_dir = tmp_path / ".codex" + legacy_path = config_dir / "config.toml" + profile_path = config_dir / "ucode.config.toml" + backup_path = tmp_path / "codex-ucode-config.backup.toml" + legacy_backup_path = tmp_path / "codex-config.backup.toml" + monkeypatch.setattr(codex, "CODEX_CONFIG_PATH", profile_path) + monkeypatch.setattr(codex, "CODEX_BACKUP_PATH", backup_path) + monkeypatch.setattr(codex, "LEGACY_CODEX_CONFIG_PATH", legacy_path) + monkeypatch.setattr(codex, "LEGACY_CODEX_BACKUP_PATH", legacy_backup_path) + monkeypatch.setattr(codex, "agent_version", lambda binary: "0.133.0") + monkeypatch.setattr(codex, "save_state", lambda state: None) + + codex.write_tool_config({"workspace": WS, "codex_models": ["gpt-5"]}) + + # Per-profile file must not be written for old Codex. + assert not profile_path.exists() + doc = read_toml_safe(legacy_path) + assert doc["profile"] == "ucode" + assert doc["profiles"]["ucode"]["model_provider"] == "ucode-databricks" + assert doc["profiles"]["ucode"]["model"] == "gpt-5" + provider = doc["model_providers"]["ucode-databricks"] + assert provider["base_url"] == f"{WS}/ai-gateway/codex/v1" + assert provider["wire_api"] == "responses" + + def test_legacy_write_preserves_other_profiles_in_shared_config(self, tmp_path, monkeypatch): + config_dir = tmp_path / ".codex" + config_dir.mkdir() + legacy_path = config_dir / "config.toml" + legacy_path.write_text( + '[profiles.other]\nmodel_provider = "keep"\n', + encoding="utf-8", + ) + profile_path = config_dir / "ucode.config.toml" + backup_path = tmp_path / "codex-ucode-config.backup.toml" + legacy_backup_path = tmp_path / "codex-config.backup.toml" + monkeypatch.setattr(codex, "CODEX_CONFIG_PATH", profile_path) + monkeypatch.setattr(codex, "CODEX_BACKUP_PATH", backup_path) + monkeypatch.setattr(codex, "LEGACY_CODEX_CONFIG_PATH", legacy_path) + monkeypatch.setattr(codex, "LEGACY_CODEX_BACKUP_PATH", legacy_backup_path) + monkeypatch.setattr(codex, "agent_version", lambda binary: "0.133.0") + monkeypatch.setattr(codex, "save_state", lambda state: None) + + codex.write_tool_config({"workspace": WS, "codex_models": ["gpt-5"]}) + + doc = read_toml_safe(legacy_path) + assert doc["profiles"]["other"]["model_provider"] == "keep" + assert doc["profiles"]["ucode"]["model_provider"] == "ucode-databricks" + -class TestCodexMinimumVersion: - def test_no_error_when_codex_is_new_enough(self, monkeypatch): +class TestCodexLegacyLayoutDetection: + def test_new_codex_uses_modern_layout(self, monkeypatch): monkeypatch.setattr(codex, "agent_version", lambda binary: "0.134.0") - assert codex.minimum_version_error() is None - assert codex.required_update_message() is None + assert codex._use_legacy_layout() is False - def test_errors_when_codex_is_too_old(self, monkeypatch): + def test_old_codex_uses_legacy_layout(self, monkeypatch): monkeypatch.setattr(codex, "agent_version", lambda binary: "0.133.0") - assert "Codex CLI must be updated to 0.134.0 or newer" in codex.minimum_version_error() - assert "updating Codex is required" in codex.required_update_message() + assert codex._use_legacy_layout() is True - def test_unknown_version_does_not_block(self, monkeypatch): + def test_unknown_version_uses_modern_layout(self, monkeypatch): monkeypatch.setattr(codex, "agent_version", lambda binary: "unknown") - assert codex.minimum_version_error() is None - assert codex.required_update_message() is None + assert codex._use_legacy_layout() is False class TestCodexDefaultModel: diff --git a/tests/test_agents_init.py b/tests/test_agents_init.py index 25c17e6..15d1e36 100644 --- a/tests/test_agents_init.py +++ b/tests/test_agents_init.py @@ -298,47 +298,6 @@ def fake_run(*args, **kwargs): assert install_tool_binary("opencode", strict=True, update_existing=True) is True - def test_existing_old_codex_raises_clear_blocker(self, monkeypatch): - def fake_which(binary: str) -> str | None: - return f"/usr/bin/{binary}" - - message = "Codex CLI must be updated to 0.134.0 or newer" - monkeypatch.setattr("ucode.agents.shutil.which", fake_which) - monkeypatch.setattr("ucode.agents.codex.minimum_version_error", lambda: message) - monkeypatch.setattr("ucode.agents.codex.required_update_message", lambda: None) - - with pytest.raises(RuntimeError, match="Codex CLI must be updated"): - install_tool_binary("codex", strict=True, update_existing=False) - - def test_configure_updates_existing_old_codex_without_optional_prompt( - self, monkeypatch, capsys - ): - calls: list[list[str]] = [] - prompt_calls: list[str] = [] - - def fake_which(binary: str) -> str | None: - return f"/usr/bin/{binary}" - - def fake_run(args, **kwargs): - calls.append(args) - return subprocess.CompletedProcess(args, 0) - - monkeypatch.setattr("ucode.agents.shutil.which", fake_which) - monkeypatch.setattr("ucode.agents.subprocess.run", fake_run) - monkeypatch.setattr( - "ucode.agents.codex.required_update_message", - lambda: "Codex CLI 0.133.0 is older than required 0.134.0", - ) - monkeypatch.setattr("ucode.agents.codex.minimum_version_error", lambda: None) - monkeypatch.setattr( - "ucode.agents.prompt_yes_no", lambda prompt: prompt_calls.append(prompt) or False - ) - - assert install_tool_binary("codex", strict=False, update_existing=True) is True - assert calls == [["npm", "install", "-g", "@openai/codex"]] - assert prompt_calls == [] - assert "older than required" in capsys.readouterr().out - def test_ensure_tool_binary_available_raises_when_missing(self, monkeypatch): monkeypatch.setattr("ucode.agents.shutil.which", lambda _: None) diff --git a/tests/test_e2e.py b/tests/test_e2e.py index 8bb3b85..8302b65 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -93,6 +93,21 @@ def _run_gemini_gateway_smoke(workspace: str, model: str, token: str) -> str: return data.get("candidates", [{}])[0].get("content", {}).get("parts", [{}])[0].get("text", "") +E2E_EXCLUDED_MODEL_IDS = { + # Listed by AI Gateway model discovery, but currently rejected at launch + # with "The provided model identifier is invalid." + "databricks-claude-opus-4-8", +} + + +def _launchable_model_items(models: dict) -> list[tuple[str, str]]: + return [ + (family, model_id) + for family, model_id in models.items() + if model_id and model_id not in E2E_EXCLUDED_MODEL_IDS + ] + + # --------------------------------------------------------------------------- # Databricks auth / token # --------------------------------------------------------------------------- @@ -424,6 +439,9 @@ def test_launch_claude_per_model( claude_models: dict = e2e_state.get("claude_models") or {} if not claude_models: pytest.skip("No Claude models available on this workspace") + launchable_models = _launchable_model_items(claude_models) + if not launchable_models: + pytest.skip("No launchable Claude models available on this workspace") # Use an isolated config dir so the claude subprocess never reads or # writes ~/.claude/settings.json during this test. @@ -436,7 +454,7 @@ def test_launch_claude_per_model( base_url = build_tool_base_url("claude", e2e_workspace) failures = [] - for family, model_id in claude_models.items(): + for family, model_id in launchable_models: with pytest.MonkeyPatch().context() as mp: mp.setattr("ucode.state.save_state", lambda s: None) claude.write_tool_config({**e2e_state, "workspace": e2e_workspace}, model_id) @@ -619,9 +637,8 @@ def _all_models(self, e2e_state: dict) -> list[tuple[str, str]]: """Return [(family, model_id), ...] for every model copilot can talk to.""" out: list[tuple[str, str]] = [] claude_models: dict = e2e_state.get("claude_models") or {} - for family, model_id in claude_models.items(): - if model_id: - out.append((f"claude-{family}", model_id)) + for family, model_id in _launchable_model_items(claude_models): + out.append((f"claude-{family}", model_id)) for model in e2e_state.get("codex_models") or []: if any(frag in model for frag in self.COPILOT_INCOMPATIBLE_MODEL_FRAGMENTS): continue @@ -680,9 +697,8 @@ class TestPiLaunch: def _all_models(self, e2e_state: dict) -> list[tuple[str, str]]: out: list[tuple[str, str]] = [] claude_models: dict = e2e_state.get("claude_models") or {} - for family, model_id in claude_models.items(): - if model_id: - out.append((f"claude-{family}", model_id)) + for family, model_id in _launchable_model_items(claude_models): + out.append((f"claude-{family}", model_id)) for model in e2e_state.get("codex_models") or []: out.append(("codex", model)) for model in e2e_state.get("gemini_models") or []: