diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 0037352f..2e3f307a 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -74,6 +74,7 @@ ) from ucode.managed_config import ( MANAGED_CONFIG_ENV_VAR, + ManagedConfigResult, get_model_recommendation, load_managed_state, managed_agent_config_enabled, @@ -1802,15 +1803,16 @@ def _reject_disabled_agent(managed: dict | None, tool: str) -> None: ) -def _fetch_managed_config(state: dict) -> tuple[dict | None, bool]: - """The workspace's managed config for this launch, or ``(None, _)`` when there is none. +def _fetch_managed_config(state: dict) -> ManagedConfigResult: + """The workspace's managed config for this launch, or a manifest-less result when there is none. - Returns ``(None, False)`` when managed configs are switched off — either the feature is disabled - or the launch passed ``--skip-managed-config`` (which clears the enabling env var for the process). + Returns a feature-disabled-free result when managed configs are switched off — either the feature + is disabled or the launch passed ``--skip-managed-config`` (which clears the enabling env var for + the process). """ if not managed_agent_config_enabled(): - return None, False + return ManagedConfigResult(None, False) with spinner("Loading..."): return refresh_managed_config(state) @@ -2493,7 +2495,7 @@ def _launch_managed_default( if not current: raise RuntimeError("No workspace configured. Run `ug configure` first.") apply_pat_environment(state) - # --dry-run avoids the fetch but still applies the last saved config. + coding_agent_config_feature_disabled = False if dry_run: managed = load_managed_state(current) else: diff --git a/src/ucode/managed_config.py b/src/ucode/managed_config.py index 7bbf512b..ca18b767 100644 --- a/src/ucode/managed_config.py +++ b/src/ucode/managed_config.py @@ -28,7 +28,7 @@ import json import os from pathlib import Path -from typing import cast +from typing import NamedTuple, cast import ucode.config_io as config_io from ucode.databricks import ( @@ -74,6 +74,22 @@ } +class FetchedManagedConfig(NamedTuple): + """A managed-config read: the normalized ``manifest`` (None when the workspace has none) and, + when the read did not settle the question, the ``reason`` it failed (None on a clean answer).""" + + manifest: dict | None + reason: str | None + + +class ManagedConfigResult(NamedTuple): + """The launch-path refresh outcome: the ``manifest`` to apply (None when absent or dropped) and + ``feature_disabled``, True when the coding-agent-configs feature is off server-side.""" + + manifest: dict | None + feature_disabled: bool + + def _as_dict(value: object) -> dict[str, object]: """Return ``value`` as a ``dict[str, object]`` when it is a dict, else an empty dict. @@ -292,34 +308,34 @@ def get_model_recommendation(workspace: str, token: str) -> tuple[dict | None, s }, None -def get_managed_config(workspace: str, token: str) -> tuple[dict | None, str | None]: +def get_managed_config(workspace: str, token: str) -> FetchedManagedConfig: """Fetch and normalize the workspace's managed config. - Returns ``(config, reason)``: - - ``(config, None)`` — the normalized manifest for the workspace's single config; - - ``(None, None)`` — the workspace definitively has no managed config (not an error); - - ``(None, reason)`` — the read didn't settle the question; ``reason`` says why. + Returns a :class:`FetchedManagedConfig`: + - ``manifest`` set, ``reason`` None — the normalized manifest for the workspace's single config; + - both None — the workspace definitively has no managed config (not an error); + - ``manifest`` None, ``reason`` set — the read didn't settle the question; ``reason`` says why. - The distinction matters to callers that cache: only ``(None, None)`` is authoritative enough to - clear a previously stored config. "No config defined" arrives two ways depending on the backend - — an empty listing (HTTP 200 with no configs) or a NOT_FOUND — and both collapse to - ``(None, None)``. Anything else, including a PERMISSION_DENIED, leaves the question unanswered - and is surfaced as a failure: an admin may have published a config the developer can't read, - which they need to know about rather than silently launch without. + The distinction matters to callers that cache: only "both None" is authoritative enough to clear + a previously stored config. "No config defined" arrives two ways depending on the backend — an + empty listing (HTTP 200 with no configs) or a NOT_FOUND — and both collapse to "both None". + Anything else, including a PERMISSION_DENIED, leaves the question unanswered and is surfaced as a + failure: an admin may have published a config the developer can't read, which they need to know + about rather than silently launch without. v0 stores at most one config per workspace, so the first entry is the workspace's config. """ configs, reason = fetch_managed_coding_agent_configs(workspace, token) if reason is not None: if _is_feature_disabled(reason): - return None, reason + return FetchedManagedConfig(None, reason) # A NOT_FOUND means the admin hasn't defined a config for this workspace — not a failure. if _is_not_found(reason): - return None, None - return None, reason + return FetchedManagedConfig(None, None) + return FetchedManagedConfig(None, reason) if not configs: - return None, None - return normalize_managed_config(configs[0]), None + return FetchedManagedConfig(None, None) + return FetchedManagedConfig(normalize_managed_config(configs[0]), None) def _is_not_found(reason: str) -> bool: @@ -406,8 +422,8 @@ def managed_state_workspace() -> str | None: return workspace if isinstance(workspace, str) and workspace else None -def refresh_managed_config(state: dict) -> tuple[dict | None, bool]: - """Fetch the workspace's managed config and persist it, returning ``(manifest, coding_agent_config_feature_disabled)``. +def refresh_managed_config(state: dict) -> ManagedConfigResult: + """Fetch the workspace's managed config and persist it as a :class:`ManagedConfigResult`. Runs on every launch so a developer picks up an admin's edits without re-running ``ucode configure``. The manifest is None when the workspace has no managed config — the normal @@ -416,33 +432,38 @@ def refresh_managed_config(state: dict) -> tuple[dict | None, bool]: A failed fetch never blocks the launch: an unreachable control plane shouldn't stop someone from coding. Instead it falls back to the last config persisted for this workspace, so the admin's most recent known policy still applies; only when there is no persisted config either does the - launch fall through to the developer's own settings. - - ``coding_agent_config_feature_disabled`` is True when the gateway returned ``FEATURE_DISABLED`` and there was no - persisted config to fall back on — the coding-agent-configs feature isn't enabled server-side, - so callers suppress the ``ucode setup`` recommendation. + launch fall through to the developer's own settings. ``FEATURE_DISABLED`` is the exception — it + is an authoritative "off", not a transient failure, so it drops the cache rather than falling + back (see below). + + ``coding_agent_config_feature_disabled`` is True whenever the gateway returned ``FEATURE_DISABLED`` — + the coding-agent-configs feature isn't enabled server-side, so callers suppress the ``ucode + setup`` recommendation. A config cached from when the feature was enabled is discarded in that + case (returned manifest is None), so a launch doesn't re-apply a policy the workspace has turned + off and ``ug configure`` doesn't route into a managed-setup flow that would dead-end. """ workspace = state.get("workspace") if not workspace: - return None, False + return ManagedConfigResult(None, False) try: token = get_databricks_token(workspace, state.get("profile")) except RuntimeError as exc: - return _persisted_fallback(workspace, str(exc)), False + return ManagedConfigResult(_persisted_fallback(workspace, str(exc)), False) managed, reason = get_managed_config(workspace, token) if reason is not None: - # A refused read leaves the cached config alone: it says nothing about whether the admin's - # config still exists, unlike a successful "no config" answer below. + if _is_feature_disabled(reason): + save_managed_state(workspace, {}) + return ManagedConfigResult(None, True) fallback = _persisted_fallback(workspace, reason, refused=_is_permission_denied(reason)) - return fallback, _is_feature_disabled(reason) and fallback is None + return ManagedConfigResult(fallback, False) if managed is None: # Record that this workspace has no config, rather than leaving an earlier one on disk: # the file doubles as the fallback above, so a removed policy would otherwise come back # into force after the next transient outage. save_managed_state(workspace, {}) - return None, False + return ManagedConfigResult(None, False) save_managed_state(workspace, managed) - return managed, False + return ManagedConfigResult(managed, False) def _is_feature_disabled(reason: str) -> bool: diff --git a/tests/conftest.py b/tests/conftest.py index ce36378b..2992622a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -26,6 +26,7 @@ def _isolate_ucode_state(tmp_path, monkeypatch): """ import ucode.config_io as config_io_mod import ucode.databricks as databricks_mod + import ucode.managed_config as managed_config_mod import ucode.managed_files as managed_files_mod import ucode.state as state_mod from ucode.agents import codex as codex_mod @@ -34,6 +35,9 @@ def _isolate_ucode_state(tmp_path, monkeypatch): state_dir.mkdir() monkeypatch.setattr(state_mod, "STATE_PATH", state_dir / "state.json") monkeypatch.setattr(config_io_mod, "APP_DIR", state_dir) + # MANAGED_STATE_PATH is bound from APP_DIR at import, so patching APP_DIR alone doesn't move it; + # rebind it or save_managed_state writes to the developer's real ~/.ucode/managed-state.json. + monkeypatch.setattr(managed_config_mod, "MANAGED_STATE_PATH", state_dir / "managed-state.json") backup_dir = state_dir / "managed-backups" monkeypatch.setattr(managed_files_mod, "MANAGED_BACKUP_DIR", backup_dir) monkeypatch.setattr( diff --git a/tests/test_cli.py b/tests/test_cli.py index cc9b0b8e..2210eec3 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4126,6 +4126,30 @@ def test_dry_run_uses_the_cache_and_does_not_fetch(self, monkeypatch): # The config bare `ucode` already read is handed down, so the launch path does not refetch. assert launched[0][1]["managed"] == self.MANAGED + def test_dry_run_with_no_cached_config_does_not_crash(self, monkeypatch): + # --dry-run doesn't fetch, so the feature-disabled flag is never assigned by the fetch path. + # With no cached config it must still be well-defined (defaults False) rather than raising + # UnboundLocalError when the guidance check reads it. + monkeypatch.setenv("ENABLE_MANAGED_AGENT_CONFIG", "1") + monkeypatch.setattr("ucode.cli.install_databricks_cli", lambda *a, **k: None) + monkeypatch.setattr("ucode.cli.apply_pat_environment", lambda *a, **k: None) + monkeypatch.setattr("ucode.cli.load_state", lambda: {"workspace": "https://w"}) + monkeypatch.setattr( + "ucode.cli.refresh_managed_config", + lambda state: pytest.fail("--dry-run must not fetch"), + ) + monkeypatch.setattr("ucode.cli.load_managed_state", lambda ws: None) + # The no-config guidance checks admin status; stub the token/admin calls so the test + # doesn't shell out to the `databricks` binary (absent in CI). + monkeypatch.setattr("ucode.cli.get_databricks_token", lambda *a, **k: "tok") + monkeypatch.setattr("ucode.cli.is_workspace_admin", lambda *a, **k: False) + monkeypatch.setattr( + "ucode.cli._launch_tool", + lambda *a, **k: pytest.fail("nothing to launch without a config"), + ) + result = runner.invoke(app, ["--dry-run"]) + assert result.exit_code == 0, result.output + def test_skip_preflight_still_resolves_an_agent_from_the_managed_config(self, monkeypatch): # --skip-preflight is now only about auth/gateway re-validation, decoupled from managed # config, so bare `ucode --skip-preflight` still fetches the config and picks its agent. diff --git a/tests/test_managed_config.py b/tests/test_managed_config.py index 05c179fc..796e0653 100644 --- a/tests/test_managed_config.py +++ b/tests/test_managed_config.py @@ -469,17 +469,25 @@ def test_feature_disabled_sets_flag_when_there_is_no_fallback(self, monkeypatch) assert result is None assert flag is True - def test_feature_disabled_with_a_fallback_does_not_set_the_flag(self, monkeypatch): - # A cached config means the launch uses it (not the "no config" branch), so the - # feature-off flag is irrelevant and must not be set. + def test_feature_disabled_ignores_a_cached_config_and_sets_the_flag(self, monkeypatch): + # FEATURE_DISABLED is authoritative, so a config cached while the feature was enabled no + # longer applies: report "no config, feature off" and clear the persisted copy so a later + # transient failure can't resurrect the disabled policy via the fallback. + saved: list[tuple] = [] reason = 'HTTP 400 Bad Request: {"error_code":"FEATURE_DISABLED"}' monkeypatch.setattr(mc_mod, "get_managed_config", lambda ws, tok: (None, reason)) monkeypatch.setattr(mc_mod, "load_managed_state", lambda ws: MANAGED) - monkeypatch.setattr(mc_mod, "print_warning", lambda msg: None) + monkeypatch.setattr(mc_mod, "save_managed_state", lambda ws, cfg: saved.append((ws, cfg))) + monkeypatch.setattr( + mc_mod, + "print_warning", + lambda msg: pytest.fail("feature-disabled must not warn about falling back to a cache"), + ) state = _state() result, flag = refresh_managed_config(state) - assert result == MANAGED - assert flag is False + assert result is None + assert flag is True + assert saved == [(WORKSPACE, {})] def test_transient_failure_does_not_set_the_flag(self, monkeypatch): monkeypatch.setattr(mc_mod, "get_managed_config", lambda ws, tok: (None, "HTTP 500"))