From dbad802f66c39a1ab9da6ee9c09a645db52bad86 Mon Sep 17 00:00:00 2001 From: Lars Moan Date: Tue, 25 Aug 2026 12:59:22 +0000 Subject: [PATCH 1/4] feat: add --model flag to ucode copilot Copilot had no way to pick a model per launch or per session: the CLI had no --model flag (unlike claude), and Copilot's own /models picker can't be trusted to work against the Databricks gateway, plus ucode overwrites COPILOT_MODEL on every token refresh anyway. The only override was an admin-only managed config. Thread an explicit model through past that refresh loop so it stays pinned for the whole session, instead of writing it once at config time and letting default_model() silently reclaim it 30 minutes later. Closes #384 --- src/ucode/agents/__init__.py | 11 ++++++-- src/ucode/agents/copilot.py | 18 +++++++----- src/ucode/cli.py | 13 +++++++-- tests/test_agent_copilot.py | 54 ++++++++++++++++++++++++++++++++++++ tests/test_agents_init.py | 28 +++++++++++++++++++ tests/test_cli.py | 53 +++++++++++++++++++++++++++++++++++ 6 files changed, 166 insertions(+), 11 deletions(-) diff --git a/src/ucode/agents/__init__.py b/src/ucode/agents/__init__.py index 0785342f..f5d22d8d 100644 --- a/src/ucode/agents/__init__.py +++ b/src/ucode/agents/__init__.py @@ -366,8 +366,15 @@ def configure_tool( return result -def launch(tool: str, state: dict, tool_args: list[str]) -> None: - _MODULES[tool].launch(state, tool_args) +def launch(tool: str, state: dict, tool_args: list[str], model: str | None = None) -> None: + # Only copilot's launch loop re-derives its model on every token refresh + # (default_model(state)), so only it needs an explicit override threaded + # through past that point; other tools already bake --model into the + # config file written by configure_tool. + if tool == "copilot": + copilot.launch(state, tool_args, model_override=model) + else: + _MODULES[tool].launch(state, tool_args) def check_gateway_endpoint(state: dict, tool: str) -> bool: diff --git a/src/ucode/agents/copilot.py b/src/ucode/agents/copilot.py index 19a52b8e..0a4f2dfb 100644 --- a/src/ucode/agents/copilot.py +++ b/src/ucode/agents/copilot.py @@ -168,30 +168,34 @@ def write_tool_config( return state, token -def _refresh_token_once(state: dict, *, force_refresh: bool = False) -> tuple[str, str]: - model = default_model(state) +def _refresh_token_once( + state: dict, *, force_refresh: bool = False, model_override: str | None = None +) -> tuple[str, str]: + model = model_override or default_model(state) if not model: raise RuntimeError("No Copilot model is available on this workspace.") _, token = write_tool_config(state, model, force_refresh=force_refresh) return model, token -def _refresh_forever(state: dict, stop_event: threading.Event) -> None: +def _refresh_forever( + state: dict, stop_event: threading.Event, model_override: str | None = None +) -> None: while not stop_event.wait(TOKEN_REFRESH_INTERVAL_SECONDS): try: - _refresh_token_once(state, force_refresh=True) + _refresh_token_once(state, force_refresh=True, model_override=model_override) except RuntimeError: continue -def launch(state: dict, tool_args: list[str]) -> None: - model, token = _refresh_token_once(state) +def launch(state: dict, tool_args: list[str], model_override: str | None = None) -> None: + model, token = _refresh_token_once(state, model_override=model_override) env = build_runtime_env(state["workspace"], model, token) stop_event = threading.Event() refresher = threading.Thread( target=_refresh_forever, - args=(state, stop_event), + args=(state, stop_event, model_override), daemon=True, ) refresher.start() diff --git a/src/ucode/cli.py b/src/ucode/cli.py index ca71dd6a..5c527281 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -1958,7 +1958,7 @@ def _launch_tool( _register_managed_mcp_servers(managed, tool, state) _apply_managed_skills(managed, tool, state) print_success(f"Starting {TOOL_SPECS[tool]['display']}") - launch_agent(tool, state, ctx.args) + launch_agent(tool, state, ctx.args, model=model if tool == "copilot" else None) except RuntimeError as exc: print_err(str(exc)) raise typer.Exit(1) from None @@ -2271,12 +2271,21 @@ def opencode_cmd( @app.command("copilot", context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) def copilot_cmd( ctx: typer.Context, + model: Annotated[ + str | None, + typer.Option( + "--model", + help="Launch on a specific Databricks model id (e.g. a UC " + "`..`). Outranks the automatic sonnet/opus/haiku/codex " + "pick and stays pinned across ucode's automatic token refreshes.", + ), + ] = None, skip_preflight: SkipPreflightOption = False, skip_managed_config: SkipManagedConfigOption = False, ) -> None: """Launch GitHub Copilot CLI via Databricks.""" _disable_managed_config_if_requested(skip_managed_config) - _launch_tool("copilot", ctx, skip_preflight=skip_preflight) + _launch_tool("copilot", ctx, model=model, skip_preflight=skip_preflight) @app.command("pi", context_settings={"allow_extra_args": True, "ignore_unknown_options": True}) diff --git a/tests/test_agent_copilot.py b/tests/test_agent_copilot.py index f6fd5136..46f26c1c 100644 --- a/tests/test_agent_copilot.py +++ b/tests/test_agent_copilot.py @@ -3,6 +3,9 @@ from __future__ import annotations import json +import threading + +import pytest from ucode.agents import copilot @@ -198,6 +201,57 @@ def test_includes_required_vars(self): assert key in copilot.MANAGED_KEYS +class TestRefreshTokenOnceModelOverride: + def test_model_override_wins_over_default_model(self, monkeypatch): + seen: dict = {} + + def fake_write_tool_config(state, model, force_refresh=False): + seen["model"] = model + return state, "tok" + + monkeypatch.setattr(copilot, "write_tool_config", fake_write_tool_config) + state = {"claude_models": {"sonnet": "discovered"}} + + model, token = copilot._refresh_token_once(state, model_override="explicit-model") + + assert model == "explicit-model" + assert token == "tok" + assert seen["model"] == "explicit-model" + + def test_falls_back_to_default_model_without_override(self, monkeypatch): + monkeypatch.setattr( + copilot, "write_tool_config", lambda state, model, force_refresh=False: (state, "tok") + ) + state = {"claude_models": {"sonnet": "discovered"}} + + model, _ = copilot._refresh_token_once(state) + + assert model == "discovered" + + def test_raises_when_no_override_and_no_default(self): + with pytest.raises(RuntimeError, match="No Copilot model is available"): + copilot._refresh_token_once({}) + + +class TestRefreshForeverModelOverride: + def test_forwards_model_override_to_each_refresh(self, monkeypatch): + calls: list[str | None] = [] + + def fake_refresh_token_once(state, *, force_refresh=False, model_override=None): + calls.append(model_override) + if len(calls) >= 2: + stop_event.set() + return model_override, "tok" + + monkeypatch.setattr(copilot, "_refresh_token_once", fake_refresh_token_once) + monkeypatch.setattr(copilot, "TOKEN_REFRESH_INTERVAL_SECONDS", 0) + + stop_event = threading.Event() + copilot._refresh_forever({}, stop_event, model_override="pinned-model") + + assert calls == ["pinned-model", "pinned-model"] + + class TestValidateCmd: def test_starts_with_binary(self): cmd = copilot.validate_cmd("copilot") diff --git a/tests/test_agents_init.py b/tests/test_agents_init.py index f346af90..832c38dc 100644 --- a/tests/test_agents_init.py +++ b/tests/test_agents_init.py @@ -178,6 +178,34 @@ def test_copilot_unavailable_with_only_gemini(self): def test_copilot_unavailable_when_no_models(self): assert check_gateway_endpoint({}, "copilot") is False + +class TestLaunch: + def test_copilot_forwards_model_as_override(self, monkeypatch): + calls: list[tuple] = [] + monkeypatch.setattr( + agents_mod.copilot, + "launch", + lambda state, tool_args, model_override=None: calls.append( + (state, tool_args, model_override) + ), + ) + + agents_mod.launch("copilot", {"workspace": "ws"}, ["--foo"], model="explicit-model") + + assert calls == [({"workspace": "ws"}, ["--foo"], "explicit-model")] + + def test_non_copilot_tools_ignore_model_argument(self, monkeypatch): + calls: list[tuple] = [] + monkeypatch.setitem( + agents_mod._MODULES, + "gemini", + type("_Stub", (), {"launch": staticmethod(lambda s, a: calls.append((s, a)))}), + ) + + agents_mod.launch("gemini", {"workspace": "ws"}, ["--foo"], model="ignored-for-gemini") + + assert calls == [({"workspace": "ws"}, ["--foo"])] + def test_pi_available_with_claude(self): assert check_gateway_endpoint({"claude_models": {"sonnet": "s4"}}, "pi") is True diff --git a/tests/test_cli.py b/tests/test_cli.py index 99b99dcc..3bc89fe4 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -357,6 +357,59 @@ def test_no_enterprise_warning_when_no_managed_settings(self): assert "enterprise managed settings" not in _strip_ansi(result.output) +class TestCopilotModelFlag: + """`ucode copilot --model ` outranks the automatic sonnet/opus/haiku/codex pick and + stays pinned across ucode's automatic token refreshes (unlike claude, copilot takes the + id as its resolved model directly rather than as a custom_model override).""" + + def test_model_threads_through_to_launch_tool(self): + with patch("ucode.cli._launch_tool") as mock_launch: + result = runner.invoke(app, ["copilot", "--model", "cat.schema.claude-opus-5"]) + assert result.exit_code == 0, result.output + assert mock_launch.call_args.kwargs["model"] == "cat.schema.claude-opus-5" + + def test_model_becomes_resolved_model_for_configure_tool(self): + with ( + patch("ucode.cli.ensure_bootstrap_dependencies"), + patch("ucode.cli._auto_configure_tool"), + patch("ucode.cli.load_state", return_value=MINIMAL_STATE), + patch("ucode.cli.ensure_provider_state", return_value=MINIMAL_STATE), + patch("ucode.cli.configure_shared_state", return_value=MINIMAL_STATE), + patch( + "ucode.cli.resolve_launch_model", + return_value=(MINIMAL_STATE, "databricks-claude-sonnet-4"), + ), + patch("ucode.cli.configure_tool", return_value=MINIMAL_STATE) as mock_configure, + patch("ucode.cli._fetch_managed_config", return_value=(None, False)), + patch("ucode.cli.launch_agent") as mock_launch_agent, + ): + result = runner.invoke(app, ["copilot", "--model", "cat.schema.claude-opus-5"]) + assert result.exit_code == 0, result.output + # Unlike claude, copilot has no family-alias pinning — the explicit id rides as the + # ordinary resolved model, both into configure_tool and into the launch call. + assert mock_configure.call_args.args[2] == "cat.schema.claude-opus-5" + assert mock_launch_agent.call_args.kwargs["model"] == "cat.schema.claude-opus-5" + + def test_no_model_flag_passes_none_to_launch_agent(self): + with ( + patch("ucode.cli.ensure_bootstrap_dependencies"), + patch("ucode.cli._auto_configure_tool"), + patch("ucode.cli.load_state", return_value=MINIMAL_STATE), + patch("ucode.cli.ensure_provider_state", return_value=MINIMAL_STATE), + patch("ucode.cli.configure_shared_state", return_value=MINIMAL_STATE), + patch( + "ucode.cli.resolve_launch_model", + return_value=(MINIMAL_STATE, "databricks-claude-sonnet-4"), + ), + patch("ucode.cli.configure_tool", return_value=MINIMAL_STATE), + patch("ucode.cli._fetch_managed_config", return_value=(None, False)), + patch("ucode.cli.launch_agent") as mock_launch_agent, + ): + result = runner.invoke(app, ["copilot"]) + assert result.exit_code == 0, result.output + assert mock_launch_agent.call_args.kwargs["model"] is None + + class TestMcpSubcommands: def test_web_search_subcommand_help(self): result = runner.invoke(app, ["mcp", "web-search", "--help"]) From 0ff75e7d705bc02b57811261c16bd62c5bdc6da8 Mon Sep 17 00:00:00 2001 From: Lars Moan Date: Tue, 25 Aug 2026 13:11:15 +0000 Subject: [PATCH 2/4] fix: use resolved_model, not raw --model, for copilot launch pinning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without an explicit --model, this passed None to launch_agent, so copilot.launch() recomputed default_model(state) from scratch instead of reusing what configure_tool had already written — silently diverging from a managed config's default or a budget recommendation, and repeating that drift on every 30-minute token refresh. resolved_model already absorbs --model, the managed default, and any budget recommendation, so it's the correct value to pin. Flagged by review on #385. --- src/ucode/cli.py | 6 +++++- tests/test_cli.py | 8 ++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 5c527281..ef564ffe 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -1958,7 +1958,11 @@ def _launch_tool( _register_managed_mcp_servers(managed, tool, state) _apply_managed_skills(managed, tool, state) print_success(f"Starting {TOOL_SPECS[tool]['display']}") - launch_agent(tool, state, ctx.args, model=model if tool == "copilot" else None) + # Pass the already-settled resolved_model (which absorbs --model, a managed config's + # default, and any budget recommendation), not the raw --model flag — otherwise a launch + # without an explicit --model would have copilot.launch() call default_model(state) fresh + # and silently rewrite whatever configure_tool just wrote, and every refresh after it. + launch_agent(tool, state, ctx.args, model=resolved_model if tool == "copilot" else None) except RuntimeError as exc: print_err(str(exc)) raise typer.Exit(1) from None diff --git a/tests/test_cli.py b/tests/test_cli.py index 3bc89fe4..a23ab22e 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -390,7 +390,11 @@ def test_model_becomes_resolved_model_for_configure_tool(self): assert mock_configure.call_args.args[2] == "cat.schema.claude-opus-5" assert mock_launch_agent.call_args.kwargs["model"] == "cat.schema.claude-opus-5" - def test_no_model_flag_passes_none_to_launch_agent(self): + def test_no_model_flag_still_pins_the_resolved_model_to_launch_agent(self): + # Without an explicit --model, resolved_model (whatever resolve_launch_model/managed + # config/budget recommendation settled on) must still ride to launch_agent — passing None + # here would make copilot.launch() recompute default_model(state) fresh and can silently + # diverge from what configure_tool actually wrote to the config file. with ( patch("ucode.cli.ensure_bootstrap_dependencies"), patch("ucode.cli._auto_configure_tool"), @@ -407,7 +411,7 @@ def test_no_model_flag_passes_none_to_launch_agent(self): ): result = runner.invoke(app, ["copilot"]) assert result.exit_code == 0, result.output - assert mock_launch_agent.call_args.kwargs["model"] is None + assert mock_launch_agent.call_args.kwargs["model"] == "databricks-claude-sonnet-4" class TestMcpSubcommands: From 321e6938631b11466f186a28e93ced7b0c03a750 Mon Sep 17 00:00:00 2001 From: Lars Moan Date: Tue, 25 Aug 2026 13:40:31 +0000 Subject: [PATCH 3/4] fix: honor --model during first-time copilot auto-configure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a first-time `ucode copilot --model X` (copilot not yet in available_tools, or no workspace configured), _auto_configure_tool ran before the --model handling further down _launch_tool, so it wrote and smoke-tested the automatic sonnet/opus/haiku/codex default instead of X. If that automatic pick failed validation, the launch aborted before X was ever tried — even though X itself may have been perfectly valid. Thread the explicit model through configure_single_tool/_configure_one (so resolve_launch_model's explicit_model wins) and through validate_tool (so copilot.validate_env smoke-tests X, not the default). Also fixes a test-ordering issue: TestLaunch had been inserted in the middle of TestCheckGatewayEndpoint, splitting its test_pi_available_* tests into a misleadingly-named group. Flagged by review on #385. --- src/ucode/agents/__init__.py | 35 +++++++++--- src/ucode/agents/copilot.py | 4 +- src/ucode/cli.py | 16 ++++-- tests/test_agents_init.py | 102 ++++++++++++++++++++++++++++++----- tests/test_cli.py | 4 +- 5 files changed, 132 insertions(+), 29 deletions(-) diff --git a/src/ucode/agents/__init__.py b/src/ucode/agents/__init__.py index f5d22d8d..247480cf 100644 --- a/src/ucode/agents/__init__.py +++ b/src/ucode/agents/__init__.py @@ -419,8 +419,13 @@ def _availability_failure_detail(tool: str, state: dict) -> str: return " (" + "; ".join(parts) + ")" -def configure_single_tool(tool: str, state: dict) -> dict: - """Check availability, configure, and persist state for one tool only.""" +def configure_single_tool(tool: str, state: dict, explicit_model: str | None = None) -> dict: + """Check availability, configure, and persist state for one tool only. + + ``explicit_model`` is a caller-requested model (e.g. `ucode copilot --model X` on a + first-time configure) that must win over the automatic default pick — otherwise a bad + automatic pick can fail the availability/validation check before X is ever tried. + """ provider = get_provider_service(state, tool) # A Model Provider Service routes through the same gateway and pins no # Databricks model, so the per-tool model availability check doesn't apply. @@ -432,14 +437,16 @@ def configure_single_tool(tool: str, state: dict) -> dict: raise RuntimeError( f"{TOOL_SPECS[tool]['display']} is not available on this workspace.{detail}" ) - state = _configure_one(tool, state, provider) + state = _configure_one(tool, state, provider, explicit_model=explicit_model) available_tools = list(set((state.get("available_tools") or []) + [tool])) state["available_tools"] = available_tools save_state(state) return state -def _configure_one(tool: str, state: dict, provider: str | None) -> dict: +def _configure_one( + tool: str, state: dict, provider: str | None, explicit_model: str | None = None +) -> dict: """Write one tool's config, routing through ``provider`` when set.""" if provider: provider_models, error, relayed = resolve_provider_models(tool, state, provider) @@ -450,7 +457,7 @@ def _configure_one(tool: str, state: dict, provider: str | None) -> dict: ) if tool == "codex": return configure_tool("codex", state) - state, model = resolve_launch_model(tool, state, None) + state, model = resolve_launch_model(tool, state, explicit_model) return configure_tool(tool, state, model) @@ -511,8 +518,14 @@ def ensure_provider_state(tool: str) -> dict: return state -def validate_tool(tool: str) -> tuple[bool, str]: - """Invoke a tool with a simple prompt to verify it works. Returns (ok, error_msg).""" +def validate_tool(tool: str, model: str | None = None) -> tuple[bool, str]: + """Invoke a tool with a simple prompt to verify it works. Returns (ok, error_msg). + + ``model`` is an explicit --model request (currently only honored for copilot): without it, + validation would smoke-test the automatic default pick instead of the model the caller + actually asked for, so a bad automatic pick could fail validation before the requested + model is ever tried. + """ spec = TOOL_SPECS[tool] binary = spec["binary"] module = _MODULES[tool] @@ -524,7 +537,13 @@ def validate_tool(tool: str) -> tuple[bool, str]: env = None if hasattr(module, "validate_env"): try: - env = module.validate_env(load_state()) + # `copilot.validate_env` is the only variant accepting `model_override`; reference + # it directly rather than through the union-typed `module` so the extra kwarg + # type-checks (ty can't narrow `module`'s type from the `tool == "copilot"` check). + if tool == "copilot": + env = copilot.validate_env(load_state(), model_override=model) + else: + env = module.validate_env(load_state()) except RuntimeError: env = None try: diff --git a/src/ucode/agents/copilot.py b/src/ucode/agents/copilot.py index 0a4f2dfb..a387db4f 100644 --- a/src/ucode/agents/copilot.py +++ b/src/ucode/agents/copilot.py @@ -229,12 +229,12 @@ def mcp_config_args() -> list[str]: return ["--additional-mcp-config", f"@{COPILOT_MCP_CONFIG_PATH}"] -def validate_env(state: dict) -> dict[str, str]: +def validate_env(state: dict, model_override: str | None = None) -> dict[str, str]: """Inject BYOK env vars for the validation subprocess (Copilot doesn't auto-load .env).""" workspace = state.get("workspace") if not workspace: raise RuntimeError("No workspace configured.") - model = default_model(state) + model = model_override or default_model(state) if not model: raise RuntimeError("No Copilot model is available on this workspace.") token = get_databricks_token(workspace, state.get("profile")) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index ef564ffe..88738eee 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -1469,8 +1469,14 @@ def claude_router_hook_cmd( sys.stdout.write(json.dumps(output)) -def _auto_configure_tool(tool: str) -> None: - """First-time setup for a single tool — mirrors configure_workspace_command.""" +def _auto_configure_tool(tool: str, model: str | None = None) -> None: + """First-time setup for a single tool — mirrors configure_workspace_command. + + ``model`` is an explicit --model request (currently only threaded for copilot): without it, + a first-time `ucode copilot --model X` would auto-configure and validate against the + automatic sonnet/opus/haiku/codex default instead of X, so a bad automatic pick could fail + validation and abort the launch before X is ever tried. + """ existing = load_state() workspace = existing.get("workspace") profile = existing.get("profile") @@ -1478,7 +1484,7 @@ def _auto_configure_tool(tool: str) -> None: workspace, profile = _prompt_for_configuration(tool) state = configure_shared_state(workspace, profile=profile, tools=[tool]) - state = configure_single_tool(tool, state) + state = configure_single_tool(tool, state, explicit_model=model) spec = TOOL_SPECS[tool] console.print( @@ -1493,7 +1499,7 @@ def _auto_configure_tool(tool: str) -> None: ) with spinner(f"Validating {spec['display']}..."): - ok, err = validate_tool(tool) + ok, err = validate_tool(tool, model=model) if ok: print_success(f"{spec['display']} is working") else: @@ -1733,7 +1739,7 @@ def _launch_tool( ) ensure_bootstrap_dependencies(tool, update_existing=needs_auto_configure) if needs_auto_configure: - _auto_configure_tool(tool) + _auto_configure_tool(tool, model=model if tool == "copilot" else None) state = ensure_provider_state(tool) # Remembered before the fallback below collapses the two cases: a managed config may not # silently override a provider the user typed on the command line (it errors instead). diff --git a/tests/test_agents_init.py b/tests/test_agents_init.py index 832c38dc..3cd0afb2 100644 --- a/tests/test_agents_init.py +++ b/tests/test_agents_init.py @@ -178,6 +178,18 @@ def test_copilot_unavailable_with_only_gemini(self): def test_copilot_unavailable_when_no_models(self): assert check_gateway_endpoint({}, "copilot") is False + def test_pi_available_with_claude(self): + assert check_gateway_endpoint({"claude_models": {"sonnet": "s4"}}, "pi") is True + + def test_pi_available_with_codex(self): + assert check_gateway_endpoint({"codex_models": ["m"]}, "pi") is True + + def test_pi_available_with_gemini(self): + assert check_gateway_endpoint({"gemini_models": ["gemini-2"]}, "pi") is True + + def test_pi_unavailable_when_no_models(self): + assert check_gateway_endpoint({}, "pi") is False + class TestLaunch: def test_copilot_forwards_model_as_override(self, monkeypatch): @@ -206,18 +218,6 @@ def test_non_copilot_tools_ignore_model_argument(self, monkeypatch): assert calls == [({"workspace": "ws"}, ["--foo"])] - def test_pi_available_with_claude(self): - assert check_gateway_endpoint({"claude_models": {"sonnet": "s4"}}, "pi") is True - - def test_pi_available_with_codex(self): - assert check_gateway_endpoint({"codex_models": ["m"]}, "pi") is True - - def test_pi_available_with_gemini(self): - assert check_gateway_endpoint({"gemini_models": ["gemini-2"]}, "pi") is True - - def test_pi_unavailable_when_no_models(self): - assert check_gateway_endpoint({}, "pi") is False - class TestDefaultModelForTool: def test_codex_returns_highest_gpt_model(self): @@ -620,6 +620,46 @@ def test_ensure_tool_binary_available_raises_when_missing(self, monkeypatch): ensure_tool_binary_available("opencode") +class TestConfigureSingleToolExplicitModel: + """A first-time `ucode copilot --model X` must configure and validate against X, not the + automatic sonnet/opus/haiku/codex default — otherwise a bad automatic pick can fail + availability/validation before X is ever tried.""" + + def test_explicit_model_wins_over_the_automatic_default(self, monkeypatch): + monkeypatch.setattr(agents_mod, "check_gateway_endpoint", lambda state, tool: True) + monkeypatch.setattr(agents_mod, "get_provider_service", lambda state, tool: None) + monkeypatch.setattr(agents_mod, "save_state", lambda state: None) + seen: dict = {} + + def fake_configure_tool(tool, state, model, **kwargs): + seen["model"] = model + return state + + monkeypatch.setattr(agents_mod, "configure_tool", fake_configure_tool) + state = {"claude_models": {"sonnet": "should-not-win"}} + + agents_mod.configure_single_tool("copilot", state, explicit_model="explicit-model") + + assert seen["model"] == "explicit-model" + + def test_falls_back_to_default_without_an_explicit_model(self, monkeypatch): + monkeypatch.setattr(agents_mod, "check_gateway_endpoint", lambda state, tool: True) + monkeypatch.setattr(agents_mod, "get_provider_service", lambda state, tool: None) + monkeypatch.setattr(agents_mod, "save_state", lambda state: None) + seen: dict = {} + + def fake_configure_tool(tool, state, model, **kwargs): + seen["model"] = model + return state + + monkeypatch.setattr(agents_mod, "configure_tool", fake_configure_tool) + state = {"claude_models": {"sonnet": "the-default"}} + + agents_mod.configure_single_tool("copilot", state) + + assert seen["model"] == "the-default" + + class TestConfigureSelectedTools: def test_merges_with_existing_available_tools(self, monkeypatch): """Configuring a new tool should not drop previously-configured tools @@ -733,3 +773,41 @@ def fail_run(cmd, **kwargs): assert ok is True assert err == "" + + def test_copilot_model_override_reaches_validate_env(self, monkeypatch): + # An explicit --model must be what gets smoke-tested, not the automatic + # sonnet/opus/haiku/codex default — otherwise a bad automatic pick can fail + # validation before the requested model is ever tried. + seen: dict = {} + + def fake_validate_env(state, model_override=None): + seen["model_override"] = model_override + return {"COPILOT_MODEL": model_override} + + monkeypatch.setattr(agents_mod.copilot, "validate_env", fake_validate_env) + monkeypatch.setattr( + "ucode.agents.subprocess.run", + lambda cmd, **kwargs: subprocess.CompletedProcess(cmd, 0, stdout="", stderr=""), + ) + monkeypatch.setattr(agents_mod, "load_state", lambda: {}) + + ok, _ = agents_mod.validate_tool("copilot", model="explicit-model") + + assert ok is True + assert seen["model_override"] == "explicit-model" + + def test_non_copilot_tools_ignore_the_model_argument(self, monkeypatch): + # Only copilot's validate_env accepts a model override today. + def fake_validate_env(state): + return {} + + monkeypatch.setattr(agents_mod.gemini, "validate_env", fake_validate_env) + monkeypatch.setattr( + "ucode.agents.subprocess.run", + lambda cmd, **kwargs: subprocess.CompletedProcess(cmd, 0, stdout="", stderr=""), + ) + monkeypatch.setattr(agents_mod, "load_state", lambda: {}) + + ok, _ = agents_mod.validate_tool("gemini", model="ignored-for-gemini") + + assert ok is True diff --git a/tests/test_cli.py b/tests/test_cli.py index a23ab22e..d1b09bba 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -987,7 +987,7 @@ def test_triggers_when_no_workspace(self): result = runner.invoke(app, ["claude"]) assert result.exit_code == 0, result.output mock_bootstrap.assert_called_once_with("claude", update_existing=True) - mock_auto.assert_called_once_with("claude") + mock_auto.assert_called_once_with("claude", model=None) def test_triggers_when_tool_not_in_available_tools(self): """Auto-configure runs when workspace exists but the tool wasn't configured.""" @@ -1012,7 +1012,7 @@ def test_triggers_when_tool_not_in_available_tools(self): result = runner.invoke(app, ["claude"]) assert result.exit_code == 0, result.output mock_bootstrap.assert_called_once_with("claude", update_existing=True) - mock_auto.assert_called_once_with("claude") + mock_auto.assert_called_once_with("claude", model=None) def test_skipped_when_already_configured(self): """Auto-configure is skipped when workspace and tool are already set up.""" From 22e01a33ec0dc42282f8de27046c3ecc8f15ce84 Mon Sep 17 00:00:00 2001 From: Lars Moan Date: Tue, 25 Aug 2026 23:22:15 +0000 Subject: [PATCH 4/4] fix: correct configure_single_tool docstring on the availability check The docstring claimed explicit_model keeps a bad automatic pick from failing the availability check. It does not: check_gateway_endpoint runs before _configure_one sees explicit_model, and takes no model at all. explicit_model only changes what gets written and smoke-tested. Flagged by review on #385. --- src/ucode/agents/__init__.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/ucode/agents/__init__.py b/src/ucode/agents/__init__.py index 247480cf..bb862527 100644 --- a/src/ucode/agents/__init__.py +++ b/src/ucode/agents/__init__.py @@ -423,8 +423,10 @@ def configure_single_tool(tool: str, state: dict, explicit_model: str | None = N """Check availability, configure, and persist state for one tool only. ``explicit_model`` is a caller-requested model (e.g. `ucode copilot --model X` on a - first-time configure) that must win over the automatic default pick — otherwise a bad - automatic pick can fail the availability/validation check before X is ever tried. + first-time configure) that must win over the automatic default pick, so the model written + to the config and smoke-tested is X — otherwise a bad automatic pick can fail validation + before X is ever tried. The availability check above is model-independent + (``check_gateway_endpoint``), so it still gates on discovery finding any model at all. """ provider = get_provider_service(state, tool) # A Model Provider Service routes through the same gateway and pins no