From 75f90cb8728d18e0e77441e276999293e6f07c78 Mon Sep 17 00:00:00 2001 From: David Liu Date: Sun, 6 Sep 2026 15:02:18 +0000 Subject: [PATCH] Stop `ug configure` erroring on feature-disabled workspaces `refresh_managed_config` treated FEATURE_DISABLED like a transient read failure and fell back to a stale cached config with the feature-disabled flag unset, masking the disabled state. An admin was then routed into the managed setup flow, which re-read the workspace, got FEATURE_DISABLED, and raised the alarming circular error. Treat FEATURE_DISABLED as authoritative: return no config with the flag set and clear the persisted cache, so `ug configure` stays on the normal per-user flow, a launch does not re-apply a policy the workspace has turned off, and a later transient read or token failure cannot resurrect the disabled policy through the fallback. Also initialize the feature-disabled flag on the `--dry-run` path (a pre-existing UnboundLocalError when the local cache is empty). Co-authored-by: Isaac --- src/ucode/cli.py | 3 ++- src/ucode/managed_config.py | 21 +++++++++++++-------- tests/conftest.py | 4 ++++ tests/test_cli.py | 24 ++++++++++++++++++++++++ tests/test_managed_config.py | 20 ++++++++++++++------ 5 files changed, 57 insertions(+), 15 deletions(-) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 60d4f1cb..dee12f1e 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -2419,7 +2419,8 @@ 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. + # --dry-run doesn't fetch, so default the feature-disabled flag rather than leave it unbound. + 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..cc43a333 100644 --- a/src/ucode/managed_config.py +++ b/src/ucode/managed_config.py @@ -416,11 +416,15 @@ 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: @@ -431,10 +435,11 @@ def refresh_managed_config(state: dict) -> tuple[dict | None, bool]: return _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 None, True fallback = _persisted_fallback(workspace, reason, refused=_is_permission_denied(reason)) - return fallback, _is_feature_disabled(reason) and fallback is None + return 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 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 9e3e05df..80e1c1bd 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4073,6 +4073,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"))