diff --git a/py/services/settings_manager.py b/py/services/settings_manager.py index 4217f92c..177cbab2 100644 --- a/py/services/settings_manager.py +++ b/py/services/settings_manager.py @@ -37,6 +37,7 @@ from ..utils.constants import ( from ..utils.preview_selection import VALID_MATURE_BLUR_LEVELS from ..utils.settings_paths import ( APP_NAME, + _portable_env_override, ensure_settings_file, get_legacy_settings_path, get_settings_dir_override, @@ -172,13 +173,23 @@ class SettingsManager: self._check_environment_variables() self._collect_configuration_warnings() - if ( - os.environ.get("LORA_MANAGER_PORTABLE", "0") == "1" - and not is_settings_dir_pinned() - ): + portable_override = _portable_env_override() + if portable_override is True and not is_settings_dir_pinned(): if not self.settings.get("use_portable_settings"): self.settings["use_portable_settings"] = True self._save_settings() + elif portable_override is False and self.settings.get( + "use_portable_settings" + ): + # Explicit opt-out from a persisted portable mode: clear the flag so + # later runs go back to the shared settings directory instead of + # requiring a manual edit of settings.json. + logger.info( + "Clearing the persisted portable-mode flag because %s=0", + "LORA_MANAGER_PORTABLE", + ) + self.settings["use_portable_settings"] = False + self._save_settings() if self._needs_initial_save: self._save_settings() diff --git a/py/utils/settings_paths.py b/py/utils/settings_paths.py index fd0b93bd..ba9fd95e 100644 --- a/py/utils/settings_paths.py +++ b/py/utils/settings_paths.py @@ -174,12 +174,42 @@ def ensure_settings_file(logger: Optional[logging.Logger] = None) -> str: return target_path +def _portable_env_override() -> Optional[bool]: + """Return the portable mode forced by ``LORA_MANAGER_PORTABLE``, if any. + + Returns: + ``True`` when the variable enables portable mode, ``False`` when it is + explicitly set to ``"0"``, and ``None`` when it is unset or holds some + other value (in which case the persisted settings flag decides). + """ + + raw = os.environ.get(_LM_PORTABLE_ENV) + if raw is None: + return None + if raw == "1": + return True + if raw == "0": + return False + return None + + def _should_use_portable_settings(path: str, logger: logging.Logger) -> bool: """Return ``True`` when the env var forces it or the settings file enables it.""" - if os.environ.get(_LM_PORTABLE_ENV, "0") == "1": + override = _portable_env_override() + if override is True: logger.debug("Portable mode enabled via %s", _LM_PORTABLE_ENV) return True + if override is False: + # Explicit opt-out. Without this, a single `LORA_MANAGER_PORTABLE=1` + # run would pin the shared plugin settings.json to portable mode + # forever, with no way back except editing that file by hand. + logger.info( + "Portable mode disabled via %s=%s", + _LM_PORTABLE_ENV, + os.environ.get(_LM_PORTABLE_ENV, ""), + ) + return False if not os.path.exists(path): return False diff --git a/tests/services/test_portable_mode_flag.py b/tests/services/test_portable_mode_flag.py new file mode 100644 index 00000000..93f54e49 --- /dev/null +++ b/tests/services/test_portable_mode_flag.py @@ -0,0 +1,105 @@ +"""Tests for the portable-mode flag lifecycle (issue #1114 follow-up). + +``LORA_MANAGER_PORTABLE=1`` persists ``use_portable_settings: true`` into the +plugin''s own settings.json. That is convenient for repeat runs, but it used to +be a one-way trip: the flag made every instance sharing that plugin folder read +(and write) the portable settings directory, and the only way back was editing +settings.json by hand. ``LORA_MANAGER_PORTABLE=0`` is now the explicit exit. +""" + +from __future__ import annotations + +import json + +import pytest + +from py.services import settings_manager as settings_manager_module +from py.services.settings_manager import SettingsManager + + +def _write_settings(path, **extra): + payload = { + "folder_paths": {"loras": ["/loras"]}, + } + payload.update(extra) + path.write_text(json.dumps(payload), encoding="utf-8") + return payload + + +@pytest.fixture +def isolated_settings_path(tmp_path, monkeypatch): + """Point SettingsManager at a settings.json we control.""" + settings_path = tmp_path / "settings.json" + monkeypatch.setattr( + "py.services.settings_manager.ensure_settings_file", + lambda logger=None: str(settings_path), + ) + settings_manager_module.reset_settings_manager() + yield settings_path + settings_manager_module.reset_settings_manager() + + +def test_portable_env_enables_and_persists_the_flag( + isolated_settings_path, monkeypatch +): + _write_settings(isolated_settings_path) + monkeypatch.setenv("LORA_MANAGER_PORTABLE", "1") + + manager = SettingsManager() + + assert manager.get("use_portable_settings") is True + persisted = json.loads(isolated_settings_path.read_text(encoding="utf-8")) + assert persisted["use_portable_settings"] is True + + +def test_explicit_zero_clears_the_persisted_flag( + isolated_settings_path, monkeypatch +): + """`=0` must undo a previous `=1`, without hand-editing settings.json.""" + _write_settings(isolated_settings_path, use_portable_settings=True) + monkeypatch.setenv("LORA_MANAGER_PORTABLE", "0") + + manager = SettingsManager() + + assert manager.get("use_portable_settings") is False + persisted = json.loads(isolated_settings_path.read_text(encoding="utf-8")) + # A default value is omitted from disk, so the key is gone entirely. + assert persisted.get("use_portable_settings") is None + + +def test_unset_env_keeps_the_persisted_flag( + isolated_settings_path, monkeypatch +): + """Portable mode must persist across runs when the variable is unset.""" + _write_settings(isolated_settings_path, use_portable_settings=True) + monkeypatch.delenv("LORA_MANAGER_PORTABLE", raising=False) + + manager = SettingsManager() + + assert manager.get("use_portable_settings") is True + + +def test_zero_is_a_noop_when_portable_was_never_enabled( + isolated_settings_path, monkeypatch +): + _write_settings(isolated_settings_path) + monkeypatch.setenv("LORA_MANAGER_PORTABLE", "0") + + manager = SettingsManager() + + assert manager.get("use_portable_settings") in (False, None) + + +def test_pinned_settings_dir_wins_over_portable_env( + isolated_settings_path, monkeypatch +): + """LORA_MANAGER_SETTINGS_DIR still takes precedence, as documented.""" + _write_settings(isolated_settings_path) + monkeypatch.setenv("LORA_MANAGER_PORTABLE", "1") + monkeypatch.setenv("LORA_MANAGER_SETTINGS_DIR", str(isolated_settings_path.parent)) + + manager = SettingsManager() + + # The pinned directory already decides the location, so the portable flag + # is deliberately left alone. + assert not manager.get("use_portable_settings") diff --git a/tests/utils/test_settings_paths.py b/tests/utils/test_settings_paths.py index 332d7e65..693a7690 100644 --- a/tests/utils/test_settings_paths.py +++ b/tests/utils/test_settings_paths.py @@ -34,10 +34,12 @@ class TestShouldUsePortableSettings: @pytest.mark.parametrize( "env_value, settings_flag, expected", [ - ("1", False, True), # env = 1 overrides settings.json false + ("1", False, True), # env = 1 forces portable on ("1", True, True), # env = 1 matches settings.json true - ("0", False, False), # env = 0 → rely on settings.json - ("0", True, True), # env = 0 → rely on settings.json + ("0", False, False), # env = 0 forces portable off + ("0", True, False), # env = 0 overrides a persisted true + ("yes", False, False), # unrecognised value → rely on settings.json + ("yes", True, True), # unrecognised value → rely on settings.json ("", False, False), # unset → rely on settings.json ("", True, True), # unset → rely on settings.json ], @@ -58,6 +60,21 @@ class TestShouldUsePortableSettings: result = _should_use_portable_settings(str(settings_file), logging.getLogger()) assert result == expected + def test_explicit_zero_is_the_documented_opt_out(self, tmp_path, caplog): + """`=0` must be honoured even against a persisted true flag.""" + settings_file = tmp_path / "settings.json" + settings_file.write_text(json.dumps({"use_portable_settings": True})) + + with pytest.MonkeyPatch.context() as mp: + mp.setenv("LORA_MANAGER_PORTABLE", "0") + with caplog.at_level(logging.INFO): + result = _should_use_portable_settings( + str(settings_file), logging.getLogger() + ) + + assert result is False + assert "Portable mode disabled" in caplog.text + def test_missing_file_without_env(self, tmp_path): """Without env var, missing settings file returns False.""" missing = tmp_path / "nonexistent.json"