From 446aa9b26baec8df1517bf289ef1e0c2935dfcbc Mon Sep 17 00:00:00 2001 From: Yif-Yang Date: Thu, 20 Aug 2026 18:35:00 +0000 Subject: [PATCH] fix: follow-up correctness fixes for #230 and #234 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects that shipped with the merged PRs: harvest_opencode: the APPDATA branch added by #230 was unreachable. `LOCALAPPDATA or APPDATA` resolves to the former in virtually every Windows session, so a database that really lives under Roaming was never found. Probe the candidate roots in order and pick the one that holds opencode.db, falling back to the Local root for messaging when no database exists yet; a relative OPENCODE_DB resolves below the same selected root. minimax_backend: configure_minimax_chat applied the region default unconditionally and wrote it into MINIMAX_BASE_URL, so a proxy or private gateway configured through the environment was silently discarded as soon as model.minimax_region was set — trainer and eval_only pass `cfg.get('minimax_base_url') or None`, i.e. None for the common case. Track whether the base URL was chosen explicitly and only fill in the region default when it was not, matching what the docs already state. Co-Authored-By: Claude Opus 5 --- skillopt/model/minimax_backend.py | 19 ++++++-- skillopt_sleep/harvest_opencode.py | 34 ++++++++++---- tests/test_harvest_opencode.py | 73 ++++++++++++++++++++++++++++++ tests/test_minimax_region.py | 40 +++++++++++++++- 4 files changed, 151 insertions(+), 15 deletions(-) diff --git a/skillopt/model/minimax_backend.py b/skillopt/model/minimax_backend.py index f1e25b9f..596924c6 100644 --- a/skillopt/model/minimax_backend.py +++ b/skillopt/model/minimax_backend.py @@ -46,6 +46,10 @@ def base_url_for_region(region: str | None) -> str: REGION = normalize_region(os.environ.get("MINIMAX_REGION")) +# An explicit base URL (a proxy or a private gateway) must survive a later +# region selection, so remember whether the current value was chosen by the +# user or merely derived from the region default. +_BASE_URL_EXPLICIT = bool(os.environ.get("MINIMAX_BASE_URL", "").strip()) BASE_URL = os.environ.get("MINIMAX_BASE_URL", "").strip() or base_url_for_region(REGION) API_KEY = os.environ.get("MINIMAX_API_KEY", "") TIMEOUT_SECONDS = float(os.environ.get("MINIMAX_TIMEOUT_SECONDS", "300") or 300) @@ -215,15 +219,20 @@ def configure_minimax_chat( enable_thinking: bool | str | None = None, ) -> None: global BASE_URL, API_KEY, TEMPERATURE, TIMEOUT_SECONDS, MAX_TOKENS, ENABLE_THINKING, REGION + global _BASE_URL_EXPLICIT with _config_lock: + if base_url is not None and str(base_url).strip(): + BASE_URL = str(base_url).strip() + _BASE_URL_EXPLICIT = True + os.environ["MINIMAX_BASE_URL"] = BASE_URL if region is not None: REGION = normalize_region(region) os.environ["MINIMAX_REGION"] = REGION - BASE_URL = base_url_for_region(REGION) - os.environ["MINIMAX_BASE_URL"] = BASE_URL - if base_url is not None: - BASE_URL = str(base_url).strip() or BASE_URL - os.environ["MINIMAX_BASE_URL"] = BASE_URL + # Only fill in the region default when no explicit base URL is in + # play; otherwise a configured proxy would be silently discarded. + if not _BASE_URL_EXPLICIT: + BASE_URL = base_url_for_region(REGION) + os.environ["MINIMAX_BASE_URL"] = BASE_URL if api_key is not None: API_KEY = str(api_key).strip() os.environ["MINIMAX_API_KEY"] = API_KEY diff --git a/skillopt_sleep/harvest_opencode.py b/skillopt_sleep/harvest_opencode.py index 0fb7e565..f68b9b95 100644 --- a/skillopt_sleep/harvest_opencode.py +++ b/skillopt_sleep/harvest_opencode.py @@ -44,17 +44,33 @@ } -def default_opencode_db() -> str: - """Return the OpenCode database selected by its environment variables.""" +def _opencode_data_candidates() -> List[str]: + """Where OpenCode may keep its data directory, most specific first.""" data_home = os.environ.get("XDG_DATA_HOME", "") if data_home: - data_dir = os.path.abspath(os.path.expanduser(data_home)) - elif sys.platform == "win32" and (os.environ.get("LOCALAPPDATA") or os.environ.get("APPDATA")): - win_appdata = os.environ.get("LOCALAPPDATA") or os.environ.get("APPDATA") or "" - data_dir = os.path.abspath(os.path.expanduser(win_appdata)) - else: - data_dir = os.path.join(os.path.expanduser("~"), ".local", "share") - opencode_data = os.path.join(data_dir, "opencode") + return [os.path.join(os.path.abspath(os.path.expanduser(data_home)), "opencode")] + + candidates: List[str] = [] + if sys.platform == "win32": + for base in (os.environ.get("LOCALAPPDATA"), os.environ.get("APPDATA")): + if base: + candidates.append( + os.path.join(os.path.abspath(os.path.expanduser(base)), "opencode") + ) + candidates.append( + os.path.join(os.path.expanduser("~"), ".local", "share", "opencode") + ) + return list(dict.fromkeys(candidates)) + + +def default_opencode_db() -> str: + """Return the OpenCode database selected by its environment variables.""" + candidates = _opencode_data_candidates() + opencode_data = candidates[0] + for candidate in candidates: + if os.path.exists(os.path.join(candidate, "opencode.db")): + opencode_data = candidate + break configured = os.environ.get("OPENCODE_DB", "") if configured == ":memory:": diff --git a/tests/test_harvest_opencode.py b/tests/test_harvest_opencode.py index acaf6a27..d54f4f5b 100644 --- a/tests/test_harvest_opencode.py +++ b/tests/test_harvest_opencode.py @@ -919,12 +919,85 @@ def test_default_database_honors_windows_appdata(monkeypatch, tmp_path: Path) -> monkeypatch.delenv("XDG_DATA_HOME", raising=False) monkeypatch.delenv("OPENCODE_DB", raising=False) monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setenv("HOME", str(tmp_path / "home")) + monkeypatch.setenv("USERPROFILE", str(tmp_path / "home")) monkeypatch.setenv("LOCALAPPDATA", str(local_app_data)) monkeypatch.delenv("APPDATA", raising=False) assert default_opencode_db() == os.path.abspath(local_app_data / "opencode" / "opencode.db") +def test_default_database_honors_windows_roaming_appdata(monkeypatch, tmp_path: Path) -> None: + """A session with only APPDATA set still resolves below Roaming.""" + roaming = tmp_path / "Roaming" + monkeypatch.delenv("XDG_DATA_HOME", raising=False) + monkeypatch.delenv("OPENCODE_DB", raising=False) + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setenv("HOME", str(tmp_path / "home")) + monkeypatch.setenv("USERPROFILE", str(tmp_path / "home")) + monkeypatch.delenv("LOCALAPPDATA", raising=False) + monkeypatch.setenv("APPDATA", str(roaming)) + + assert default_opencode_db() == os.path.abspath(roaming / "opencode" / "opencode.db") + + +def test_default_database_prefers_the_appdata_root_that_has_the_database( + monkeypatch, tmp_path: Path +) -> None: + """Both roots are set (the usual Windows session) but only Roaming has the db.""" + local_app_data = tmp_path / "LocalAppData" + roaming = tmp_path / "Roaming" + roaming_db = roaming / "opencode" / "opencode.db" + roaming_db.parent.mkdir(parents=True) + roaming_db.write_bytes(b"") + monkeypatch.delenv("XDG_DATA_HOME", raising=False) + monkeypatch.delenv("OPENCODE_DB", raising=False) + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setenv("LOCALAPPDATA", str(local_app_data)) + monkeypatch.setenv("APPDATA", str(roaming)) + + assert default_opencode_db() == os.path.abspath(roaming_db) + + +def test_default_database_prefers_local_appdata_when_neither_exists( + monkeypatch, tmp_path: Path +) -> None: + """With no database on disk the Local root stays the reported default.""" + local_app_data = tmp_path / "LocalAppData" + roaming = tmp_path / "Roaming" + monkeypatch.delenv("XDG_DATA_HOME", raising=False) + monkeypatch.delenv("OPENCODE_DB", raising=False) + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setenv("HOME", str(tmp_path / "home")) + monkeypatch.setenv("USERPROFILE", str(tmp_path / "home")) + monkeypatch.setenv("LOCALAPPDATA", str(local_app_data)) + monkeypatch.setenv("APPDATA", str(roaming)) + + assert default_opencode_db() == os.path.abspath( + local_app_data / "opencode" / "opencode.db" + ) + + +def test_relative_opencode_db_resolves_below_the_selected_windows_root( + monkeypatch, tmp_path: Path +) -> None: + """A relative OPENCODE_DB follows the root that actually holds the database.""" + local_app_data = tmp_path / "LocalAppData" + roaming = tmp_path / "Roaming" + roaming_db = roaming / "opencode" / "opencode.db" + roaming_db.parent.mkdir(parents=True) + roaming_db.write_bytes(b"") + monkeypatch.delenv("XDG_DATA_HOME", raising=False) + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setenv("LOCALAPPDATA", str(local_app_data)) + monkeypatch.setenv("APPDATA", str(roaming)) + monkeypatch.setenv("OPENCODE_DB", "nightly.db") + + assert default_opencode_db() == os.path.abspath( + roaming / "opencode" / "nightly.db" + ) + + def test_default_database_falls_back_to_home_local_share(monkeypatch, tmp_path: Path) -> None: home = tmp_path / "home" monkeypatch.delenv("XDG_DATA_HOME", raising=False) diff --git a/tests/test_minimax_region.py b/tests/test_minimax_region.py index 3dcf09ef..c9567c3d 100644 --- a/tests/test_minimax_region.py +++ b/tests/test_minimax_region.py @@ -14,7 +14,7 @@ _GLOBAL_BASE_URL = "https://api.minimax.io/v1" _CN_BASE_URL = "https://api.minimaxi.com/v1" _ENV_KEYS = ("MINIMAX_REGION", "MINIMAX_BASE_URL") -_GLOBAL_KEYS = ("REGION", "BASE_URL") +_GLOBAL_KEYS = ("REGION", "BASE_URL", "_BASE_URL_EXPLICIT") @pytest.fixture(autouse=True) @@ -116,3 +116,41 @@ def test_configure_rejects_unsupported_region() -> None: with pytest.raises(ValueError, match="Unsupported MiniMax region"): minimax_backend.configure_minimax_chat(region="apac") assert minimax_backend.get_base_url() == before + + +def test_env_base_url_survives_a_later_region_selection( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A proxy set via MINIMAX_BASE_URL is not clobbered by model.minimax_region.""" + module = _reload_with_env( + monkeypatch, + MINIMAX_REGION=None, + MINIMAX_BASE_URL="https://proxy.internal/v1", + ) + try: + module.configure_minimax_chat(region="cn_zh", base_url=None) + assert module.get_region() == "cn_zh" + assert module.get_base_url() == "https://proxy.internal/v1" + assert os.environ["MINIMAX_BASE_URL"] == "https://proxy.internal/v1" + finally: + monkeypatch.undo() + importlib.reload(minimax_backend) + + +def test_explicit_base_url_survives_a_later_region_only_call() -> None: + """Once configured explicitly, the base URL outlives subsequent region switches.""" + minimax_backend.configure_minimax_chat(base_url="https://proxy.internal/v1") + minimax_backend.configure_minimax_chat(region="cn_zh") + assert minimax_backend.get_region() == "cn_zh" + assert minimax_backend.get_base_url() == "https://proxy.internal/v1" + + minimax_backend.configure_minimax_chat(region="global_en") + assert minimax_backend.get_base_url() == "https://proxy.internal/v1" + + +def test_region_still_applies_when_base_url_is_blank() -> None: + """A blank base_url (the usual `cfg.get(...) or None`) keeps region selection working.""" + minimax_backend.configure_minimax_chat(region="cn_zh", base_url=None) + assert minimax_backend.get_base_url() == _CN_BASE_URL + minimax_backend.configure_minimax_chat(region="global_en", base_url="") + assert minimax_backend.get_base_url() == _GLOBAL_BASE_URL