From 569a890dc37c5f4e3741618fc5d5e2dbbe955e20 Mon Sep 17 00:00:00 2001 From: Andy Xu Date: Mon, 21 Sep 2026 20:10:34 +0000 Subject: [PATCH 1/5] Expose Unity Gateway model catalogs to Codex App --- README.md | 10 +- src/ucode/agents/codex.py | 78 ++++++++++- tests/README.md | 2 +- tests/conftest.py | 4 + tests/integration/README.md | 5 +- .../test_ug_codex_managed_model_discovery.py | 33 ++++- tests/integration/utils/harness.py | 8 +- tests/test_agent_codex.py | 122 ++++++++++++++++++ 8 files changed, 248 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 94fcacf80..bce55cc09 100644 --- a/README.md +++ b/README.md @@ -56,6 +56,14 @@ models for Claude Code's `/model` picker. Discovery defaults to `system.ai` when no provider or model location is selected. Use `--provider` or `--model-location` to select another model source; managed workspace configs control their own sources. +When `ug codex` discovers a model catalog, it also refreshes +`~/.ucode/codex-model-catalog.json` and points the shared `~/.codex/config.toml` +at it. Managed static model lists use the same path. Restart Codex App to load +the latest list; the app's provider and authentication must already be configured +for the corresponding gateway. The most recently refreshed workspace supplies +the app catalog. An existing custom catalog setting is preserved, and `ug revert` +removes the shared catalog reference installed by ug. + ## Configure ```bash @@ -166,7 +174,7 @@ with `ug configure` to control installation. | Tool | Managed files | |------|---------------| -| Codex | `~/.codex/ucode.config.toml`, legacy `~/.codex/config.toml`, `/etc/codex/managed_config.toml` (Linux and macOS) | +| Codex | `~/.codex/ucode.config.toml`, shared catalog reference in `~/.codex/config.toml`, `~/.ucode/codex-model-catalog.json`, `/etc/codex/managed_config.toml` (Linux and macOS) | | Claude Code | `~/.claude/ucode-settings.json`, `~/.claude.json`, `/etc/claude-code/managed-settings.json` (Linux), `/Library/Application Support/ClaudeCode/managed-settings.json` (macOS) | | Gemini CLI | `~/.gemini/ucode.env`, `~/.ucode/.gemini-home/.gemini/settings.json` | | OpenCode | `~/.ucode/opencode-xdg/opencode/opencode.json`, `~/.ucode/opencode-xdg/opencode/plugin/ucode-auth.js` | diff --git a/src/ucode/agents/codex.py b/src/ucode/agents/codex.py index 704a3adce..e2b509723 100644 --- a/src/ucode/agents/codex.py +++ b/src/ucode/agents/codex.py @@ -359,9 +359,14 @@ def revert_legacy_shared_config() -> bool: through the workspace gateway. ``ucode revert`` only restored the per-profile file, leaving those edits in place. Surgically strip them here. + Also remove the shared app catalog reference installed by modern ucode. Returns True if anything was removed. """ - return _strip_legacy_ucode_entries(_legacy_config_path()) + legacy_changed = _strip_legacy_ucode_entries(_legacy_config_path()) + app_catalog_changed = _remove_app_catalog_reference() + if app_catalog_changed and CODEX_MODEL_CATALOG_PATH.exists(): + CODEX_MODEL_CATALOG_PATH.unlink() + return legacy_changed or app_catalog_changed def configured_paths(state: dict) -> list[str]: @@ -475,9 +480,11 @@ def compose(base: dict, *, include_catalog: bool = True) -> dict: return base if catalog is not None: - write_json_file(CODEX_MODEL_CATALOG_PATH, catalog) - elif CODEX_MODEL_CATALOG_PATH.exists() and not is_dry_run(): - CODEX_MODEL_CATALOG_PATH.unlink() + _sync_app_model_catalog(catalog) + elif not is_dry_run(): + _remove_app_catalog_reference() + if CODEX_MODEL_CATALOG_PATH.exists(): + CODEX_MODEL_CATALOG_PATH.unlink() doc = read_toml_safe(CODEX_CONFIG_PATH) compose(doc) @@ -771,6 +778,9 @@ def _model_catalog_path(workspace: str, scope: str) -> Path: def _write_model_catalog(path: Path, catalog: dict) -> None: + if is_dry_run(): + write_json_file(path, catalog) + return temp_path = None try: path.parent.mkdir(parents=True, exist_ok=True) @@ -791,6 +801,63 @@ def _write_model_catalog(path: Path, catalog: dict) -> None: pass +def _is_ucode_catalog_reference(value: object) -> bool: + return isinstance(value, str) and Path(value).expanduser() == CODEX_MODEL_CATALOG_PATH + + +def _read_app_config() -> tomlkit.TOMLDocument: + path = _legacy_config_path() + try: + return tomlkit.parse(path.read_text(encoding="utf-8")) + except FileNotFoundError: + return tomlkit.document() + except (OSError, UnicodeError, ParseError) as exc: + raise RuntimeError(f"Cannot update Codex App settings at {path}: {exc}") from exc + + +def _install_app_catalog_reference() -> bool: + """Point Codex App at ucode's stable catalog without replacing a user catalog.""" + if is_dry_run(): + return False + path = _legacy_config_path() + doc = _read_app_config() + existing = doc.get("model_catalog_json") + if existing is not None and not _is_ucode_catalog_reference(existing): + print_warning_err( + f"Codex App already uses the custom model catalog {existing}; leaving it unchanged." + ) + return False + catalog_path = str(CODEX_MODEL_CATALOG_PATH) + if existing == catalog_path: + return False + doc["model_catalog_json"] = catalog_path + write_toml_file(path, doc) + return True + + +def _remove_app_catalog_reference() -> bool: + """Remove only a shared catalog reference owned by ucode.""" + if is_dry_run(): + return False + path = _legacy_config_path() + if not path.exists(): + return False + doc = _read_app_config() + if not _is_ucode_catalog_reference(doc.get("model_catalog_json")): + return False + doc.pop("model_catalog_json", None) + write_toml_file(path, doc) + return True + + +def _sync_app_model_catalog(catalog: dict) -> None: + """Refresh the stable catalog that Codex App loads from shared config.""" + if is_dry_run(): + return + _write_model_catalog(CODEX_MODEL_CATALOG_PATH, catalog) + _install_app_catalog_reference() + + def _launch_token(state: dict, workspace: str, *, force_refresh: bool = False) -> str: """The token Codex authenticates with: a custom-OAuth client token when configured, else the CLI profile. The single auth-selection point — the OTLP proxy's token @@ -980,10 +1047,11 @@ def launch( identifier=catalog_identifier, ) except CodexMpsModelCatalogUnavailable: - pass + _remove_app_catalog_reference() else: catalog_path = _model_catalog_path(workspace, catalog_scope) _write_model_catalog(catalog_path, catalog) + _sync_app_model_catalog(catalog) profile_doc["model_catalog_json"] = str(catalog_path) # Codex otherwise boots on its bundled default model (e.g. gpt-5.6-sol), # which an MPS's allowlist doesn't route, so the first request 403s. Pin diff --git a/tests/README.md b/tests/README.md index f45a8a831..effe17f1f 100644 --- a/tests/README.md +++ b/tests/README.md @@ -57,7 +57,7 @@ All tests live directly in `integration/`; shared mechanics live in `utils/`. | `test_case_01_*`, `test_case_03_*` | Launch managed Claude after configure and from fresh state, with personal discovery enabled and disabled | Claude receives the admin MPS header, caches native discovery results, and opens its real model picker | | `test_case_05_*`, `test_case_07_*` | Pass a provider or model-location override to managed Claude after configure and from fresh state | ug rejects the override before Claude starts and preserves agent-owned state | | `test_case_09_*`, `test_case_11_*` | Disable discovery and pass a provider or model-location override to managed Claude | ug still rejects both configured and fresh launches | -| `test_case_02_*`, `test_case_04_*` | Launch managed Codex after configure and from fresh state, with personal discovery enabled and disabled | Codex exposes exactly the admin MPS-scoped catalog | +| `test_case_02_*`, `test_case_04_*` | Launch managed Codex after configure and from fresh state, with personal discovery enabled and disabled | ug-launched and fresh bare Codex app servers expose exactly the admin MPS-scoped catalog through the stable catalog and shared config; GUI rendering is not covered | | `test_case_06_*`, `test_case_08_*` | Pass a provider or model-location override to managed Codex after configure and from fresh state | ug rejects the override before Codex starts and preserves agent-owned state | | `test_case_10_*`, `test_case_12_*` | Disable discovery and pass a provider or model-location override to managed Codex | ug still rejects both configured and fresh launches | | `test_ug_configure_managed_codex_catalog_fallback` | Configure from an injected managed response containing a GPT model absent from Codex's bundled catalog | Actionable metadata warning; conservative catalog entry for the unknown model; real Codex prompt on the valid default model | diff --git a/tests/conftest.py b/tests/conftest.py index a1a7419ff..467906ee8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -50,6 +50,10 @@ def _isolate_ucode_state(tmp_path, monkeypatch): managed_files_mod, "MANAGED_BACKUP_MANIFEST_PATH", backup_dir / "manifest.json" ) monkeypatch.setattr(codex_mod, "codex_managed_config_path", lambda: None) + monkeypatch.setattr( + codex_mod, "CODEX_MODEL_CATALOG_PATH", state_dir / "codex-model-catalog.json" + ) + monkeypatch.setattr(codex_mod, "CODEX_CONFIG_PATH", tmp_path / ".codex" / "ucode.config.toml") def reject_privileged_write(path, _desired_text): pytest.fail( diff --git a/tests/integration/README.md b/tests/integration/README.md index 2ac6a4ddf..07b8eebca 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -340,7 +340,10 @@ module fetches the workspace's published config once, replaces Claude's static m across all configured/fresh scenarios. The Codex module does the same with `main.default.ci_e2e_openai_mps`. The tests verify Claude's admin header, native cache and real model picker, Codex's exact app-server catalog, and both agents' rejection of personal source -overrides. Codex state comparisons exclude `.codex/tmp/arg0`, the disposable executable links +overrides. Codex discovery also refreshes a stable catalog and shared-config reference, then a +fresh bare `codex app-server` must return the same models without ug launch overrides. This +checks desktop startup configuration, not GUI rendering or inference through the desktop app. +Codex state comparisons exclude `.codex/tmp/arg0`, the disposable executable links recreated by version checks, while continuing to compare persistent agent files. In addition, `test_ug_configure_managed_codex_catalog_fallback` injects the intentionally nonexistent `system.ai.gpt-99`, keeping it out of the real workspace while launching Codex through that diff --git a/tests/integration/test_ug_codex_managed_model_discovery.py b/tests/integration/test_ug_codex_managed_model_discovery.py index b9690160a..72d1a75bd 100644 --- a/tests/integration/test_ug_codex_managed_model_discovery.py +++ b/tests/integration/test_ug_codex_managed_model_discovery.py @@ -69,6 +69,10 @@ def _assert_managed_provider_catalog(session, models): # in Codex's generated profile. assert "model_catalog_json" not in config, config + shared_config = tomllib.loads((session.home / ".codex" / "config.toml").read_text()) + app_catalog_path = session.home / ".ucode" / "codex-model-catalog.json" + assert shared_config.get("model_catalog_json") == str(app_catalog_path), shared_config + managed_cache = json.loads((session.home / ".ucode/managed-config.json").read_text()) raw_config = managed_cache.get("config") enabled_agents = raw_config.get("enabled_agents") if isinstance(raw_config, dict) else None @@ -89,6 +93,7 @@ def _assert_managed_provider_catalog(session, models): catalog_paths = list((session.home / ".ucode").glob("codex-model-catalog-*.json")) assert len(catalog_paths) == 1, catalog_paths catalog = json.loads(catalog_paths[0].read_text()) + assert json.loads(app_catalog_path.read_text()) == catalog catalog_ids = [ model.get("slug") for model in catalog.get("models", []) @@ -101,7 +106,8 @@ def _assert_managed_provider_catalog(session, models): def test_case_02_managed_codex_uses_admin_discovery_after_configure(live_session, workspace): """Scenario: configure managed Codex, then launch its app server. - Expected: Codex exposes exactly the admin-managed model catalog. + Expected: ug-launched and fresh bare Codex app servers expose exactly the admin-managed + model catalog. This verifies the desktop startup configuration, not GUI rendering. """ session = live_session configured = session.run( @@ -117,12 +123,17 @@ def test_case_02_managed_codex_uses_admin_discovery_after_configure(live_session models = session.codex_model_ids(["app-server", "--listen", "stdio://"]) _assert_managed_provider_catalog(session, models) + app_models = session.codex_model_ids( + ["app-server", "--listen", "stdio://"], name="bare-codex-models", binary="codex" + ) + assert app_models == models, (app_models, models) def test_case_02_managed_codex_uses_admin_discovery_from_fresh_state(live_session, workspace): """Scenario: launch managed Codex with --workspace from fresh state. - Expected: Codex exposes exactly the admin-managed model catalog. + Expected: ug-launched and fresh bare Codex app servers expose exactly the admin-managed + model catalog. This verifies the desktop startup configuration, not GUI rendering. """ session = live_session models = session.codex_model_ids( @@ -130,12 +141,17 @@ def test_case_02_managed_codex_uses_admin_discovery_from_fresh_state(live_sessio ) _assert_managed_provider_catalog(session, models) + app_models = session.codex_model_ids( + ["app-server", "--listen", "stdio://"], name="bare-codex-models", binary="codex" + ) + assert app_models == models, (app_models, models) def test_case_04_managed_codex_ignores_discovery_disable_after_configure(live_session, workspace): """Scenario: configure managed Codex, disable discovery, then launch its app server. - Expected: workspace-managed discovery still supplies the admin's catalog. + Expected: workspace-managed discovery supplies the admin's catalog to ug-launched and + fresh bare Codex app servers, even with the personal discovery flag disabled. """ session = live_session configured = session.run( @@ -152,12 +168,17 @@ def test_case_04_managed_codex_ignores_discovery_disable_after_configure(live_se models = session.codex_model_ids(["app-server", "--listen", "stdio://"]) _assert_managed_provider_catalog(session, models) + app_models = session.codex_model_ids( + ["app-server", "--listen", "stdio://"], name="bare-codex-models", binary="codex" + ) + assert app_models == models, (app_models, models) def test_case_04_managed_codex_ignores_discovery_disable_from_fresh_state(live_session, workspace): """Scenario: disable discovery and launch managed Codex with --workspace from fresh state. - Expected: workspace-managed discovery still supplies the admin's catalog. + Expected: workspace-managed discovery supplies the admin's catalog to ug-launched and + fresh bare Codex app servers, even with the personal discovery flag disabled. """ session = live_session session.env["UG_ENABLE_MODEL_DISCOVERY"] = "0" @@ -167,6 +188,10 @@ def test_case_04_managed_codex_ignores_discovery_disable_from_fresh_state(live_s ) _assert_managed_provider_catalog(session, models) + app_models = session.codex_model_ids( + ["app-server", "--listen", "stdio://"], name="bare-codex-models", binary="codex" + ) + assert app_models == models, (app_models, models) def test_case_06_managed_codex_rejects_provider_override_after_configure( diff --git a/tests/integration/utils/harness.py b/tests/integration/utils/harness.py index b72827f69..2f6ad5574 100644 --- a/tests/integration/utils/harness.py +++ b/tests/integration/utils/harness.py @@ -198,9 +198,10 @@ def app_server_handshake( timeout: int = 120, request: tuple[str, dict] | None = None, name: str = "app-server", + binary: str | None = None, ) -> dict: """Speak the real Codex stdio protocol and require an initialize response.""" - command = [str(self.binary), "codex", *args] + command = [binary, *args] if binary else [str(self.binary), "codex", *args] messages: queue.Queue = queue.Queue() transcript: list[str] = [] diagnostics: list[str] = [] @@ -287,7 +288,9 @@ def wait_for_response(request_id, description): f"{name}.json", {"argv": command, "stdout": transcript, "stderr": diagnostics} ) - def codex_model_ids(self, args: list[str], name: str = "codex-models") -> list[str]: + def codex_model_ids( + self, args: list[str], name: str = "codex-models", *, binary: str | None = None + ) -> list[str]: """Ask the real Codex app-server for the catalog its model picker uses.""" response = self.app_server_handshake( args, @@ -296,6 +299,7 @@ def codex_model_ids(self, args: list[str], name: str = "codex-models") -> list[s {"cursor": None, "limit": 1000, "includeHidden": False}, ), name=f"{name}-app-server", + binary=binary, ) result = response["result"] models = result.get("data") diff --git a/tests/test_agent_codex.py b/tests/test_agent_codex.py index 008cd09e2..b2d96b9df 100644 --- a/tests/test_agent_codex.py +++ b/tests/test_agent_codex.py @@ -696,6 +696,90 @@ def test_returns_false_when_no_shared_config(self, tmp_path, monkeypatch): assert codex.revert_legacy_shared_config() is False + def test_strips_ucode_app_catalog_reference(self, tmp_path, monkeypatch): + config_dir = tmp_path / ".codex" + config_dir.mkdir() + profile_path = config_dir / "ucode.config.toml" + shared_path = config_dir / "config.toml" + catalog_path = tmp_path / ".ucode" / "codex-model-catalog.json" + catalog_path.parent.mkdir(exist_ok=True) + catalog_path.write_text("{}", encoding="utf-8") + shared_path.write_text( + f'model_catalog_json = "{catalog_path}"\npersonality = "friendly"\n', + encoding="utf-8", + ) + monkeypatch.setattr(codex, "CODEX_CONFIG_PATH", profile_path) + monkeypatch.setattr(codex, "CODEX_MODEL_CATALOG_PATH", catalog_path) + + assert codex.revert_legacy_shared_config() is True + + assert read_toml_safe(shared_path) == {"personality": "friendly"} + assert not catalog_path.exists() + + +class TestCodexAppCatalog: + def test_refresh_keeps_stable_pointer_and_preserves_settings(self, tmp_path): + shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" + shared_path.parent.mkdir() + shared_path.write_text('# User settings\nmodel = "gpt-user"\n', encoding="utf-8") + first_catalog = {"models": [{"slug": "first-model"}]} + second_catalog = {"models": [{"slug": "second-model"}]} + + codex._sync_app_model_catalog(first_catalog) + first_config = shared_path.read_text() + codex._sync_app_model_catalog(second_catalog) + + assert shared_path.read_text() == first_config + assert "# User settings" in first_config + assert read_toml_safe(shared_path) == { + "model": "gpt-user", + "model_catalog_json": str(codex.CODEX_MODEL_CATALOG_PATH), + } + assert json.loads(codex.CODEX_MODEL_CATALOG_PATH.read_text()) == second_catalog + + def test_invalid_shared_config_is_not_overwritten(self): + shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" + shared_path.parent.mkdir() + original = 'model = "unfinished\n' + shared_path.write_text(original, encoding="utf-8") + + with pytest.raises(RuntimeError, match="Cannot update Codex App settings"): + codex._sync_app_model_catalog({"models": [{"slug": "gpt-mps"}]}) + + assert shared_path.read_text() == original + + def test_dry_run_leaves_config_and_catalog_unchanged(self, monkeypatch): + codex._sync_app_model_catalog({"models": [{"slug": "previous"}]}) + shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" + original = shared_path.read_bytes() + catalog_before = codex.CODEX_MODEL_CATALOG_PATH.read_bytes() + monkeypatch.setattr(codex, "is_dry_run", lambda: True) + + codex._sync_app_model_catalog({"models": [{"slug": "new"}]}) + + assert shared_path.read_bytes() == original + assert codex.CODEX_MODEL_CATALOG_PATH.read_bytes() == catalog_before + + def test_reconfigure_clears_discovered_app_catalog(self, monkeypatch): + codex._sync_app_model_catalog({"models": [{"slug": "previous-workspace"}]}) + monkeypatch.setattr(codex, "agent_version", lambda _: "0.154.0") + monkeypatch.setattr(codex, "save_state", lambda _: None) + + codex.write_tool_config({"workspace": WS}) + + shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" + assert "model_catalog_json" not in read_toml_safe(shared_path) + assert not codex.CODEX_MODEL_CATALOG_PATH.exists() + + def test_revert_preserves_subsequent_user_catalog(self, tmp_path): + codex._sync_app_model_catalog({"models": [{"slug": "gpt-mps"}]}) + shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" + original = 'model_catalog_json = "/user/models.json"\n' + shared_path.write_text(original, encoding="utf-8") + + assert codex.revert_legacy_shared_config() is False + assert shared_path.read_text() == original + class TestCodexDefaultModel: @pytest.fixture(autouse=True) @@ -790,8 +874,10 @@ def test_sets_oauth_token(self, tmp_path, monkeypatch): def test_provider_discovery_uses_authoritative_catalog(self, tmp_path, monkeypatch): launches = self._patch(tmp_path, monkeypatch) catalog_path = tmp_path / "models.json" + app_catalog_path = tmp_path / "codex-model-catalog.json" catalog = {"models": [{"slug": "gpt-mps"}]} fetch_kwargs = {} + monkeypatch.setattr(codex, "CODEX_MODEL_CATALOG_PATH", app_catalog_path) monkeypatch.setattr(codex, "_model_catalog_path", lambda workspace, provider: catalog_path) monkeypatch.setattr( codex, @@ -806,6 +892,10 @@ def test_provider_discovery_uses_authoritative_catalog(self, tmp_path, monkeypat ) assert catalog_path.exists() + assert json.loads(app_catalog_path.read_text()) == catalog + assert read_toml_safe(tmp_path / "config.toml")["model_catalog_json"] == str( + app_catalog_path + ) assert f'model_catalog_json="{catalog_path}"' in launches[0] assert fetch_kwargs == { "source": codex.CodexCatalogSource.PROVIDER, @@ -819,6 +909,30 @@ def test_provider_discovery_uses_authoritative_catalog(self, tmp_path, monkeypat ) assert 'Databricks-Model-Provider-Service = "main.default.openai"' in provider_arg + def test_provider_discovery_preserves_custom_app_catalog(self, tmp_path, monkeypatch, capsys): + launches = self._patch(tmp_path, monkeypatch) + catalog_path = tmp_path / "models.json" + app_catalog_path = tmp_path / "codex-model-catalog.json" + shared_path = tmp_path / "config.toml" + shared_path.write_text('model_catalog_json = "/user/models.json"\n', encoding="utf-8") + monkeypatch.setattr(codex, "CODEX_MODEL_CATALOG_PATH", app_catalog_path) + monkeypatch.setattr(codex, "_model_catalog_path", lambda workspace, scope: catalog_path) + monkeypatch.setattr( + codex, + "_fetch_codex_model_catalog", + lambda workspace, token, **kwargs: {"models": [{"slug": "gpt-mps"}]}, + ) + + codex.launch( + {"workspace": WS, "_codex_launch_provider": "main.default.openai"}, + [], + options=LaunchOptions(), + ) + + assert launches + assert read_toml_safe(shared_path)["model_catalog_json"] == "/user/models.json" + assert "leaving it unchanged" in " ".join(capsys.readouterr().err.split()) + def test_provider_pins_first_catalog_model(self, tmp_path, monkeypatch): launches = self._patch(tmp_path, monkeypatch) catalog = {"models": [{"slug": "gpt-primary"}, {"slug": "gpt-secondary"}]} @@ -973,12 +1087,19 @@ def fetch(workspace, token, **kwargs): ] assert fetched == ["main.first", "main.second"] assert catalog_args[0] != catalog_args[1] + assert json.loads(codex.CODEX_MODEL_CATALOG_PATH.read_text()) == { + "models": [{"slug": "main.second"}] + } + assert read_toml_safe(tmp_path / "config.toml")["model_catalog_json"] == str( + codex.CODEX_MODEL_CATALOG_PATH + ) @pytest.mark.parametrize("tool_args", [[], ["--model", "gpt-mps"]]) def test_provider_launches_when_discovery_is_unavailable( self, tmp_path, monkeypatch, tool_args ): launches = self._patch(tmp_path, monkeypatch) + codex._sync_app_model_catalog({"models": [{"slug": "previous-workspace"}]}) monkeypatch.setattr( codex, "_fetch_codex_model_catalog", @@ -999,6 +1120,7 @@ def test_provider_launches_when_discovery_is_unavailable( if tool_args: assert launches[0][-len(tool_args) :] == tool_args assert not any(arg.startswith("model_catalog_json=") for arg in launches[0]) + assert "model_catalog_json" not in read_toml_safe(tmp_path / "config.toml") provider_arg = next( arg for arg in launches[0] if arg.startswith("model_providers.Databricks=") ) From ca609705e69ad36ecb8d439684d60f6397323293 Mon Sep 17 00:00:00 2001 From: Andy Xu Date: Mon, 21 Sep 2026 22:03:10 +0000 Subject: [PATCH 2/5] Validate shared Codex catalogs and detach them before upgrades --- README.md | 14 +- src/ucode/agents/__init__.py | 9 + src/ucode/agents/codex.py | 58 +++++-- src/ucode/agents/codex_catalog.py | 87 ++++++---- tests/README.md | 4 +- tests/integration/README.md | 14 +- .../test_ug_codex_managed_model_discovery.py | 21 ++- .../integration/test_ug_configure_managed.py | 17 +- tests/test_agent_codex.py | 160 ++++++++++++++++++ tests/test_agents_init.py | 100 +++++++++++ tests/test_codex_catalog.py | 70 ++++++++ 11 files changed, 501 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index bce55cc09..2a29cea49 100644 --- a/README.md +++ b/README.md @@ -56,14 +56,24 @@ models for Claude Code's `/model` picker. Discovery defaults to `system.ai` when no provider or model location is selected. Use `--provider` or `--model-location` to select another model source; managed workspace configs control their own sources. -When `ug codex` discovers a model catalog, it also refreshes +When `ug codex` discovers a model catalog, it validates it with the installed +Codex binary, then refreshes `~/.ucode/codex-model-catalog.json` and points the shared `~/.codex/config.toml` at it. Managed static model lists use the same path. Restart Codex App to load the latest list; the app's provider and authentication must already be configured for the corresponding gateway. The most recently refreshed workspace supplies -the app catalog. An existing custom catalog setting is preserved, and `ug revert` +the app catalog. Existing custom catalog settings (including Isaac's catalog) +and custom providers are preserved; catalogs are not combined. `ug revert` removes the shared catalog reference installed by ug. +Before installing or updating Codex, ug detaches its shared catalog reference. +Run `ug codex` again to refresh discovery, or rerun `ug configure` for a managed +static list, before restarting or reconnecting the app. Do the same after +updating Codex outside ug. A failed catalog validation removes ug's shared +reference and reports an error rather than publishing an incompatible catalog. +Validation uses the Codex binary on this host; it does not verify a desktop +app's separate bundled binary on another machine. + ## Configure ```bash diff --git a/src/ucode/agents/__init__.py b/src/ucode/agents/__init__.py index a70d93c38..5a62d9108 100644 --- a/src/ucode/agents/__init__.py +++ b/src/ucode/agents/__init__.py @@ -134,6 +134,11 @@ def _update_installed_tool_binary(tool: str, version: str | None = None) -> bool command = ["npm", "install", "-g", target] print_note(f"Upgrading {spec['display']}...") + if tool == "codex": + # Codex's update command and npm replacement can invalidate the metadata + # referenced by the desktop app. Detach before mutating the binary and + # leave the reference detached until a later validated catalog publish. + codex.detach_app_model_catalog() try: subprocess.run(command, check=True, timeout=300) except (OSError, subprocess.CalledProcessError, subprocess.TimeoutExpired): @@ -224,6 +229,10 @@ def install_tool_binary( print_section("Bootstrap") print_warning(f"`{binary}` was not found. Installing {spec['display']}...") + if tool == "codex": + # The newly installed binary may not understand the previous catalog. + # Detachment must happen before npm mutates the installation. + codex.detach_app_model_catalog() try: subprocess.run(["npm", "install", "-g", package], check=True, timeout=300) except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as exc: diff --git a/src/ucode/agents/codex.py b/src/ucode/agents/codex.py index d6835c105..65609bc79 100644 --- a/src/ucode/agents/codex.py +++ b/src/ucode/agents/codex.py @@ -82,7 +82,7 @@ from ucode.ui import print_warning_err from .args import LaunchOptions -from .codex_catalog import prepare_codex_catalog +from .codex_catalog import prepare_codex_catalog, validate_codex_catalog CODEX_CONFIG_DIR = Path.home() / ".codex" CODEX_PROFILE_NAME = "ucode" @@ -373,7 +373,7 @@ def revert_legacy_shared_config() -> bool: Returns True if anything was removed. """ legacy_changed = _strip_legacy_ucode_entries(_legacy_config_path()) - app_catalog_changed = _remove_app_catalog_reference() + app_catalog_changed = detach_app_model_catalog() if app_catalog_changed and CODEX_MODEL_CATALOG_PATH.exists(): CODEX_MODEL_CATALOG_PATH.unlink() return legacy_changed or app_catalog_changed @@ -453,11 +453,15 @@ def write_tool_config( catalog_path = str(CODEX_MODEL_CATALOG_PATH) if static_models and not provider else None # Build and validate before modifying config so failure cannot leave a stale # catalog enabled or partially rewrite the user's configuration. - catalog = ( - prepare_codex_catalog(SPEC["binary"], static_models) - if static_models and not provider - else None - ) + try: + catalog = ( + prepare_codex_catalog(SPEC["binary"], static_models) + if static_models and not provider + else None + ) + except RuntimeError: + _detach_app_catalog_after_failure() + raise _remove_legacy_ucode_profile() # Back up only a file that predates ucode's management of the tool. A @@ -494,7 +498,7 @@ def compose(base: dict, *, include_catalog: bool = True) -> dict: if catalog is not None: _sync_app_model_catalog(catalog) elif not is_dry_run(): - _remove_app_catalog_reference() + detach_app_model_catalog() if CODEX_MODEL_CATALOG_PATH.exists(): CODEX_MODEL_CATALOG_PATH.unlink() @@ -839,6 +843,15 @@ def _install_app_catalog_reference() -> bool: f"Codex App already uses the custom model catalog {existing}; leaving it unchanged." ) return False + provider = doc.get("model_provider") + if provider not in (None, CODEX_MODEL_PROVIDER_NAME, LEGACY_CODEX_MODEL_PROVIDER_NAME): + if _is_ucode_catalog_reference(existing): + doc.pop("model_catalog_json", None) + write_toml_file(path, doc) + print_warning_err( + f"Codex App uses the custom provider {provider}; leaving its model catalog unmanaged." + ) + return False catalog_path = str(CODEX_MODEL_CATALOG_PATH) if existing == catalog_path: return False @@ -847,7 +860,7 @@ def _install_app_catalog_reference() -> bool: return True -def _remove_app_catalog_reference() -> bool: +def detach_app_model_catalog() -> bool: """Remove only a shared catalog reference owned by ucode.""" if is_dry_run(): return False @@ -862,10 +875,19 @@ def _remove_app_catalog_reference() -> bool: return True +def _detach_app_catalog_after_failure() -> None: + """Keep the original catalog error when shared settings cannot be edited.""" + try: + detach_app_model_catalog() + except RuntimeError as exc: + print_warning_err(str(exc)) + + def _sync_app_model_catalog(catalog: dict) -> None: - """Refresh the stable catalog that Codex App loads from shared config.""" + """Publish a validated catalog without overwriting unreadable app settings.""" if is_dry_run(): return + _read_app_config() _write_model_catalog(CODEX_MODEL_CATALOG_PATH, catalog) _install_app_catalog_reference() @@ -972,6 +994,9 @@ def _run_codex( workspace: str | None, ) -> None: """Launch Codex — via the loopback proxy when OTLP tracing is on, else exec-replace.""" + if tool_args[:1] == ["update"]: + # exec replaces ug, so reattach only on a later validated refresh. + detach_app_model_catalog() if otel_tracing and workspace: _launch_codex_with_otel_proxy(state, base_argv, tool_args, workspace) else: @@ -1040,7 +1065,10 @@ def launch( ) _set_provider_header(profile_doc, provider) _set_parent_schema_header(profile_doc, parent_schema if not provider else None) - if workspace and token and (provider or parent_schema): + updating = tool_args[:1] == ["update"] + if updating and _is_ucode_catalog_reference(profile_doc.get("model_catalog_json")): + profile_doc.pop("model_catalog_json") + if workspace and token and (provider or parent_schema) and not updating: try: if provider is not None: catalog_source = CodexCatalogSource.PROVIDER @@ -1058,8 +1086,14 @@ def launch( source=catalog_source, identifier=catalog_identifier, ) + validate_codex_catalog(binary, catalog) except CodexMpsModelCatalogUnavailable: - _remove_app_catalog_reference() + detach_app_model_catalog() + except RuntimeError: + # A failed discovery/validation must not leave a previous workspace's + # catalog active in independently launched app servers. + _detach_app_catalog_after_failure() + raise else: catalog_path = _model_catalog_path(workspace, catalog_scope) _write_model_catalog(catalog_path, catalog) diff --git a/src/ucode/agents/codex_catalog.py b/src/ucode/agents/codex_catalog.py index c6954340a..63e0f44d3 100644 --- a/src/ucode/agents/codex_catalog.py +++ b/src/ucode/agents/codex_catalog.py @@ -15,6 +15,16 @@ from ucode.ui import print_warning _TIMEOUT_SECONDS = 30 +_CATALOG_VALIDATION_ERROR = ( + "Could not load the Codex model catalog with the selected Codex binary. " + "Update Codex and retry `ug codex`." +) +_PREPARE_CATALOG_ERROR = ( + "Could not build the managed Codex model catalog locally. " + "Upgrade the active Codex installation and verify " + "`codex debug models --bundled` works, then retry configuration. " + "Gateway discovery was not used." +) # Known hosted-model capabilities, following universe#2591694. These are not # universal non-GPT defaults: an arbitrary UC service need not support images, @@ -124,6 +134,47 @@ def build_codex_catalog( return {"models": models} +def _run_catalog_command( + binary: str, args: list[str], *, home: str, env: dict[str, str] +) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [binary, *args], + env=env, + cwd=home, + stdin=subprocess.DEVNULL, + capture_output=True, + text=True, + timeout=_TIMEOUT_SECONDS, + check=True, + ) + + +def _validate_codex_catalog_in_home(binary: str, catalog: dict, home: str) -> None: + candidate = Path(home) / "catalog.json" + candidate.write_text(json.dumps(catalog), encoding="utf-8") + env = {**os.environ, "CODEX_HOME": home} + _run_catalog_command( + binary, + [ + "-c", + f"model_catalog_json={json.dumps(str(candidate))}", + "debug", + "models", + ], + home=home, + env=env, + ) + + +def validate_codex_catalog(binary: str, catalog: dict) -> None: + """Validate a candidate catalog with the selected Codex binary in isolation.""" + try: + with tempfile.TemporaryDirectory(prefix="ug-codex-catalog-") as home: + _validate_codex_catalog_in_home(binary, catalog, home) + except (OSError, TypeError, ValueError, subprocess.SubprocessError): + raise RuntimeError(_CATALOG_VALIDATION_ERROR) from None + + def prepare_codex_catalog(binary: str, names: list[str]) -> dict: """Extract and validate with the launch binary; never fetch or reuse stale metadata. @@ -134,19 +185,9 @@ def prepare_codex_catalog(binary: str, names: list[str]) -> dict: try: with tempfile.TemporaryDirectory(prefix="ug-codex-catalog-") as home: env = {**os.environ, "CODEX_HOME": home} - - def run(args: list[str]) -> subprocess.CompletedProcess[str]: - return subprocess.run( - [binary, *args], - env=env, - cwd=home, - capture_output=True, - text=True, - timeout=_TIMEOUT_SECONDS, - check=True, - ) - - result = run(["debug", "models", "--bundled"]) + result = _run_catalog_command( + binary, ["debug", "models", "--bundled"], home=home, env=env + ) payload = json.loads(result.stdout) bundled = payload.get("models") if isinstance(payload, dict) else None if ( @@ -160,23 +201,9 @@ def run(args: list[str]) -> subprocess.CompletedProcess[str]: raise ValueError("invalid bundled model catalog") fallback_warnings: list[str] = [] catalog = build_codex_catalog(bundled, names, warn=fallback_warnings.append) - candidate = Path(home) / "catalog.json" - candidate.write_text(json.dumps(catalog), encoding="utf-8") - run( - [ - "-c", - f"model_catalog_json={json.dumps(str(candidate))}", - "debug", - "models", - ] - ) - except (OSError, ValueError, subprocess.SubprocessError) as exc: - raise RuntimeError( - "Could not build the managed Codex model catalog locally. " - "Upgrade the active Codex installation and verify " - "`codex debug models --bundled` works, then retry configuration. " - "Gateway discovery was not used." - ) from exc + _validate_codex_catalog_in_home(binary, catalog, home) + except (OSError, TypeError, ValueError, subprocess.SubprocessError): + raise RuntimeError(_PREPARE_CATALOG_ERROR) from None for message in fallback_warnings: print_warning(message) return catalog diff --git a/tests/README.md b/tests/README.md index 9fb179988..637bb4be1 100644 --- a/tests/README.md +++ b/tests/README.md @@ -53,11 +53,11 @@ All tests live directly in `integration/`; shared mechanics live in `utils/`. | `test_ug_configure_claude_repeat_and_revert`, `test_ug_configure_codex_repeat_and_revert` | Configure twice over user settings; complete a task; revert twice | Settings preserved; no bearer in ug state; generated config removed; status unconfigured | | `test_ug_configure_claude_cleans_stale_skills_mcp_on_workspace_switch` | Configure the first workspace, register its skills MCP, switch to a second real workspace, and use Claude | Old registration removed from Claude and the new workspace state; old workspace bucket preserved; repeat configure stays clean; real file task completes on the second workspace | | `test_ug_configure_claude_rejects_invalid_credentials`, `test_ug_configure_codex_rejects_invalid_credentials` | Configure with a rejected bearer against the real workspace | Authentication failure; no successful saved setup | -| `test_ug_configure_managed_claude`, `test_ug_configure_managed_codex` | Configure against a workspace that publishes a managed CodingAgentConfig | No agent selector; each agent's generated config exposes exactly the admin's static model_services; real gateway prompt on launch | +| `test_ug_configure_managed_claude`, `test_ug_configure_managed_codex` | Configure against a workspace that publishes a managed CodingAgentConfig | No agent selector; each agent's generated config exposes exactly the admin's static model_services. The Codex case also checks the shared catalog pointer and a fresh bare app-server's visible model list; its TUI assertion covers prompt/input/normal exit. GUI rendering and inference are not covered | | `test_case_01_*`, `test_case_03_*` | Launch managed Claude after configure and from fresh state, with personal discovery enabled and disabled | Claude receives the admin MPS header, caches native discovery results, and opens its real model picker | | `test_case_05_*`, `test_case_07_*` | Pass a provider or model-location override to managed Claude after configure and from fresh state | ug rejects the override before Claude starts and preserves agent-owned state | | `test_case_09_*`, `test_case_11_*` | Disable discovery and pass a provider or model-location override to managed Claude | ug still rejects both configured and fresh launches | -| `test_case_02_*`, `test_case_04_*` | Launch managed Codex after configure and from fresh state, with personal discovery enabled and disabled | ug-launched and fresh bare Codex app servers expose exactly the admin MPS-scoped catalog through the stable catalog and shared config; GUI rendering is not covered | +| `test_case_02_*`, `test_case_04_*` | Launch managed Codex after configure and from fresh state, with personal discovery enabled and disabled | ug-launched and fresh bare Codex app servers expose exactly the admin MPS-scoped catalog through the stable catalog and shared config. The configured case also uses real `ug revert` to remove the ug-owned pointer and stable file while preserving an unrelated user setting; GUI rendering and inference are not covered | | `test_case_06_*`, `test_case_08_*` | Pass a provider or model-location override to managed Codex after configure and from fresh state | ug rejects the override before Codex starts and preserves agent-owned state | | `test_case_10_*`, `test_case_12_*` | Disable discovery and pass a provider or model-location override to managed Codex | ug still rejects both configured and fresh launches | | `test_ug_configure_managed_codex_catalog_fallback` | Configure from an injected managed response containing a GPT model absent from Codex's bundled catalog | Actionable metadata warning; conservative catalog entry for the unknown model; real Codex prompt on the valid default model | diff --git a/tests/integration/README.md b/tests/integration/README.md index ccdc2e8b1..5000f4e25 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -110,7 +110,7 @@ test_ug_configure_claude_workspace_switch.py # real skills MCP cleanup across tw test_ug_configure_codex_lifecycle.py # repeat setup, revert, rejected credentials test_ug_claude_managed_model_discovery.py # fetched/reused Claude MPS policy cases test_ug_codex_managed_model_discovery.py # fetched/reused Codex MPS policy cases -test_ug_configure_managed.py # managed workspace: static model list, no agent selector +test_ug_configure_managed.py # managed workspace: static model list/catalog pointer, no agent selector test_ug_configure_managed_models.py # injected model sources, smart-routing banner, Codex fallback metadata test_ug_configure_managed_mcp.py # injected managed MCP list test_ug_configure_managed_skills.py # injected managed skills: download, coexist, reconcile away @@ -326,7 +326,10 @@ cannot still be running when that gate passes. Full coverage on PRs needs no lab **Managed config** jobs against a second workspace that publishes an admin CodingAgentConfig, which the shared `live` workspace deliberately does not. `ug configure` applies the admin config with no agent selector, and each agent's generated config exposes exactly the admin's static -`model_services` (Claude's `availableModels`/`modelPicker`, Codex's model catalog). +`model_services` (Claude's `availableModels`/`modelPicker`, Codex's model catalog). The managed +Codex case also checks that the shared app config points at the stable catalog and that a fresh +bare Codex app-server returns the expected visible model before the existing TUI prompt/input +assertion. It does not claim GUI rendering or inference coverage. Treat that published CodingAgentConfig as shared CI fixture state. The managed lanes assert its exact model ids and its both-agent enablement, so editing the managed workspace's config (models, @@ -341,8 +344,11 @@ across all configured/fresh scenarios. The Codex module does the same with `main.default.ci_e2e_openai_mps`. The tests verify Claude's admin header, native cache and real model picker, Codex's exact app-server catalog, and both agents' rejection of personal source overrides. Codex discovery also refreshes a stable catalog and shared-config reference, then a -fresh bare `codex app-server` must return the same models without ug launch overrides. This -checks desktop startup configuration, not GUI rendering or inference through the desktop app. +fresh bare `codex app-server` must return the same models without ug launch overrides. The +configured discovery journey subsequently runs real `ug revert`, verifies that the shared pointer +and stable catalog are gone, and checks that an ordinary user-owned Codex setting survives. This +checks desktop startup configuration and cleanup, not GUI rendering or inference through the +desktop app. Codex state comparisons exclude `.codex/tmp/arg0`, the disposable executable links recreated by version checks, while continuing to compare persistent agent files. In addition, `test_ug_configure_managed_codex_catalog_fallback` injects the intentionally nonexistent diff --git a/tests/integration/test_ug_codex_managed_model_discovery.py b/tests/integration/test_ug_codex_managed_model_discovery.py index 72d1a75bd..e50e0e2c2 100644 --- a/tests/integration/test_ug_codex_managed_model_discovery.py +++ b/tests/integration/test_ug_codex_managed_model_discovery.py @@ -16,6 +16,7 @@ is_managed_config_control_plane_cache, use_managed_config_stub, ) +from utils.terminal import TerminalProcess pytestmark = [pytest.mark.managed_fixture, pytest.mark.codex] @@ -107,9 +108,18 @@ def test_case_02_managed_codex_uses_admin_discovery_after_configure(live_session """Scenario: configure managed Codex, then launch its app server. Expected: ug-launched and fresh bare Codex app servers expose exactly the admin-managed - model catalog. This verifies the desktop startup configuration, not GUI rendering. + model catalog. After those checks, real `ug revert` removes the ug-owned shared catalog + pointer and stable catalog while preserving an unrelated user-owned Codex setting. This + verifies desktop startup configuration and cleanup, not GUI rendering or inference. """ session = live_session + user_config = session.home / ".codex" / "config.toml" + original_user_config = ( + "# user-owned Codex settings\n[notice]\nhide_rate_limit_model_nudge = true\n" + ) + user_config.parent.mkdir(parents=True) + user_config.write_text(original_user_config) + configured = session.run( "configure", "--workspace", @@ -128,6 +138,15 @@ def test_case_02_managed_codex_uses_admin_discovery_after_configure(live_session ) assert app_models == models, (app_models, models) + with TerminalProcess( + session, "ug", [str(session.binary), "revert"], "managed-discovery-revert" + ) as terminal: + terminal.finish() + shared_config = tomllib.loads(user_config.read_text()) + assert "model_catalog_json" not in shared_config, shared_config + assert tomllib.loads(user_config.read_text()) == tomllib.loads(original_user_config) + assert not (session.home / ".ucode" / "codex-model-catalog.json").exists() + def test_case_02_managed_codex_uses_admin_discovery_from_fresh_state(live_session, workspace): """Scenario: launch managed Codex with --workspace from fresh state. diff --git a/tests/integration/test_ug_configure_managed.py b/tests/integration/test_ug_configure_managed.py index 759f2fd71..f0e656ffa 100644 --- a/tests/integration/test_ug_configure_managed.py +++ b/tests/integration/test_ug_configure_managed.py @@ -9,6 +9,7 @@ """ import json +import tomllib import pytest from utils.terminal import AgentTerminal @@ -54,8 +55,9 @@ def test_ug_configure_managed_codex(live_session, workspace): Expected: ug applies the admin config to every enabled agent without showing the personal agent selector, Codex's generated model catalog lists exactly the admin's static - model_services, and launching Codex reaches a real gateway prompt rather than the - account-login flow. + model_services, and the shared Codex App config points at that stable catalog. A fresh bare + Codex app-server returns the expected visible model, while the existing TUI assertion reaches + a prompt, accepts input, and exits normally. GUI rendering and inference are not covered. """ session = live_session result = session.run("configure", "--workspace", workspace, "--skip-upgrade", timeout=240) @@ -69,6 +71,17 @@ def test_ug_configure_managed_codex(live_session, workspace): ] assert listed == [MANAGED_CODEX_MODEL], catalog + shared_config = tomllib.loads((session.home / ".codex" / "config.toml").read_text()) + assert shared_config.get("model_catalog_json") == str( + session.home / ".ucode" / "codex-model-catalog.json" + ), shared_config + bare_models = session.codex_model_ids( + ["app-server", "--listen", "stdio://"], + name="bare-managed-codex-models", + binary="codex", + ) + assert bare_models == [MANAGED_CODEX_MODEL], bare_models + with AgentTerminal(session, "codex", [str(session.binary), "codex"], "managed-codex") as tui: tui.boot() tui.check_input_and_exit() diff --git a/tests/test_agent_codex.py b/tests/test_agent_codex.py index 7027dcedf..ac9b5958c 100644 --- a/tests/test_agent_codex.py +++ b/tests/test_agent_codex.py @@ -804,11 +804,34 @@ def test_invalid_shared_config_is_not_overwritten(self): shared_path.parent.mkdir() original = 'model = "unfinished\n' shared_path.write_text(original, encoding="utf-8") + codex.CODEX_MODEL_CATALOG_PATH.write_text("previous catalog", encoding="utf-8") with pytest.raises(RuntimeError, match="Cannot update Codex App settings"): codex._sync_app_model_catalog({"models": [{"slug": "gpt-mps"}]}) assert shared_path.read_text() == original + assert codex.CODEX_MODEL_CATALOG_PATH.read_text() == "previous catalog" + + @pytest.mark.parametrize("previous_catalog", [False, True]) + def test_custom_provider_keeps_its_own_model_discovery(self, previous_catalog): + if previous_catalog: + codex._sync_app_model_catalog({"models": [{"slug": "previous"}]}) + shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" + shared_path.parent.mkdir(exist_ok=True) + original = '# User settings\nmodel_provider = "custom"\nmodel = "user-model"\n' + shared_path.write_text( + original + + ( + f'model_catalog_json = "{codex.CODEX_MODEL_CATALOG_PATH}"\n' + if previous_catalog + else "" + ), + encoding="utf-8", + ) + + codex._sync_app_model_catalog({"models": [{"slug": "gpt-mps"}]}) + + assert shared_path.read_text() == original def test_dry_run_leaves_config_and_catalog_unchanged(self, monkeypatch): codex._sync_app_model_catalog({"models": [{"slug": "previous"}]}) @@ -833,6 +856,23 @@ def test_reconfigure_clears_discovered_app_catalog(self, monkeypatch): assert "model_catalog_json" not in read_toml_safe(shared_path) assert not codex.CODEX_MODEL_CATALOG_PATH.exists() + def test_failed_static_validation_detaches_previous_app_catalog(self, monkeypatch): + codex._sync_app_model_catalog({"models": [{"slug": "old-model"}]}) + original_catalog = codex.CODEX_MODEL_CATALOG_PATH.read_bytes() + monkeypatch.setattr(codex, "agent_version", lambda _: "0.154.0") + + def reject(*args): + raise RuntimeError("catalog is incompatible") + + monkeypatch.setattr(codex, "prepare_codex_catalog", reject) + with pytest.raises(RuntimeError, match="incompatible"): + codex.write_tool_config({"workspace": WS, "codex_static_models": ["new-model"]}) + + assert "model_catalog_json" not in read_toml_safe( + codex.CODEX_CONFIG_PATH.parent / "config.toml" + ) + assert codex.CODEX_MODEL_CATALOG_PATH.read_bytes() == original_catalog + def test_revert_preserves_subsequent_user_catalog(self, tmp_path): codex._sync_app_model_catalog({"models": [{"slug": "gpt-mps"}]}) shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" @@ -918,6 +958,9 @@ def _patch(tmp_path, monkeypatch): lambda workspace, profile=None, force_refresh=False: "tok", ) monkeypatch.setattr(codex, "clear_model_preferences", lambda state: False) + # These launch tests isolate the real-binary validation boundary; its + # subprocess contract is covered in test_codex_catalog.py. + monkeypatch.setattr(codex, "validate_codex_catalog", lambda binary, catalog: None) return launches def test_sets_oauth_token(self, tmp_path, monkeypatch): @@ -933,6 +976,47 @@ def test_sets_oauth_token(self, tmp_path, monkeypatch): assert os.environ["OAUTH_TOKEN"] == "fresh-token" assert launches[0][-1] == "--search" + @pytest.mark.parametrize("custom_catalog", [None, "/user/isaac-app-model-catalog.json"]) + def test_native_update_detaches_catalog_without_discovery( + self, tmp_path, monkeypatch, custom_catalog + ): + self._patch(tmp_path, monkeypatch) + codex._sync_app_model_catalog({"models": [{"slug": "old-model"}]}) + shared_path = tmp_path / "config.toml" + if custom_catalog: + shared_path.write_text(f'model_catalog_json = "{custom_catalog}"\n') + profile_path = codex.CODEX_CONFIG_PATH + profile_path.write_text( + f'model_catalog_json = "{codex.CODEX_MODEL_CATALOG_PATH}"\n' + profile_path.read_text() + ) + monkeypatch.setattr( + codex, + "_fetch_codex_model_catalog", + lambda *a, **k: pytest.fail("update must not depend on gateway discovery"), + ) + monkeypatch.setattr( + codex, + "validate_codex_catalog", + lambda *a, **k: pytest.fail("update must not load an old catalog"), + ) + launches = [] + + def execute(argv): + assert read_toml_safe(shared_path).get("model_catalog_json") == custom_catalog + assert not any(arg.startswith("model_catalog_json=") for arg in argv) + launches.append(argv) + + monkeypatch.setattr(codex, "exec_or_spawn", execute) + codex.launch( + {"workspace": WS, "_codex_launch_provider": "main.default.openai"}, + ["update"], + options=LaunchOptions(), + ) + + assert len(launches) == 1 + assert launches[0][-1] == "update" + assert read_toml_safe(shared_path).get("model_catalog_json") == custom_catalog + def test_provider_discovery_uses_authoritative_catalog(self, tmp_path, monkeypatch): launches = self._patch(tmp_path, monkeypatch) catalog_path = tmp_path / "models.json" @@ -971,6 +1055,61 @@ def test_provider_discovery_uses_authoritative_catalog(self, tmp_path, monkeypat ) assert 'Databricks-Model-Provider-Service = "main.default.openai"' in provider_arg + def test_discovery_is_validated_before_catalogs_are_published(self, tmp_path, monkeypatch): + launches = self._patch(tmp_path, monkeypatch) + catalog = {"models": [{"slug": "gpt-mps", "future_metadata": {"tools": True}}]} + monkeypatch.setattr(codex, "_fetch_codex_model_catalog", lambda *a, **k: catalog) + validations = [] + + def validate(binary, candidate): + assert not codex.CODEX_MODEL_CATALOG_PATH.exists() + assert not list( + codex.CODEX_MODEL_CATALOG_PATH.parent.glob("codex-model-catalog-*.json") + ) + validations.append((binary, candidate)) + + monkeypatch.setattr(codex, "validate_codex_catalog", validate) + codex.launch( + {"workspace": WS, "_codex_launch_provider": "main.default.openai"}, + [], + options=LaunchOptions(), + ) + + assert validations == [(codex.SPEC["binary"], catalog)] + assert json.loads(codex.CODEX_MODEL_CATALOG_PATH.read_text()) == catalog + assert launches + + @pytest.mark.parametrize("custom_catalog", [None, "/user/isaac-app-model-catalog.json"]) + def test_incompatible_discovery_removes_only_ug_reference( + self, tmp_path, monkeypatch, custom_catalog + ): + launches = self._patch(tmp_path, monkeypatch) + codex._sync_app_model_catalog({"models": [{"slug": "old-model"}]}) + original_catalog = codex.CODEX_MODEL_CATALOG_PATH.read_bytes() + shared_path = tmp_path / "config.toml" + if custom_catalog: + shared_path.write_text(f'model_catalog_json = "{custom_catalog}"\n') + monkeypatch.setattr( + codex, "_fetch_codex_model_catalog", lambda *a, **k: {"models": [{"slug": "new"}]} + ) + + def reject(binary, catalog): + raise RuntimeError("The installed Codex cannot load this model catalog") + + monkeypatch.setattr(codex, "validate_codex_catalog", reject) + + with pytest.raises(RuntimeError, match="cannot load this model catalog"): + codex.launch( + {"workspace": WS, "_codex_launch_provider": "main.default.openai"}, + [], + options=LaunchOptions(), + ) + + assert not launches + assert read_toml_safe(shared_path).get("model_catalog_json") == custom_catalog + assert codex.CODEX_MODEL_CATALOG_PATH.read_bytes() == original_catalog + assert not list(codex.CODEX_MODEL_CATALOG_PATH.parent.glob("codex-model-catalog-*.json")) + def test_provider_discovery_preserves_custom_app_catalog(self, tmp_path, monkeypatch, capsys): launches = self._patch(tmp_path, monkeypatch) catalog_path = tmp_path / "models.json" @@ -1205,6 +1344,27 @@ def test_provider_keeps_other_discovery_failures_fatal(self, tmp_path, monkeypat assert launches == [] + def test_discovery_error_survives_unreadable_shared_config(self, tmp_path, monkeypatch, capsys): + launches = self._patch(tmp_path, monkeypatch) + shared_path = tmp_path / "config.toml" + original = 'model = "unfinished\n' + shared_path.write_text(original) + + def fail_discovery(*args, **kwargs): + raise RuntimeError("HTTP 403 Forbidden") + + monkeypatch.setattr(codex, "_fetch_codex_model_catalog", fail_discovery) + with pytest.raises(RuntimeError, match="HTTP 403 Forbidden"): + codex.launch( + {"workspace": WS, "_codex_launch_provider": "main.default.openai"}, + [], + options=LaunchOptions(), + ) + + assert not launches + assert shared_path.read_text() == original + assert "Cannot update Codex App settings" in " ".join(capsys.readouterr().err.split()) + def test_provider_rejects_managed_model_catalog(self, tmp_path, monkeypatch): launches = self._patch(tmp_path, monkeypatch) managed_path = tmp_path / "managed_config.toml" diff --git a/tests/test_agents_init.py b/tests/test_agents_init.py index 99caa179d..e5f7955d0 100644 --- a/tests/test_agents_init.py +++ b/tests/test_agents_init.py @@ -588,6 +588,15 @@ def _boom(*a, **k): # pragma: no cover - must never run class TestInstallToolBinary: + @staticmethod + def _seed_codex_catalog_reference(catalog_ref: str | None = None): + codex = agents_mod.codex + shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" + shared_path.parent.mkdir(parents=True, exist_ok=True) + reference = catalog_ref or str(codex.CODEX_MODEL_CATALOG_PATH) + shared_path.write_text(f'model_catalog_json = "{reference}"\n', encoding="utf-8") + return shared_path + def test_non_strict_returns_false_when_npm_missing(self, monkeypatch): monkeypatch.setattr("ucode.agents.shutil.which", lambda _: None) @@ -656,6 +665,97 @@ def test_required_update_prompts_and_rechecks(self, monkeypatch, tool, command): assert calls == [command] assert prompts == [(f"Upgrade {TOOL_SPECS[tool]['display']} if available?", True)] + @pytest.mark.parametrize( + "catalog_ref", + [None, "/user/isaac-app-model-catalog.json"], + ids=["ug-catalog", "custom-catalog"], + ) + def test_codex_update_detaches_only_ug_catalog_before_mutation(self, monkeypatch, catalog_ref): + shared_path = self._seed_codex_catalog_reference(catalog_ref) + calls = [] + + monkeypatch.setattr("ucode.agents.shutil.which", lambda binary: f"/usr/bin/{binary}") + monkeypatch.setattr("ucode.agents._too_new_downgrade", lambda _: None) + monkeypatch.setattr("ucode.agents._minimum_version_error", lambda _: None) + + def fake_run(args, **kwargs): + calls.append(args) + contents = shared_path.read_text(encoding="utf-8") + if catalog_ref is None: + assert "model_catalog_json" not in contents + else: + assert f'model_catalog_json = "{catalog_ref}"' in contents + return subprocess.CompletedProcess(args, 0) + + monkeypatch.setattr("ucode.agents.subprocess.run", fake_run) + + assert agents_mod._update_installed_tool_binary("codex") is True + assert calls == [["codex", "update"]] + + def test_codex_update_failure_leaves_ug_catalog_detached(self, monkeypatch): + shared_path = self._seed_codex_catalog_reference() + + monkeypatch.setattr("ucode.agents.shutil.which", lambda binary: f"/usr/bin/{binary}") + monkeypatch.setattr("ucode.agents._minimum_version_error", lambda _: "must upgrade") + monkeypatch.setattr("ucode.agents._too_new_downgrade", lambda _: None) + + def fail_run(args, **kwargs): + assert "model_catalog_json" not in shared_path.read_text(encoding="utf-8") + raise subprocess.CalledProcessError(1, args) + + monkeypatch.setattr("ucode.agents.subprocess.run", fail_run) + + assert agents_mod._update_installed_tool_binary("codex") is False + assert "model_catalog_json" not in shared_path.read_text(encoding="utf-8") + + def test_codex_install_failure_leaves_ug_catalog_detached(self, monkeypatch): + shared_path = self._seed_codex_catalog_reference() + + def fake_which(binary: str) -> str | None: + return "/usr/bin/npm" if binary == "npm" else None + + def fail_run(args, **kwargs): + assert "model_catalog_json" not in shared_path.read_text(encoding="utf-8") + raise subprocess.CalledProcessError(1, args) + + monkeypatch.setattr("ucode.agents.shutil.which", fake_which) + monkeypatch.setattr("ucode.agents.subprocess.run", fail_run) + + assert install_tool_binary("codex", strict=False) is False + assert "model_catalog_json" not in shared_path.read_text(encoding="utf-8") + + def test_catalog_detach_failure_blocks_codex_binary_mutation(self, monkeypatch): + calls = [] + + monkeypatch.setattr("ucode.agents.shutil.which", lambda binary: f"/usr/bin/{binary}") + monkeypatch.setattr( + agents_mod.codex, + "detach_app_model_catalog", + lambda: (_ for _ in ()).throw(RuntimeError("cannot detach catalog")), + ) + monkeypatch.setattr( + "ucode.agents.subprocess.run", lambda args, **kwargs: calls.append(args) + ) + + with pytest.raises(RuntimeError, match="cannot detach catalog"): + agents_mod._update_installed_tool_binary("codex") + assert calls == [] + + def test_claude_update_does_not_detach_codex_catalog(self, monkeypatch): + shared_path = self._seed_codex_catalog_reference() + calls = [] + + monkeypatch.setattr("ucode.agents.shutil.which", lambda binary: f"/usr/bin/{binary}") + monkeypatch.setattr("ucode.agents._minimum_version_error", lambda _: None) + monkeypatch.setattr( + "ucode.agents.subprocess.run", + lambda args, **kwargs: calls.append(args) or subprocess.CompletedProcess(args, 0), + ) + + assert agents_mod._update_installed_tool_binary("claude") is True + assert calls == [["claude", "upgrade"]] + assert "model_catalog_json" in shared_path.read_text(encoding="utf-8") + @pytest.mark.parametrize("tool", ["claude", "codex"]) def test_required_update_declined_blocks_launch(self, monkeypatch, tool): monkeypatch.setattr("ucode.agents.shutil.which", lambda binary: f"/usr/bin/{binary}") diff --git a/tests/test_codex_catalog.py b/tests/test_codex_catalog.py index be93b1541..6f00ee13e 100644 --- a/tests/test_codex_catalog.py +++ b/tests/test_codex_catalog.py @@ -166,6 +166,7 @@ def run(argv, **kwargs): homes.append(home) assert kwargs["cwd"] == str(home) assert home.exists() + assert kwargs["stdin"] is subprocess.DEVNULL assert kwargs["timeout"] == 30 assert kwargs["check"] is True if len(calls) == 1: @@ -186,6 +187,75 @@ def run(argv, **kwargs): assert not homes[0].exists() +def test_validate_discovered_catalog_with_same_binary_in_isolation(monkeypatch): + discovered = { + "models": [ + { + "slug": "system.ai.gpt-6-astra", + "future_field": {"nested": [1, "keep"]}, + "visibility": "list", + } + ], + "future_catalog_field": {"enabled": True}, + } + original = copy.deepcopy(discovered) + calls = [] + homes = [] + + def run(argv, **kwargs): + calls.append(argv) + home = Path(kwargs["env"]["CODEX_HOME"]) + homes.append(home) + assert argv[0] == "/selected/codex" + assert argv[-2:] == ["debug", "models"] + assert kwargs["cwd"] == str(home) + assert kwargs["env"]["CODEX_HOME"] != "/user/codex-home" + assert kwargs["stdin"] is subprocess.DEVNULL + assert kwargs["timeout"] == 30 + assert kwargs["check"] is True + assert kwargs["capture_output"] is True + assert kwargs["text"] is True + setting = json.loads(argv[2].split("=", 1)[1]) + candidate = Path(setting) + assert candidate.parent == home + assert json.loads(candidate.read_text()) == discovered + return subprocess.CompletedProcess(argv, 0, "") + + monkeypatch.setenv("CODEX_HOME", "/user/codex-home") + monkeypatch.setattr(catalog.subprocess, "run", run) + + assert catalog.validate_codex_catalog("/selected/codex", discovered) is None + + assert len(calls) == 1 + assert calls[0][1] == "-c" + assert discovered == original + assert not homes[0].exists() + + +@pytest.mark.parametrize( + "error", + [ + FileNotFoundError("codex not found"), + subprocess.TimeoutExpired("codex", 30), + subprocess.CalledProcessError(1, ["codex"], stderr="catalog secret"), + ], + ids=["missing-binary", "timeout", "rejected-catalog"], +) +def test_validate_discovered_catalog_failure_is_actionable_without_payload(monkeypatch, error): + catalog_payload = {"models": [{"slug": "system.ai.secret-model"}]} + + def run(*args, **kwargs): + raise error + + monkeypatch.setattr(catalog.subprocess, "run", run) + + with pytest.raises(RuntimeError, match="Update Codex") as raised: + catalog.validate_codex_catalog("/selected/codex", catalog_payload) + + assert "secret-model" not in str(raised.value) + assert "catalog secret" not in str(raised.value) + + def test_missing_gpt_warning_is_printed_after_validation(monkeypatch, bundled): calls = [] warnings = [] From e08d60d2366f29f5168a0774f6a740b9c12b5b7b Mon Sep 17 00:00:00 2001 From: Andy Xu Date: Tue, 22 Sep 2026 04:00:35 +0000 Subject: [PATCH 3/5] Explain Codex App catalog reloads --- README.md | 26 ++++++++-- src/ucode/agents/codex.py | 31 +++++++++-- tests/README.md | 2 +- tests/integration/README.md | 3 +- .../integration/test_ug_configure_managed.py | 4 +- tests/test_agent_codex.py | 52 ++++++++++++++++++- 6 files changed, 107 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 2a29cea49..43e9e7bae 100644 --- a/README.md +++ b/README.md @@ -59,16 +59,34 @@ to select another model source; managed workspace configs control their own sour When `ug codex` discovers a model catalog, it validates it with the installed Codex binary, then refreshes `~/.ucode/codex-model-catalog.json` and points the shared `~/.codex/config.toml` -at it. Managed static model lists use the same path. Restart Codex App to load -the latest list; the app's provider and authentication must already be configured -for the corresponding gateway. The most recently refreshed workspace supplies +at it. Managed static model lists use the same path. The app's provider and +authentication must already be configured for the corresponding gateway. +The most recently refreshed workspace supplies the app catalog. Existing custom catalog settings (including Isaac's catalog) and custom providers are preserved; catalogs are not combined. `ug revert` removes the shared catalog reference installed by ug. +Codex loads the catalog when its app server starts. An already-running server +keeps its old list, even if you reconnect or open a new task. After active tasks +finish, restart the app server on the **connected host**, then reconnect the app. +If the host uses Codex's standalone managed daemon, run: + +```bash +codex app-server daemon restart +``` + +The daemon command requires a standalone Codex installation. For an app server +started by npm Codex or by the desktop app, restart the process or application +that owns `codex app-server --listen unix://`; the daemon command cannot manage +it. Restarting the desktop app alone can reconnect to the same remote server. +ug reports the restart step when it changes the app catalog; it does not restart +active servers automatically. Checking out this branch alone does not refresh +the catalog: run `uv run ug codex` from the checkout for discovery, or +`uv run ug configure` for a managed static list, before restarting the server. + Before installing or updating Codex, ug detaches its shared catalog reference. Run `ug codex` again to refresh discovery, or rerun `ug configure` for a managed -static list, before restarting or reconnecting the app. Do the same after +static list, then restart the app server as described above. Do the same after updating Codex outside ug. A failed catalog validation removes ug's shared reference and reports an error rather than publishing an incompatible catalog. Validation uses the Codex binary on this host; it does not verify a desktop diff --git a/src/ucode/agents/codex.py b/src/ucode/agents/codex.py index 65609bc79..32cb4817a 100644 --- a/src/ucode/agents/codex.py +++ b/src/ucode/agents/codex.py @@ -31,6 +31,7 @@ deep_merge_dict, is_dry_run, prune_key_paths, + read_json_safe, read_toml_safe, write_json_file, write_toml_file, @@ -848,10 +849,13 @@ def _install_app_catalog_reference() -> bool: if _is_ucode_catalog_reference(existing): doc.pop("model_catalog_json", None) write_toml_file(path, doc) + changed = True + else: + changed = False print_warning_err( f"Codex App uses the custom provider {provider}; leaving its model catalog unmanaged." ) - return False + return changed catalog_path = str(CODEX_MODEL_CATALOG_PATH) if existing == catalog_path: return False @@ -872,6 +876,7 @@ def detach_app_model_catalog() -> bool: return False doc.pop("model_catalog_json", None) write_toml_file(path, doc) + _print_app_catalog_restart_notice() return True @@ -883,13 +888,33 @@ def _detach_app_catalog_after_failure() -> None: print_warning_err(str(exc)) +def _print_app_catalog_restart_notice() -> None: + # A desktop reconnect can reuse a daemon whose model manager still holds + # the startup catalog. Never restart it here: it may be running tasks. + print_warning_err( + "Codex App model catalog changed. Existing app servers keep their startup " + "model list. After active tasks finish, restart the app server on the connected " + "host, then reconnect. For a Codex standalone daemon, run " + "`codex app-server daemon restart`; otherwise restart the process or application " + "that owns the app server. Reconnecting alone does not reload the catalog." + ) + + def _sync_app_model_catalog(catalog: dict) -> None: """Publish a validated catalog without overwriting unreadable app settings.""" if is_dry_run(): return - _read_app_config() + doc = _read_app_config() + catalog_changed = read_json_safe(CODEX_MODEL_CATALOG_PATH) != catalog _write_model_catalog(CODEX_MODEL_CATALOG_PATH, catalog) - _install_app_catalog_reference() + reference_changed = _install_app_catalog_reference() + app_uses_catalog = reference_changed or ( + _is_ucode_catalog_reference(doc.get("model_catalog_json")) + and doc.get("model_provider") + in (None, CODEX_MODEL_PROVIDER_NAME, LEGACY_CODEX_MODEL_PROVIDER_NAME) + ) + if reference_changed or (app_uses_catalog and catalog_changed): + _print_app_catalog_restart_notice() def _launch_token(state: dict, workspace: str, *, force_refresh: bool = False) -> str: diff --git a/tests/README.md b/tests/README.md index 637bb4be1..4a4cb560c 100644 --- a/tests/README.md +++ b/tests/README.md @@ -53,7 +53,7 @@ All tests live directly in `integration/`; shared mechanics live in `utils/`. | `test_ug_configure_claude_repeat_and_revert`, `test_ug_configure_codex_repeat_and_revert` | Configure twice over user settings; complete a task; revert twice | Settings preserved; no bearer in ug state; generated config removed; status unconfigured | | `test_ug_configure_claude_cleans_stale_skills_mcp_on_workspace_switch` | Configure the first workspace, register its skills MCP, switch to a second real workspace, and use Claude | Old registration removed from Claude and the new workspace state; old workspace bucket preserved; repeat configure stays clean; real file task completes on the second workspace | | `test_ug_configure_claude_rejects_invalid_credentials`, `test_ug_configure_codex_rejects_invalid_credentials` | Configure with a rejected bearer against the real workspace | Authentication failure; no successful saved setup | -| `test_ug_configure_managed_claude`, `test_ug_configure_managed_codex` | Configure against a workspace that publishes a managed CodingAgentConfig | No agent selector; each agent's generated config exposes exactly the admin's static model_services. The Codex case also checks the shared catalog pointer and a fresh bare app-server's visible model list; its TUI assertion covers prompt/input/normal exit. GUI rendering and inference are not covered | +| `test_ug_configure_managed_claude`, `test_ug_configure_managed_codex` | Configure against a workspace that publishes a managed CodingAgentConfig | No agent selector; each agent's generated config exposes exactly the admin's static model_services. The Codex case also checks the shared catalog pointer, stderr guidance to restart the daemon, and a fresh bare app-server's visible model list; its TUI assertion covers prompt/input/normal exit. GUI rendering and inference are not covered | | `test_case_01_*`, `test_case_03_*` | Launch managed Claude after configure and from fresh state, with personal discovery enabled and disabled | Claude receives the admin MPS header, caches native discovery results, and opens its real model picker | | `test_case_05_*`, `test_case_07_*` | Pass a provider or model-location override to managed Claude after configure and from fresh state | ug rejects the override before Claude starts and preserves agent-owned state | | `test_case_09_*`, `test_case_11_*` | Disable discovery and pass a provider or model-location override to managed Claude | ug still rejects both configured and fresh launches | diff --git a/tests/integration/README.md b/tests/integration/README.md index 5000f4e25..b9f979e47 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -327,7 +327,8 @@ cannot still be running when that gate passes. Full coverage on PRs needs no lab which the shared `live` workspace deliberately does not. `ug configure` applies the admin config with no agent selector, and each agent's generated config exposes exactly the admin's static `model_services` (Claude's `availableModels`/`modelPicker`, Codex's model catalog). The managed -Codex case also checks that the shared app config points at the stable catalog and that a fresh +Codex case also checks stderr guidance to restart the daemon after publication, +that the shared app config points at the stable catalog, and that a fresh bare Codex app-server returns the expected visible model before the existing TUI prompt/input assertion. It does not claim GUI rendering or inference coverage. diff --git a/tests/integration/test_ug_configure_managed.py b/tests/integration/test_ug_configure_managed.py index f0e656ffa..98989dca9 100644 --- a/tests/integration/test_ug_configure_managed.py +++ b/tests/integration/test_ug_configure_managed.py @@ -55,13 +55,15 @@ def test_ug_configure_managed_codex(live_session, workspace): Expected: ug applies the admin config to every enabled agent without showing the personal agent selector, Codex's generated model catalog lists exactly the admin's static - model_services, and the shared Codex App config points at that stable catalog. A fresh bare + model_services, the shared Codex App config points at that stable catalog, and configure + reports the daemon restart step on stderr. A fresh bare Codex app-server returns the expected visible model, while the existing TUI assertion reaches a prompt, accepts input, and exits normally. GUI rendering and inference are not covered. """ session = live_session result = session.run("configure", "--workspace", workspace, "--skip-upgrade", timeout=240) assert "Select coding agents to configure:" not in result.stdout, result.stdout + assert "codex app-server daemon restart" in " ".join(result.stderr.split()), result.stderr catalog = json.loads((session.home / ".ucode" / "codex-model-catalog.json").read_text()) listed = [ diff --git a/tests/test_agent_codex.py b/tests/test_agent_codex.py index ac9b5958c..ab7a7b1c2 100644 --- a/tests/test_agent_codex.py +++ b/tests/test_agent_codex.py @@ -780,6 +780,50 @@ def test_strips_ucode_app_catalog_reference(self, tmp_path, monkeypatch): class TestCodexAppCatalog: + def test_catalog_changes_report_server_restart_on_stderr(self, capsys): + first_catalog = {"models": [{"slug": "first-model"}]} + codex._sync_app_model_catalog(first_catalog) + published = capsys.readouterr() + assert published.out == "" + assert "codex app-server daemon restart" in " ".join(published.err.split()) + assert "After active tasks finish" in published.err + + codex._sync_app_model_catalog(first_catalog) + unchanged = capsys.readouterr() + assert unchanged.out == unchanged.err == "" + + codex._sync_app_model_catalog({"models": [{"slug": "second-model"}]}) + refreshed = capsys.readouterr() + assert refreshed.out == "" + assert "codex app-server daemon restart" in " ".join(refreshed.err.split()) + + def test_attaching_existing_catalog_reports_server_restart(self, capsys): + catalog = {"models": [{"slug": "first-model"}]} + codex._write_model_catalog(codex.CODEX_MODEL_CATALOG_PATH, catalog) + + codex._sync_app_model_catalog(catalog) + + assert "codex app-server daemon restart" in " ".join(capsys.readouterr().err.split()) + + def test_detaching_catalog_reports_server_restart(self, capsys): + codex._sync_app_model_catalog({"models": [{"slug": "first-model"}]}) + capsys.readouterr() + + assert codex.detach_app_model_catalog() + + assert "codex app-server daemon restart" in " ".join(capsys.readouterr().err.split()) + + def test_custom_app_catalog_does_not_report_ug_restart(self, capsys): + shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" + shared_path.parent.mkdir() + shared_path.write_text('model_catalog_json = "/user/models.json"\n') + + codex._sync_app_model_catalog({"models": [{"slug": "gateway-model"}]}) + + output = capsys.readouterr() + assert "leaving it unchanged" in " ".join(output.err.split()) + assert "daemon restart" not in output.err + def test_refresh_keeps_stable_pointer_and_preserves_settings(self, tmp_path): shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" shared_path.parent.mkdir() @@ -813,7 +857,7 @@ def test_invalid_shared_config_is_not_overwritten(self): assert codex.CODEX_MODEL_CATALOG_PATH.read_text() == "previous catalog" @pytest.mark.parametrize("previous_catalog", [False, True]) - def test_custom_provider_keeps_its_own_model_discovery(self, previous_catalog): + def test_custom_provider_keeps_its_own_model_discovery(self, previous_catalog, capsys): if previous_catalog: codex._sync_app_model_catalog({"models": [{"slug": "previous"}]}) shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" @@ -829,9 +873,15 @@ def test_custom_provider_keeps_its_own_model_discovery(self, previous_catalog): encoding="utf-8", ) + capsys.readouterr() codex._sync_app_model_catalog({"models": [{"slug": "gpt-mps"}]}) assert shared_path.read_text() == original + output = capsys.readouterr().err + if previous_catalog: + assert "daemon restart" in " ".join(output.split()) + else: + assert "daemon restart" not in output def test_dry_run_leaves_config_and_catalog_unchanged(self, monkeypatch): codex._sync_app_model_catalog({"models": [{"slug": "previous"}]}) From 9ab859c2fc201779383cdc96b142a1964fac4928 Mon Sep 17 00:00:00 2001 From: Andy Xu Date: Tue, 22 Sep 2026 13:57:04 +0000 Subject: [PATCH 4/5] Simplify Codex App catalog publication and coverage Read shared configuration once during publication and centralize subprocess isolation. Consolidate duplicate publication, restart, validation-order, and install failure tests into existing lifecycle and launch coverage. Keep real app-server integration assertions and shorten usage guidance. --- README.md | 51 ++++--------- src/ucode/agents/__init__.py | 6 +- src/ucode/agents/codex.py | 68 +++++++---------- src/ucode/agents/codex_catalog.py | 35 ++++----- tests/test_agent_codex.py | 120 ++++++++++-------------------- tests/test_agents_init.py | 29 +++----- 6 files changed, 104 insertions(+), 205 deletions(-) diff --git a/README.md b/README.md index 43e9e7bae..c1c942213 100644 --- a/README.md +++ b/README.md @@ -56,41 +56,22 @@ models for Claude Code's `/model` picker. Discovery defaults to `system.ai` when no provider or model location is selected. Use `--provider` or `--model-location` to select another model source; managed workspace configs control their own sources. -When `ug codex` discovers a model catalog, it validates it with the installed -Codex binary, then refreshes -`~/.ucode/codex-model-catalog.json` and points the shared `~/.codex/config.toml` -at it. Managed static model lists use the same path. The app's provider and -authentication must already be configured for the corresponding gateway. -The most recently refreshed workspace supplies -the app catalog. Existing custom catalog settings (including Isaac's catalog) -and custom providers are preserved; catalogs are not combined. `ug revert` -removes the shared catalog reference installed by ug. - -Codex loads the catalog when its app server starts. An already-running server -keeps its old list, even if you reconnect or open a new task. After active tasks -finish, restart the app server on the **connected host**, then reconnect the app. -If the host uses Codex's standalone managed daemon, run: - -```bash -codex app-server daemon restart -``` - -The daemon command requires a standalone Codex installation. For an app server -started by npm Codex or by the desktop app, restart the process or application -that owns `codex app-server --listen unix://`; the daemon command cannot manage -it. Restarting the desktop app alone can reconnect to the same remote server. -ug reports the restart step when it changes the app catalog; it does not restart -active servers automatically. Checking out this branch alone does not refresh -the catalog: run `uv run ug codex` from the checkout for discovery, or -`uv run ug configure` for a managed static list, before restarting the server. - -Before installing or updating Codex, ug detaches its shared catalog reference. -Run `ug codex` again to refresh discovery, or rerun `ug configure` for a managed -static list, then restart the app server as described above. Do the same after -updating Codex outside ug. A failed catalog validation removes ug's shared -reference and reports an error rather than publishing an incompatible catalog. -Validation uses the Codex binary on this host; it does not verify a desktop -app's separate bundled binary on another machine. +`ug codex` validates discovered models with the installed Codex binary and publishes +them to `~/.ucode/codex-model-catalog.json`, referenced by shared `~/.codex/config.toml` +for Codex App. Managed static lists use the same path during `ug configure`. The +latest refresh supplies the app's catalog; custom catalogs (including Isaac's) and +custom providers are preserved. The app's gateway provider and authentication must +already be configured. Validation covers the local Codex binary. + +Codex loads the catalog at app-server startup. When ug reports a catalog change, +finish active tasks, restart the app server on the **connected host**, then reconnect. +Use `codex app-server daemon restart` for a standalone managed daemon; otherwise +restart the process or application that owns the server. Reconnecting or reopening +the desktop app can reuse a remote server with the old list. + +ug removes its shared reference on discovery/validation failure, reconfiguration, +revert, or before installing/updating Codex. After an update, run `ug codex` to refresh +discovery or `ug configure` for a managed static list, then restart the app server. ## Configure diff --git a/src/ucode/agents/__init__.py b/src/ucode/agents/__init__.py index 5a62d9108..7013cc2ee 100644 --- a/src/ucode/agents/__init__.py +++ b/src/ucode/agents/__init__.py @@ -135,9 +135,7 @@ def _update_installed_tool_binary(tool: str, version: str | None = None) -> bool print_note(f"Upgrading {spec['display']}...") if tool == "codex": - # Codex's update command and npm replacement can invalidate the metadata - # referenced by the desktop app. Detach before mutating the binary and - # leave the reference detached until a later validated catalog publish. + # Detach potentially incompatible metadata until the next validated refresh. codex.detach_app_model_catalog() try: subprocess.run(command, check=True, timeout=300) @@ -230,8 +228,6 @@ def install_tool_binary( print_section("Bootstrap") print_warning(f"`{binary}` was not found. Installing {spec['display']}...") if tool == "codex": - # The newly installed binary may not understand the previous catalog. - # Detachment must happen before npm mutates the installation. codex.detach_app_model_catalog() try: subprocess.run(["npm", "install", "-g", package], check=True, timeout=300) diff --git a/src/ucode/agents/codex.py b/src/ucode/agents/codex.py index 32cb4817a..04a2f2ef2 100644 --- a/src/ucode/agents/codex.py +++ b/src/ucode/agents/codex.py @@ -832,50 +832,15 @@ def _read_app_config() -> tomlkit.TOMLDocument: raise RuntimeError(f"Cannot update Codex App settings at {path}: {exc}") from exc -def _install_app_catalog_reference() -> bool: - """Point Codex App at ucode's stable catalog without replacing a user catalog.""" - if is_dry_run(): - return False - path = _legacy_config_path() - doc = _read_app_config() - existing = doc.get("model_catalog_json") - if existing is not None and not _is_ucode_catalog_reference(existing): - print_warning_err( - f"Codex App already uses the custom model catalog {existing}; leaving it unchanged." - ) - return False - provider = doc.get("model_provider") - if provider not in (None, CODEX_MODEL_PROVIDER_NAME, LEGACY_CODEX_MODEL_PROVIDER_NAME): - if _is_ucode_catalog_reference(existing): - doc.pop("model_catalog_json", None) - write_toml_file(path, doc) - changed = True - else: - changed = False - print_warning_err( - f"Codex App uses the custom provider {provider}; leaving its model catalog unmanaged." - ) - return changed - catalog_path = str(CODEX_MODEL_CATALOG_PATH) - if existing == catalog_path: - return False - doc["model_catalog_json"] = catalog_path - write_toml_file(path, doc) - return True - - def detach_app_model_catalog() -> bool: """Remove only a shared catalog reference owned by ucode.""" if is_dry_run(): return False - path = _legacy_config_path() - if not path.exists(): - return False doc = _read_app_config() if not _is_ucode_catalog_reference(doc.get("model_catalog_json")): return False doc.pop("model_catalog_json", None) - write_toml_file(path, doc) + write_toml_file(_legacy_config_path(), doc) _print_app_catalog_restart_notice() return True @@ -907,13 +872,30 @@ def _sync_app_model_catalog(catalog: dict) -> None: doc = _read_app_config() catalog_changed = read_json_safe(CODEX_MODEL_CATALOG_PATH) != catalog _write_model_catalog(CODEX_MODEL_CATALOG_PATH, catalog) - reference_changed = _install_app_catalog_reference() - app_uses_catalog = reference_changed or ( - _is_ucode_catalog_reference(doc.get("model_catalog_json")) - and doc.get("model_provider") - in (None, CODEX_MODEL_PROVIDER_NAME, LEGACY_CODEX_MODEL_PROVIDER_NAME) - ) - if reference_changed or (app_uses_catalog and catalog_changed): + + existing = doc.get("model_catalog_json") + if existing is not None and not _is_ucode_catalog_reference(existing): + print_warning_err( + f"Codex App already uses the custom model catalog {existing}; leaving it unchanged." + ) + return + + catalog_path = str(CODEX_MODEL_CATALOG_PATH) + provider = doc.get("model_provider") + if provider not in (None, CODEX_MODEL_PROVIDER_NAME, LEGACY_CODEX_MODEL_PROVIDER_NAME): + print_warning_err( + f"Codex App uses the custom provider {provider}; leaving its model catalog unmanaged." + ) + catalog_path = None + + reference_changed = existing != catalog_path + if reference_changed: + if catalog_path is None: + doc.pop("model_catalog_json", None) + else: + doc["model_catalog_json"] = catalog_path + write_toml_file(_legacy_config_path(), doc) + if reference_changed or (catalog_path is not None and catalog_changed): _print_app_catalog_restart_notice() diff --git a/src/ucode/agents/codex_catalog.py b/src/ucode/agents/codex_catalog.py index 63e0f44d3..179552155 100644 --- a/src/ucode/agents/codex_catalog.py +++ b/src/ucode/agents/codex_catalog.py @@ -15,16 +15,6 @@ from ucode.ui import print_warning _TIMEOUT_SECONDS = 30 -_CATALOG_VALIDATION_ERROR = ( - "Could not load the Codex model catalog with the selected Codex binary. " - "Update Codex and retry `ug codex`." -) -_PREPARE_CATALOG_ERROR = ( - "Could not build the managed Codex model catalog locally. " - "Upgrade the active Codex installation and verify " - "`codex debug models --bundled` works, then retry configuration. " - "Gateway discovery was not used." -) # Known hosted-model capabilities, following universe#2591694. These are not # universal non-GPT defaults: an arbitrary UC service need not support images, @@ -135,11 +125,11 @@ def build_codex_catalog( def _run_catalog_command( - binary: str, args: list[str], *, home: str, env: dict[str, str] + binary: str, args: list[str], home: str ) -> subprocess.CompletedProcess[str]: return subprocess.run( [binary, *args], - env=env, + env={**os.environ, "CODEX_HOME": home}, cwd=home, stdin=subprocess.DEVNULL, capture_output=True, @@ -152,7 +142,6 @@ def _run_catalog_command( def _validate_codex_catalog_in_home(binary: str, catalog: dict, home: str) -> None: candidate = Path(home) / "catalog.json" candidate.write_text(json.dumps(catalog), encoding="utf-8") - env = {**os.environ, "CODEX_HOME": home} _run_catalog_command( binary, [ @@ -161,8 +150,7 @@ def _validate_codex_catalog_in_home(binary: str, catalog: dict, home: str) -> No "debug", "models", ], - home=home, - env=env, + home, ) @@ -172,7 +160,10 @@ def validate_codex_catalog(binary: str, catalog: dict) -> None: with tempfile.TemporaryDirectory(prefix="ug-codex-catalog-") as home: _validate_codex_catalog_in_home(binary, catalog, home) except (OSError, TypeError, ValueError, subprocess.SubprocessError): - raise RuntimeError(_CATALOG_VALIDATION_ERROR) from None + raise RuntimeError( + "Could not load the Codex model catalog with the selected Codex binary. " + "Update Codex and retry `ug codex`." + ) from None def prepare_codex_catalog(binary: str, names: list[str]) -> dict: @@ -184,10 +175,7 @@ def prepare_codex_catalog(binary: str, names: list[str]) -> dict: """ try: with tempfile.TemporaryDirectory(prefix="ug-codex-catalog-") as home: - env = {**os.environ, "CODEX_HOME": home} - result = _run_catalog_command( - binary, ["debug", "models", "--bundled"], home=home, env=env - ) + result = _run_catalog_command(binary, ["debug", "models", "--bundled"], home) payload = json.loads(result.stdout) bundled = payload.get("models") if isinstance(payload, dict) else None if ( @@ -203,7 +191,12 @@ def prepare_codex_catalog(binary: str, names: list[str]) -> dict: catalog = build_codex_catalog(bundled, names, warn=fallback_warnings.append) _validate_codex_catalog_in_home(binary, catalog, home) except (OSError, TypeError, ValueError, subprocess.SubprocessError): - raise RuntimeError(_PREPARE_CATALOG_ERROR) from None + raise RuntimeError( + "Could not build the managed Codex model catalog locally. " + "Upgrade the active Codex installation and verify " + "`codex debug models --bundled` works, then retry configuration. " + "Gateway discovery was not used." + ) from None for message in fallback_warnings: print_warning(message) return catalog diff --git a/tests/test_agent_codex.py b/tests/test_agent_codex.py index ab7a7b1c2..1122de80c 100644 --- a/tests/test_agent_codex.py +++ b/tests/test_agent_codex.py @@ -758,31 +758,39 @@ def test_returns_false_when_no_shared_config(self, tmp_path, monkeypatch): assert codex.revert_legacy_shared_config() is False - def test_strips_ucode_app_catalog_reference(self, tmp_path, monkeypatch): - config_dir = tmp_path / ".codex" - config_dir.mkdir() - profile_path = config_dir / "ucode.config.toml" - shared_path = config_dir / "config.toml" - catalog_path = tmp_path / ".ucode" / "codex-model-catalog.json" - catalog_path.parent.mkdir(exist_ok=True) + def test_strips_ucode_app_catalog_reference(self, capsys): + shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" + shared_path.parent.mkdir() + catalog_path = codex.CODEX_MODEL_CATALOG_PATH catalog_path.write_text("{}", encoding="utf-8") shared_path.write_text( f'model_catalog_json = "{catalog_path}"\npersonality = "friendly"\n', encoding="utf-8", ) - monkeypatch.setattr(codex, "CODEX_CONFIG_PATH", profile_path) - monkeypatch.setattr(codex, "CODEX_MODEL_CATALOG_PATH", catalog_path) - assert codex.revert_legacy_shared_config() is True assert read_toml_safe(shared_path) == {"personality": "friendly"} assert not catalog_path.exists() + assert "codex app-server daemon restart" in " ".join(capsys.readouterr().err.split()) class TestCodexAppCatalog: - def test_catalog_changes_report_server_restart_on_stderr(self, capsys): + def test_publish_refresh_and_reattach_preserve_settings_and_report_restart(self, capsys): + shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" + shared_path.parent.mkdir() + original = '# User settings\nmodel = "gpt-user"\n' + shared_path.write_text(original, encoding="utf-8") first_catalog = {"models": [{"slug": "first-model"}]} + second_catalog = {"models": [{"slug": "second-model"}]} + codex._sync_app_model_catalog(first_catalog) + first_config = shared_path.read_text() + assert "# User settings" in first_config + assert read_toml_safe(shared_path) == { + "model": "gpt-user", + "model_catalog_json": str(codex.CODEX_MODEL_CATALOG_PATH), + } + assert json.loads(codex.CODEX_MODEL_CATALOG_PATH.read_text()) == first_catalog published = capsys.readouterr() assert published.out == "" assert "codex app-server daemon restart" in " ".join(published.err.split()) @@ -792,56 +800,18 @@ def test_catalog_changes_report_server_restart_on_stderr(self, capsys): unchanged = capsys.readouterr() assert unchanged.out == unchanged.err == "" - codex._sync_app_model_catalog({"models": [{"slug": "second-model"}]}) + codex._sync_app_model_catalog(second_catalog) + assert shared_path.read_text() == first_config + assert json.loads(codex.CODEX_MODEL_CATALOG_PATH.read_text()) == second_catalog refreshed = capsys.readouterr() assert refreshed.out == "" assert "codex app-server daemon restart" in " ".join(refreshed.err.split()) - def test_attaching_existing_catalog_reports_server_restart(self, capsys): - catalog = {"models": [{"slug": "first-model"}]} - codex._write_model_catalog(codex.CODEX_MODEL_CATALOG_PATH, catalog) - - codex._sync_app_model_catalog(catalog) - - assert "codex app-server daemon restart" in " ".join(capsys.readouterr().err.split()) - - def test_detaching_catalog_reports_server_restart(self, capsys): - codex._sync_app_model_catalog({"models": [{"slug": "first-model"}]}) - capsys.readouterr() - - assert codex.detach_app_model_catalog() - - assert "codex app-server daemon restart" in " ".join(capsys.readouterr().err.split()) - - def test_custom_app_catalog_does_not_report_ug_restart(self, capsys): - shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" - shared_path.parent.mkdir() - shared_path.write_text('model_catalog_json = "/user/models.json"\n') - - codex._sync_app_model_catalog({"models": [{"slug": "gateway-model"}]}) - - output = capsys.readouterr() - assert "leaving it unchanged" in " ".join(output.err.split()) - assert "daemon restart" not in output.err - - def test_refresh_keeps_stable_pointer_and_preserves_settings(self, tmp_path): - shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" - shared_path.parent.mkdir() - shared_path.write_text('# User settings\nmodel = "gpt-user"\n', encoding="utf-8") - first_catalog = {"models": [{"slug": "first-model"}]} - second_catalog = {"models": [{"slug": "second-model"}]} - - codex._sync_app_model_catalog(first_catalog) - first_config = shared_path.read_text() + # Reattaching an unchanged catalog also requires a server restart. + shared_path.write_text(original, encoding="utf-8") codex._sync_app_model_catalog(second_catalog) - assert shared_path.read_text() == first_config - assert "# User settings" in first_config - assert read_toml_safe(shared_path) == { - "model": "gpt-user", - "model_catalog_json": str(codex.CODEX_MODEL_CATALOG_PATH), - } - assert json.loads(codex.CODEX_MODEL_CATALOG_PATH.read_text()) == second_catalog + assert "codex app-server daemon restart" in " ".join(capsys.readouterr().err.split()) def test_invalid_shared_config_is_not_overwritten(self): shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" @@ -1071,8 +1041,9 @@ def test_provider_discovery_uses_authoritative_catalog(self, tmp_path, monkeypat launches = self._patch(tmp_path, monkeypatch) catalog_path = tmp_path / "models.json" app_catalog_path = tmp_path / "codex-model-catalog.json" - catalog = {"models": [{"slug": "gpt-mps"}]} + catalog = {"models": [{"slug": "gpt-mps", "future_metadata": {"tools": True}}]} fetch_kwargs = {} + validations = [] monkeypatch.setattr(codex, "CODEX_MODEL_CATALOG_PATH", app_catalog_path) monkeypatch.setattr(codex, "_model_catalog_path", lambda workspace, provider: catalog_path) monkeypatch.setattr( @@ -1081,13 +1052,20 @@ def test_provider_discovery_uses_authoritative_catalog(self, tmp_path, monkeypat lambda workspace, token, **kwargs: fetch_kwargs.update(kwargs) or catalog, ) + def validate(binary, candidate): + assert not catalog_path.exists() + assert not app_catalog_path.exists() + validations.append((binary, candidate)) + + monkeypatch.setattr(codex, "validate_codex_catalog", validate) codex.launch( {"workspace": WS, "_codex_launch_provider": "main.default.openai"}, [], options=LaunchOptions(), ) - assert catalog_path.exists() + assert validations == [(codex.SPEC["binary"], catalog)] + assert json.loads(catalog_path.read_text()) == catalog assert json.loads(app_catalog_path.read_text()) == catalog assert read_toml_safe(tmp_path / "config.toml")["model_catalog_json"] == str( app_catalog_path @@ -1105,30 +1083,6 @@ def test_provider_discovery_uses_authoritative_catalog(self, tmp_path, monkeypat ) assert 'Databricks-Model-Provider-Service = "main.default.openai"' in provider_arg - def test_discovery_is_validated_before_catalogs_are_published(self, tmp_path, monkeypatch): - launches = self._patch(tmp_path, monkeypatch) - catalog = {"models": [{"slug": "gpt-mps", "future_metadata": {"tools": True}}]} - monkeypatch.setattr(codex, "_fetch_codex_model_catalog", lambda *a, **k: catalog) - validations = [] - - def validate(binary, candidate): - assert not codex.CODEX_MODEL_CATALOG_PATH.exists() - assert not list( - codex.CODEX_MODEL_CATALOG_PATH.parent.glob("codex-model-catalog-*.json") - ) - validations.append((binary, candidate)) - - monkeypatch.setattr(codex, "validate_codex_catalog", validate) - codex.launch( - {"workspace": WS, "_codex_launch_provider": "main.default.openai"}, - [], - options=LaunchOptions(), - ) - - assert validations == [(codex.SPEC["binary"], catalog)] - assert json.loads(codex.CODEX_MODEL_CATALOG_PATH.read_text()) == catalog - assert launches - @pytest.mark.parametrize("custom_catalog", [None, "/user/isaac-app-model-catalog.json"]) def test_incompatible_discovery_removes_only_ug_reference( self, tmp_path, monkeypatch, custom_catalog @@ -1182,7 +1136,9 @@ def test_provider_discovery_preserves_custom_app_catalog(self, tmp_path, monkeyp assert launches assert read_toml_safe(shared_path)["model_catalog_json"] == "/user/models.json" - assert "leaving it unchanged" in " ".join(capsys.readouterr().err.split()) + output = capsys.readouterr() + assert "leaving it unchanged" in " ".join(output.err.split()) + assert "daemon restart" not in output.err def test_provider_pins_first_catalog_model(self, tmp_path, monkeypatch): launches = self._patch(tmp_path, monkeypatch) diff --git a/tests/test_agents_init.py b/tests/test_agents_init.py index e5f7955d0..1c731103a 100644 --- a/tests/test_agents_init.py +++ b/tests/test_agents_init.py @@ -692,10 +692,14 @@ def fake_run(args, **kwargs): assert agents_mod._update_installed_tool_binary("codex") is True assert calls == [["codex", "update"]] - def test_codex_update_failure_leaves_ug_catalog_detached(self, monkeypatch): + @pytest.mark.parametrize("installed", [False, True], ids=["install", "update"]) + def test_codex_install_or_update_failure_leaves_catalog_detached(self, monkeypatch, installed): shared_path = self._seed_codex_catalog_reference() - monkeypatch.setattr("ucode.agents.shutil.which", lambda binary: f"/usr/bin/{binary}") + monkeypatch.setattr( + "ucode.agents.shutil.which", + lambda binary: f"/usr/bin/{binary}" if installed or binary == "npm" else None, + ) monkeypatch.setattr("ucode.agents._minimum_version_error", lambda _: "must upgrade") monkeypatch.setattr("ucode.agents._too_new_downgrade", lambda _: None) @@ -705,23 +709,10 @@ def fail_run(args, **kwargs): monkeypatch.setattr("ucode.agents.subprocess.run", fail_run) - assert agents_mod._update_installed_tool_binary("codex") is False - assert "model_catalog_json" not in shared_path.read_text(encoding="utf-8") - - def test_codex_install_failure_leaves_ug_catalog_detached(self, monkeypatch): - shared_path = self._seed_codex_catalog_reference() - - def fake_which(binary: str) -> str | None: - return "/usr/bin/npm" if binary == "npm" else None - - def fail_run(args, **kwargs): - assert "model_catalog_json" not in shared_path.read_text(encoding="utf-8") - raise subprocess.CalledProcessError(1, args) - - monkeypatch.setattr("ucode.agents.shutil.which", fake_which) - monkeypatch.setattr("ucode.agents.subprocess.run", fail_run) - - assert install_tool_binary("codex", strict=False) is False + if installed: + assert agents_mod._update_installed_tool_binary("codex") is False + else: + assert install_tool_binary("codex", strict=False) is False assert "model_catalog_json" not in shared_path.read_text(encoding="utf-8") def test_catalog_detach_failure_blocks_codex_binary_mutation(self, monkeypatch): From f96781b6b584c0fb0191c91bd23867ffee2310a5 Mon Sep 17 00:00:00 2001 From: Andy Xu Date: Tue, 22 Sep 2026 17:14:47 +0000 Subject: [PATCH 5/5] Make Codex app catalog sync helper public --- src/ucode/agents/codex.py | 6 +++--- tests/test_agent_codex.py | 30 +++++++++++++++--------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/ucode/agents/codex.py b/src/ucode/agents/codex.py index 04a2f2ef2..595f894b0 100644 --- a/src/ucode/agents/codex.py +++ b/src/ucode/agents/codex.py @@ -497,7 +497,7 @@ def compose(base: dict, *, include_catalog: bool = True) -> dict: return base if catalog is not None: - _sync_app_model_catalog(catalog) + sync_app_model_catalog(catalog) elif not is_dry_run(): detach_app_model_catalog() if CODEX_MODEL_CATALOG_PATH.exists(): @@ -865,7 +865,7 @@ def _print_app_catalog_restart_notice() -> None: ) -def _sync_app_model_catalog(catalog: dict) -> None: +def sync_app_model_catalog(catalog: dict) -> None: """Publish a validated catalog without overwriting unreadable app settings.""" if is_dry_run(): return @@ -1104,7 +1104,7 @@ def launch( else: catalog_path = _model_catalog_path(workspace, catalog_scope) _write_model_catalog(catalog_path, catalog) - _sync_app_model_catalog(catalog) + sync_app_model_catalog(catalog) profile_doc["model_catalog_json"] = str(catalog_path) # Codex otherwise boots on its bundled default model (e.g. gpt-5.6-sol), # which an MPS's allowlist doesn't route, so the first request 403s. Pin diff --git a/tests/test_agent_codex.py b/tests/test_agent_codex.py index 1122de80c..618b845a5 100644 --- a/tests/test_agent_codex.py +++ b/tests/test_agent_codex.py @@ -783,7 +783,7 @@ def test_publish_refresh_and_reattach_preserve_settings_and_report_restart(self, first_catalog = {"models": [{"slug": "first-model"}]} second_catalog = {"models": [{"slug": "second-model"}]} - codex._sync_app_model_catalog(first_catalog) + codex.sync_app_model_catalog(first_catalog) first_config = shared_path.read_text() assert "# User settings" in first_config assert read_toml_safe(shared_path) == { @@ -796,11 +796,11 @@ def test_publish_refresh_and_reattach_preserve_settings_and_report_restart(self, assert "codex app-server daemon restart" in " ".join(published.err.split()) assert "After active tasks finish" in published.err - codex._sync_app_model_catalog(first_catalog) + codex.sync_app_model_catalog(first_catalog) unchanged = capsys.readouterr() assert unchanged.out == unchanged.err == "" - codex._sync_app_model_catalog(second_catalog) + codex.sync_app_model_catalog(second_catalog) assert shared_path.read_text() == first_config assert json.loads(codex.CODEX_MODEL_CATALOG_PATH.read_text()) == second_catalog refreshed = capsys.readouterr() @@ -809,7 +809,7 @@ def test_publish_refresh_and_reattach_preserve_settings_and_report_restart(self, # Reattaching an unchanged catalog also requires a server restart. shared_path.write_text(original, encoding="utf-8") - codex._sync_app_model_catalog(second_catalog) + codex.sync_app_model_catalog(second_catalog) assert shared_path.read_text() == first_config assert "codex app-server daemon restart" in " ".join(capsys.readouterr().err.split()) @@ -821,7 +821,7 @@ def test_invalid_shared_config_is_not_overwritten(self): codex.CODEX_MODEL_CATALOG_PATH.write_text("previous catalog", encoding="utf-8") with pytest.raises(RuntimeError, match="Cannot update Codex App settings"): - codex._sync_app_model_catalog({"models": [{"slug": "gpt-mps"}]}) + codex.sync_app_model_catalog({"models": [{"slug": "gpt-mps"}]}) assert shared_path.read_text() == original assert codex.CODEX_MODEL_CATALOG_PATH.read_text() == "previous catalog" @@ -829,7 +829,7 @@ def test_invalid_shared_config_is_not_overwritten(self): @pytest.mark.parametrize("previous_catalog", [False, True]) def test_custom_provider_keeps_its_own_model_discovery(self, previous_catalog, capsys): if previous_catalog: - codex._sync_app_model_catalog({"models": [{"slug": "previous"}]}) + codex.sync_app_model_catalog({"models": [{"slug": "previous"}]}) shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" shared_path.parent.mkdir(exist_ok=True) original = '# User settings\nmodel_provider = "custom"\nmodel = "user-model"\n' @@ -844,7 +844,7 @@ def test_custom_provider_keeps_its_own_model_discovery(self, previous_catalog, c ) capsys.readouterr() - codex._sync_app_model_catalog({"models": [{"slug": "gpt-mps"}]}) + codex.sync_app_model_catalog({"models": [{"slug": "gpt-mps"}]}) assert shared_path.read_text() == original output = capsys.readouterr().err @@ -854,19 +854,19 @@ def test_custom_provider_keeps_its_own_model_discovery(self, previous_catalog, c assert "daemon restart" not in output def test_dry_run_leaves_config_and_catalog_unchanged(self, monkeypatch): - codex._sync_app_model_catalog({"models": [{"slug": "previous"}]}) + codex.sync_app_model_catalog({"models": [{"slug": "previous"}]}) shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" original = shared_path.read_bytes() catalog_before = codex.CODEX_MODEL_CATALOG_PATH.read_bytes() monkeypatch.setattr(codex, "is_dry_run", lambda: True) - codex._sync_app_model_catalog({"models": [{"slug": "new"}]}) + codex.sync_app_model_catalog({"models": [{"slug": "new"}]}) assert shared_path.read_bytes() == original assert codex.CODEX_MODEL_CATALOG_PATH.read_bytes() == catalog_before def test_reconfigure_clears_discovered_app_catalog(self, monkeypatch): - codex._sync_app_model_catalog({"models": [{"slug": "previous-workspace"}]}) + codex.sync_app_model_catalog({"models": [{"slug": "previous-workspace"}]}) monkeypatch.setattr(codex, "agent_version", lambda _: "0.154.0") monkeypatch.setattr(codex, "save_state", lambda _: None) @@ -877,7 +877,7 @@ def test_reconfigure_clears_discovered_app_catalog(self, monkeypatch): assert not codex.CODEX_MODEL_CATALOG_PATH.exists() def test_failed_static_validation_detaches_previous_app_catalog(self, monkeypatch): - codex._sync_app_model_catalog({"models": [{"slug": "old-model"}]}) + codex.sync_app_model_catalog({"models": [{"slug": "old-model"}]}) original_catalog = codex.CODEX_MODEL_CATALOG_PATH.read_bytes() monkeypatch.setattr(codex, "agent_version", lambda _: "0.154.0") @@ -894,7 +894,7 @@ def reject(*args): assert codex.CODEX_MODEL_CATALOG_PATH.read_bytes() == original_catalog def test_revert_preserves_subsequent_user_catalog(self, tmp_path): - codex._sync_app_model_catalog({"models": [{"slug": "gpt-mps"}]}) + codex.sync_app_model_catalog({"models": [{"slug": "gpt-mps"}]}) shared_path = codex.CODEX_CONFIG_PATH.parent / "config.toml" original = 'model_catalog_json = "/user/models.json"\n' shared_path.write_text(original, encoding="utf-8") @@ -1001,7 +1001,7 @@ def test_native_update_detaches_catalog_without_discovery( self, tmp_path, monkeypatch, custom_catalog ): self._patch(tmp_path, monkeypatch) - codex._sync_app_model_catalog({"models": [{"slug": "old-model"}]}) + codex.sync_app_model_catalog({"models": [{"slug": "old-model"}]}) shared_path = tmp_path / "config.toml" if custom_catalog: shared_path.write_text(f'model_catalog_json = "{custom_catalog}"\n') @@ -1088,7 +1088,7 @@ def test_incompatible_discovery_removes_only_ug_reference( self, tmp_path, monkeypatch, custom_catalog ): launches = self._patch(tmp_path, monkeypatch) - codex._sync_app_model_catalog({"models": [{"slug": "old-model"}]}) + codex.sync_app_model_catalog({"models": [{"slug": "old-model"}]}) original_catalog = codex.CODEX_MODEL_CATALOG_PATH.read_bytes() shared_path = tmp_path / "config.toml" if custom_catalog: @@ -1306,7 +1306,7 @@ def test_provider_launches_when_discovery_is_unavailable( self, tmp_path, monkeypatch, tool_args ): launches = self._patch(tmp_path, monkeypatch) - codex._sync_app_model_catalog({"models": [{"slug": "previous-workspace"}]}) + codex.sync_app_model_catalog({"models": [{"slug": "previous-workspace"}]}) monkeypatch.setattr( codex, "_fetch_codex_model_catalog",