From 6b51280cda903793c4b29733c27ee1cdbede4962 Mon Sep 17 00:00:00 2001 From: Rohit Agrawal Date: Mon, 14 Sep 2026 12:16:20 -0400 Subject: [PATCH] Preserve app-server stdout and generated-config cleanup --- src/ucode/agents/claude.py | 8 +++++-- src/ucode/agents/codex.py | 13 ++++++++--- src/ucode/cli.py | 15 +++++++++++++ src/ucode/state.py | 10 +++++++++ src/ucode/ui.py | 13 +++++++++++ tests/test_agent_claude.py | 37 ++++++++++++++++++++++++++++++ tests/test_agent_codex.py | 46 ++++++++++++++++++++++++++++++++++++++ tests/test_cli.py | 25 +++++++++++++++++++++ 8 files changed, 162 insertions(+), 5 deletions(-) diff --git a/src/ucode/agents/claude.py b/src/ucode/agents/claude.py index 61e24fea4..b5cc72071 100644 --- a/src/ucode/agents/claude.py +++ b/src/ucode/agents/claude.py @@ -54,7 +54,7 @@ remove_smart_routing_hooks, sync_smart_routing_hooks, ) -from ucode.state import MANAGED_OVERLAY_KEY, mark_tool_managed, save_state +from ucode.state import MANAGED_OVERLAY_KEY, is_tool_managed, mark_tool_managed, save_state from ucode.telemetry import agent_version, ucode_version from ucode.tracing import tracing_env from ucode.ui import print_note, print_success, print_warning @@ -585,7 +585,11 @@ def write_tool_config( coding_agent_config_defaults: dict[str, str] | None = None, parent_schema: str | None = None, ) -> dict: - backup_existing_file(CLAUDE_SETTINGS_PATH, CLAUDE_BACKUP_PATH) + # Back up only a file that predates ucode's management of the tool. A + # re-configure would otherwise snapshot ucode's own generated file, and + # revert would restore that snapshot instead of deleting the file. + if not is_tool_managed(state, "claude"): + backup_existing_file(CLAUDE_SETTINGS_PATH, CLAUDE_BACKUP_PATH) web_search_model = _resolve_web_search_model(state) # Relayed inference points at a local refresh proxy; its loopback base URL is # recorded in state so launch starts the proxy on the matching port. diff --git a/src/ucode/agents/codex.py b/src/ucode/agents/codex.py index 09de63612..5e69d849d 100644 --- a/src/ucode/agents/codex.py +++ b/src/ucode/agents/codex.py @@ -63,7 +63,7 @@ sync_smart_routing_hooks, ) from ucode.smart_routing.codex_routing import codex_model_id -from ucode.state import get_provider_service, mark_tool_managed, save_state +from ucode.state import get_provider_service, is_tool_managed, mark_tool_managed, save_state from ucode.telemetry import agent_version, ucode_version from ucode.ui import print_warning_err @@ -399,7 +399,11 @@ def write_tool_config( return state _remove_legacy_ucode_profile() - backup_existing_file(CODEX_CONFIG_PATH, CODEX_BACKUP_PATH) + # Back up only a file that predates ucode's management of the tool. A + # re-configure would otherwise snapshot ucode's own generated file, and + # revert would restore that snapshot instead of deleting the file. + if not is_tool_managed(state, "codex"): + backup_existing_file(CODEX_CONFIG_PATH, CODEX_BACKUP_PATH) overlay = render_overlay( workspace, chosen_model, @@ -572,7 +576,10 @@ def clear_model_preferences(state: dict) -> bool: doc.pop(key) changed = True if changed: - backup_existing_file(CODEX_CONFIG_PATH, CODEX_BACKUP_PATH) + # Never snapshot ucode's own generated file here; revert would restore + # the snapshot instead of deleting the file. + if not is_tool_managed(state, "codex"): + backup_existing_file(CODEX_CONFIG_PATH, CODEX_BACKUP_PATH) write_toml_file(CODEX_CONFIG_PATH, doc) return changed diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 9a04223b5..679954928 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -140,6 +140,7 @@ prompt_for_tools, prompt_for_workspace, prompt_yes_no, + redirect_output_to_stderr, set_verbosity, spinner, status_badge, @@ -1986,6 +1987,16 @@ def _download_managed_skills(managed: dict, state: dict) -> None: print_note(f"Downloaded workspace skill(s) to disk: {', '.join(written)}") +def _child_owns_stdout(tool: str, tool_args: list[str]) -> bool: + """True when the forwarded agent command speaks a stdio protocol on stdout. + + ``codex app-server`` puts its JSON-RPC stream on stdout, so ug's status + output must move to stderr for that launch; the file descriptor stays + untouched for the agent process. + """ + return tool == "codex" and tool_args[:1] == ["app-server"] + + def _should_launch_smart_routing( tool: str, tool_args: list[str], @@ -2049,6 +2060,10 @@ def _launch_tool( ) -> None: try: tool = normalize_tool(tool_name) + # Before any status print: a stdio-protocol subcommand owns stdout, so + # every ug line from here on must go to stderr instead. + if _child_owns_stdout(tool, ctx.args): + redirect_output_to_stderr() if provider is not None and parent_schema is not None: raise RuntimeError("--provider and --parent cannot be used together.") if parent_schema is not None and not is_valid_catalog_schema(parent_schema): diff --git a/src/ucode/state.py b/src/ucode/state.py index c02855537..e37f60e3e 100644 --- a/src/ucode/state.py +++ b/src/ucode/state.py @@ -251,6 +251,16 @@ def clear_state() -> None: raise RuntimeError(f"Failed to clear state file: {STATE_PATH}") from exc +def is_tool_managed(state: dict, tool: str) -> bool: + """True once configure has written ucode's config for ``tool``. + + Used to distinguish a config file the user owned before ucode ran from one + ucode itself generated: only the former is backup-worthy, because revert + restores a backup in place instead of deleting ucode's generated file. + """ + return bool((state.get("managed_configs") or {}).get(tool)) + + def mark_tool_managed(state: dict, tool: str, managed_keys: list) -> dict: """Record which config keys ucode manages for ``tool``.""" managed_configs = dict(state.get("managed_configs") or {}) diff --git a/src/ucode/ui.py b/src/ucode/ui.py index ce92bcbcf..c2c4af57c 100644 --- a/src/ucode/ui.py +++ b/src/ucode/ui.py @@ -24,6 +24,19 @@ console = Console(highlight=False) err_console = Console(stderr=True, highlight=False) + +def redirect_output_to_stderr() -> None: + """Move status output to stderr because a child process now owns stdout. + + ``codex app-server`` speaks its JSON-RPC protocol on stdout, so any ug line + printed there corrupts the stream. Rich resolves ``console.file`` to + ``sys.stdout`` at print time, so rebinding the module attribute sends every + status print to stderr while ``os.execvp`` still hands the launched agent + the untouched stdout file descriptor. + """ + sys.stdout = sys.stderr + + # Past this many options the choice list is pinned to a fixed-height scrolling viewport (see # `_cap_choice_viewport`) rather than growing to fill the terminal, and the pickers append a # "↑/↓ scroll" note to their instruction line. The value is both the boundary and the number of diff --git a/tests/test_agent_claude.py b/tests/test_agent_claude.py index 23826da34..4a8a0ac14 100644 --- a/tests/test_agent_claude.py +++ b/tests/test_agent_claude.py @@ -1727,3 +1727,40 @@ def test_missing_login_runs_browser_flow(self, monkeypatch): monkeypatch.setattr(claude, "print_success", lambda *a, **kw: None) claude._ensure_subscription_login() assert calls == [[claude.SPEC["binary"], "auth", "login"]] + + +class TestWriteToolConfigBackup: + """A re-configure must not snapshot the file ucode itself generated.""" + + def _patch(self, monkeypatch, tmp_path): + monkeypatch.setattr(claude, "CLAUDE_SETTINGS_PATH", tmp_path / "ucode-settings.json") + monkeypatch.setattr(claude, "CLAUDE_BACKUP_PATH", tmp_path / "backup.json") + monkeypatch.setattr("ucode.config_io.APP_DIR", tmp_path) + monkeypatch.setattr(claude, "save_state", lambda state: None) + monkeypatch.setattr(claude, "_register_web_search_mcp", lambda *a, **kw: None) + + def test_first_configure_backs_up_user_owned_file(self, tmp_path, monkeypatch): + self._patch(monkeypatch, tmp_path) + (tmp_path / "ucode-settings.json").write_text( + '{"permissions": {"allow": ["Read"]}}', encoding="utf-8" + ) + + claude.write_tool_config( + {"workspace": WS, "claude_models": {}}, "databricks-claude-sonnet-4" + ) + + backup = (tmp_path / "backup.json").read_text(encoding="utf-8") + assert backup == '{"permissions": {"allow": ["Read"]}}' + + def test_reconfigure_does_not_back_up_generated_file(self, tmp_path, monkeypatch): + self._patch(monkeypatch, tmp_path) + state = { + "workspace": WS, + "claude_models": {}, + # load_state after a first configure: ucode already manages this file. + "managed_configs": {"claude": {"keys": [["env", "ANTHROPIC_BASE_URL"]]}}, + } + + claude.write_tool_config(state, "databricks-claude-sonnet-4") + + assert not (tmp_path / "backup.json").exists() diff --git a/tests/test_agent_codex.py b/tests/test_agent_codex.py index 07162a5fa..7185cfd20 100644 --- a/tests/test_agent_codex.py +++ b/tests/test_agent_codex.py @@ -1065,3 +1065,49 @@ def deny_managed_write(*args, **kwargs): with pytest.raises(managed_files.ManagedFileWriteUnavailable, match="sudo denied"): codex.write_tool_config({"workspace": WS, "codex_models": ["gpt-5"]}) + + +class TestWriteConfigBackup: + """A re-configure must not snapshot the file ucode itself generated.""" + + def _patch(self, monkeypatch, tmp_path): + monkeypatch.setattr(codex, "CODEX_CONFIG_PATH", tmp_path / "ucode.config.toml") + monkeypatch.setattr(codex, "CODEX_BACKUP_PATH", tmp_path / "backup.toml") + monkeypatch.setattr("ucode.config_io.APP_DIR", tmp_path) + monkeypatch.setattr(codex, "agent_version", lambda binary: "0.134.0") + monkeypatch.setattr(codex, "save_state", lambda state: None) + + def test_first_configure_backs_up_user_owned_profile(self, tmp_path, monkeypatch): + self._patch(monkeypatch, tmp_path) + (tmp_path / "ucode.config.toml").write_text("# user comment\n", encoding="utf-8") + + codex.write_tool_config({"workspace": WS, "codex_models": ["gpt-5"]}) + + assert (tmp_path / "backup.toml").read_text(encoding="utf-8") == "# user comment\n" + + def test_reconfigure_does_not_back_up_generated_profile(self, tmp_path, monkeypatch): + self._patch(monkeypatch, tmp_path) + state = { + "workspace": WS, + "codex_models": ["gpt-5"], + # load_state after a first configure: ucode already manages this file. + "managed_configs": {"codex": {"keys": [["model_provider"]]}}, + } + + codex.write_tool_config(state) + + assert not (tmp_path / "backup.toml").exists() + + def test_clear_model_preferences_does_not_back_up_generated_profile( + self, tmp_path, monkeypatch + ): + self._patch(monkeypatch, tmp_path) + (tmp_path / "ucode.config.toml").write_text('model = "system.ai.gpt-5"\n', encoding="utf-8") + + changed = codex.clear_model_preferences( + {"workspace": WS, "managed_configs": {"codex": {"keys": []}}} + ) + + assert changed is True + assert "model" not in read_toml_safe(tmp_path / "ucode.config.toml") + assert not (tmp_path / "backup.toml").exists() diff --git a/tests/test_cli.py b/tests/test_cli.py index 65638e89f..dddd90308 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4024,3 +4024,28 @@ def test_still_logs_in_when_nothing_external_is_set(self, monkeypatch): monkeypatch.delenv("DATABRICKS_BEARER_COMMAND", raising=False) assert self._run(monkeypatch) == ["https://ws.cloud.databricks.com"] + + +class TestStdioProtocolLaunch: + """`codex app-server` owns stdout, so ug's status output moves to stderr.""" + + def test_app_server_subcommand_owns_stdout(self): + assert cli_mod._child_owns_stdout("codex", ["app-server", "--listen", "stdio://"]) is True + + def test_other_codex_launches_keep_stdout(self): + assert cli_mod._child_owns_stdout("codex", []) is False + assert cli_mod._child_owns_stdout("codex", ["exec", "--json", "hi"]) is False + + def test_other_agents_never_own_stdout(self): + assert cli_mod._child_owns_stdout("claude", ["app-server"]) is False + assert cli_mod._child_owns_stdout("gemini", []) is False + + def test_redirect_rebinds_stdout_without_touching_the_descriptor(self): + import sys + + real_stdout = sys.stdout + try: + cli_mod.redirect_output_to_stderr() + assert sys.stdout is sys.stderr + finally: + sys.stdout = real_stdout