From 7574a44c1b3c2eaf81c9f2ccc8670d362157e7a8 Mon Sep 17 00:00:00 2001 From: Padraic Shafer Date: Fri, 4 Sep 2026 14:25:44 -0700 Subject: [PATCH 01/14] fix(cli): migrate config from bare file to XDG-compliant path The CLI config was written to ~/.config/nsls2 as a plain file, colliding with other nsls2 tools that need ~/.config/nsls2/ as a directory. New location: $XDG_CONFIG_HOME/nsls2/api/cli.toml (falls back to ~/.config on POSIX, %APPDATA% on Windows). If the legacy bare file exists and is writable on first use, it is moved automatically to the new path so existing settings (base_url, token) are preserved. Non-writable legacy files emit a warning and are left in place; the tool continues normally. Assisted-by: claude-opus-4-8 Assisted-by: claude-sonnet-4-6 --- src/nsls2api/cli/settings.py | 73 ++++++++++++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 4 deletions(-) diff --git a/src/nsls2api/cli/settings.py b/src/nsls2api/cli/settings.py index f0c2d7aa..c947b7d5 100644 --- a/src/nsls2api/cli/settings.py +++ b/src/nsls2api/cli/settings.py @@ -1,5 +1,6 @@ import configparser import os +import sys from enum import Enum from pathlib import Path from typing import Any @@ -23,13 +24,77 @@ class Config: @staticmethod def get_filepath() -> Path: - """Get the configuration file path""" - config_user_home = os.path.expanduser("~") - return Path(config_user_home) / ".config" / "nsls2" + """Get the configuration file path ($XDG_CONFIG_HOME/nsls2/api/cli.toml). + + Respects XDG_CONFIG_HOME if set; falls back to ~/.config on POSIX and + %APPDATA% on Windows. + """ + if sys.platform == "win32": + appdata = os.environ.get("APPDATA", "") + base = Path(appdata) if appdata else Path.home() / "AppData" / "Roaming" + else: + xdg = os.environ.get("XDG_CONFIG_HOME", "").strip() + base = Path(xdg) if xdg else Path.home() / ".config" + return base / "nsls2" / "api" / "cli.toml" + + @staticmethod + def _legacy_filepath() -> Path: + """Returns the old (pre-migration) config path: ~/.config/nsls2 (a bare file).""" + return Path.home() / ".config" / "nsls2" + + @classmethod + def migrate_legacy_config(cls) -> Path | None: + """Migrate the legacy bare-file config to the new location, if applicable. + + If ``~/.config/nsls2`` exists as a **regular file** (the old format) and + the new config file does not yet exist, move the legacy file to the new + location, creating intermediate directories as needed. + + Returns the new path if migration occurred, ``None`` otherwise. + + Emits a warning to stderr and returns ``None`` (without raising) if the + move fails for any reason (e.g. permissions), so a migration failure + never breaks a normal CLI command. + """ + legacy = cls._legacy_filepath() + new = cls.get_filepath() + + # Already migrated (or user already has a new-style config) — nothing to do. + if new.exists(): + return None + + # Only migrate a plain FILE. If legacy is a directory, skip silently. + if not legacy.is_file(): + return None + + # Check writability before attempting the move. + if not os.access(legacy, os.W_OK): + print( + f"Warning: nsls2api config migration skipped — " + f"'{legacy}' is not writable. " + f"To migrate manually: mv '{legacy}' '{new}'", + file=sys.stderr, + ) + return None + + try: + new.parent.mkdir(parents=True, exist_ok=True) + legacy.replace(new) + except OSError as exc: + print( + f"Warning: nsls2api config migration failed ({exc}). " + f"Your settings remain at '{legacy}'. " + f"To migrate manually: mv '{legacy}' '{new}'", + file=sys.stderr, + ) + return None + + return new @classmethod def read(cls) -> configparser.ConfigParser: - """Read the configuration file""" + """Read the configuration file, migrating legacy config if present.""" + cls.migrate_legacy_config() config = configparser.ConfigParser() config_filepath = cls.get_filepath() config.read(config_filepath) From 020bdd326e34a51fef1341a769f29bd28d365e85 Mon Sep 17 00:00:00 2001 From: Padraic Shafer Date: Fri, 4 Sep 2026 14:44:06 -0700 Subject: [PATCH 02/14] feat(cli): add cli settings tests + fix migration staging Stage the legacy file to a temp sibling (via tempfile.mkstemp) to free the 'nsls2' name before creating the directory tree, then move it into place. On failure, roll back to the original location; if rollback also fails, the warning points at the temp path for manual recovery. All manual-migration hints now use the correct two-step (mv .bak && mkdir -p && mv) sequence instead of the previously impossible self-referential mv. Add tests/cli/test_settings.py covering path resolution, set/get round-trip, and legacy migration (self-collision regression, no-op cases, non-writable warn-and-proceed, and read()-triggered auto-migration). Assisted-by: claude-opus-4-8 Assisted-by: claude-sonnet-4-6 --- src/nsls2api/cli/settings.py | 53 +++++-- src/nsls2api/tests/cli/__init__.py | 0 src/nsls2api/tests/cli/test_settings.py | 193 ++++++++++++++++++++++++ 3 files changed, 237 insertions(+), 9 deletions(-) create mode 100644 src/nsls2api/tests/cli/__init__.py create mode 100644 src/nsls2api/tests/cli/test_settings.py diff --git a/src/nsls2api/cli/settings.py b/src/nsls2api/cli/settings.py index c947b7d5..48d711a8 100644 --- a/src/nsls2api/cli/settings.py +++ b/src/nsls2api/cli/settings.py @@ -1,6 +1,7 @@ import configparser import os import sys +import tempfile from enum import Enum from pathlib import Path from typing import Any @@ -50,6 +51,11 @@ def migrate_legacy_config(cls) -> Path | None: the new config file does not yet exist, move the legacy file to the new location, creating intermediate directories as needed. + Because the legacy file occupies the same name (``nsls2``) that must + become a *directory* for the new path, migration stages the legacy file + to a temporary sibling location first (via :func:`tempfile.mkstemp`) to + free the name before calling ``mkdir``. + Returns the new path if migration occurred, ``None`` otherwise. Emits a warning to stderr and returns ``None`` (without raising) if the @@ -72,21 +78,50 @@ def migrate_legacy_config(cls) -> Path | None: print( f"Warning: nsls2api config migration skipped — " f"'{legacy}' is not writable. " - f"To migrate manually: mv '{legacy}' '{new}'", + f"To migrate manually:\n" + f" mv '{legacy}' '{legacy}.bak' && " + f"mkdir -p '{new.parent}' && mv '{legacy}.bak' '{new}'", file=sys.stderr, ) return None + # Stage the legacy file to a temp sibling so the 'nsls2' name is freed + # before we try to create a directory with that same name. + fd, tmp_name = tempfile.mkstemp(dir=legacy.parent, prefix=".nsls2-migrate-") + os.close(fd) + tmp = Path(tmp_name) try: - new.parent.mkdir(parents=True, exist_ok=True) - legacy.replace(new) + legacy.replace(tmp) # free the 'nsls2' name + new.parent.mkdir(parents=True, exist_ok=True) # 'nsls2' can now be a dir + tmp.replace(new) # move into final position except OSError as exc: - print( - f"Warning: nsls2api config migration failed ({exc}). " - f"Your settings remain at '{legacy}'. " - f"To migrate manually: mv '{legacy}' '{new}'", - file=sys.stderr, - ) + # Best-effort rollback: restore config to its original location. + restored = False + if tmp.exists() and not legacy.exists(): + try: + tmp.replace(legacy) + restored = True + except OSError: + pass + if restored: + print( + f"Warning: nsls2api config migration failed ({exc}). " + f"Your settings remain at '{legacy}'. " + f"To migrate manually:\n" + f" mv '{legacy}' '{legacy}.bak' && " + f"mkdir -p '{new.parent}' && mv '{legacy}.bak' '{new}'", + file=sys.stderr, + ) + else: + # Rollback also failed — settings are at the temp path. + print( + f"Warning: nsls2api config migration failed ({exc}) and " + f"could not be rolled back. " + f"Your settings are at '{tmp}'. " + f"To recover:\n" + f" mkdir -p '{new.parent}' && mv '{tmp}' '{new}'", + file=sys.stderr, + ) return None return new diff --git a/src/nsls2api/tests/cli/__init__.py b/src/nsls2api/tests/cli/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/src/nsls2api/tests/cli/test_settings.py b/src/nsls2api/tests/cli/test_settings.py new file mode 100644 index 00000000..badb61d0 --- /dev/null +++ b/src/nsls2api/tests/cli/test_settings.py @@ -0,0 +1,193 @@ +"""Tests for nsls2api.cli.settings — config path, read/write, and legacy migration.""" + +from __future__ import annotations + +import configparser +import os +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest + +from nsls2api.cli.settings import Config, ConfigKey + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _make_legacy(home: Path, content: str = "[api]\nbase_url = http://127.0.0.1:8080\n") -> Path: + """Create the legacy bare-file config at /.config/nsls2.""" + legacy = home / ".config" / "nsls2" + legacy.parent.mkdir(parents=True, exist_ok=True) + legacy.write_text(content) + return legacy + + +def _patch_home(tmp_path: Path): + """Context manager that redirects Path.home() and HOME to tmp_path.""" + return patch.object(Path, "home", return_value=tmp_path) + + +# --------------------------------------------------------------------------- +# get_filepath — path resolution +# --------------------------------------------------------------------------- + +class TestGetFilepath: + def test_default_posix_path(self, tmp_path: Path): + """Without XDG_CONFIG_HOME, resolves to ~/.config/nsls2/api/cli.toml.""" + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + result = Config.get_filepath() + assert result == tmp_path / ".config" / "nsls2" / "api" / "cli.toml" + + def test_respects_xdg_config_home(self, tmp_path: Path): + """XDG_CONFIG_HOME is honoured when set.""" + xdg = str(tmp_path / "xdg") + with _patch_home(tmp_path), patch.dict(os.environ, {"XDG_CONFIG_HOME": xdg}): + result = Config.get_filepath() + assert result == tmp_path / "xdg" / "nsls2" / "api" / "cli.toml" + + def test_blank_xdg_config_home_falls_back(self, tmp_path: Path): + """A blank XDG_CONFIG_HOME is treated as unset.""" + with _patch_home(tmp_path), patch.dict(os.environ, {"XDG_CONFIG_HOME": " "}): + result = Config.get_filepath() + assert result == tmp_path / ".config" / "nsls2" / "api" / "cli.toml" + + +# --------------------------------------------------------------------------- +# set_value / read — basic round-trip +# --------------------------------------------------------------------------- + +class TestSetValueRead: + def test_set_creates_dirs_and_file(self, tmp_path: Path): + """set_value creates the nsls2/api/ directory and writes cli.toml.""" + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + Config.set_value("api", ConfigKey.BASE_URL, "https://example.com") + filepath = Config.get_filepath() + + assert filepath.exists() + assert filepath.parent.is_dir() + cfg = configparser.ConfigParser() + cfg.read(filepath) + assert cfg.get("api", "base_url") == "https://example.com" + + def test_get_value_round_trip(self, tmp_path: Path): + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + Config.set_value("api", ConfigKey.TOKEN, "tok123") + result = Config.get_value("api", ConfigKey.TOKEN) + assert result == "tok123" + + def test_get_value_missing_returns_none(self, tmp_path: Path): + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + result = Config.get_value("api", ConfigKey.BASE_URL) + assert result is None + + +# --------------------------------------------------------------------------- +# migrate_legacy_config +# --------------------------------------------------------------------------- + +class TestMigrateLegacyConfig: + def test_migrates_legacy_file(self, tmp_path: Path): + """Legacy bare file is moved to the new location; content is preserved. + + This is the key self-collision regression test: the legacy file occupies + the path component that must become a directory, so staging via a temp + file is required. After migration: + - ~/.config/nsls2 must be a DIRECTORY (not a file) + - ~/.config/nsls2/api/cli.toml must exist with original content + - no staging temp files remain in ~/.config/ + """ + legacy = _make_legacy(tmp_path) + dot_config = tmp_path / ".config" + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + new_path = Config.migrate_legacy_config() + + assert new_path is not None + assert new_path == tmp_path / ".config" / "nsls2" / "api" / "cli.toml" + assert new_path.exists() + assert not legacy.exists() # bare file is gone … + assert legacy.is_dir() # … and replaced by a directory + content = new_path.read_text() + assert "base_url" in content + assert "127.0.0.1:8080" in content + # No staging temp files left behind. + leftover = list(dot_config.glob(".nsls2-migrate-*")) + assert leftover == [], f"Unexpected temp files left over: {leftover}" + + def test_no_op_when_new_already_exists(self, tmp_path: Path): + """Migration is skipped when the new config file is already present.""" + legacy = _make_legacy(tmp_path) + new = tmp_path / ".config" / "nsls2" / "api" / "cli.toml" + new.parent.mkdir(parents=True, exist_ok=True) + new.write_text("[api]\nbase_url = https://already.here\n") + + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + result = Config.migrate_legacy_config() + + assert result is None + assert legacy.exists() # legacy untouched + assert new.read_text().startswith("[api]\nbase_url = https://already.here") + + def test_no_op_when_legacy_absent(self, tmp_path: Path): + """Migration is a no-op when the legacy file doesn't exist.""" + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + result = Config.migrate_legacy_config() + assert result is None + + def test_no_op_when_legacy_is_directory(self, tmp_path: Path): + """Migration skips silently if ~/.config/nsls2 is already a directory.""" + legacy_dir = tmp_path / ".config" / "nsls2" + legacy_dir.mkdir(parents=True, exist_ok=True) + (legacy_dir / "cli").mkdir() + + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + result = Config.migrate_legacy_config() + assert result is None + + @pytest.mark.skipif(sys.platform == "win32", reason="chmod not meaningful on Windows") + def test_warns_and_proceeds_when_not_writable(self, tmp_path: Path, capsys): + """Non-writable legacy file: warning is printed with two-step manual hint; + no exception raised; returns None.""" + legacy = _make_legacy(tmp_path) + legacy.chmod(0o444) # read-only + + try: + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + new_path = Config.get_filepath() + result = Config.migrate_legacy_config() + finally: + legacy.chmod(0o644) # restore so tmp_path cleanup works + + assert result is None + captured = capsys.readouterr() + assert "migration skipped" in captured.err + assert str(legacy) in captured.err + # Hint must be the correct two-step sequence, not the old impossible mv. + assert "mkdir -p" in captured.err + assert str(new_path.parent) in captured.err + + def test_migration_triggered_by_read(self, tmp_path: Path): + """Calling read() auto-migrates the legacy file and returns its contents.""" + _make_legacy(tmp_path, "[api]\nbase_url = http://127.0.0.1:8080\ntoken = abc\n") + legacy = tmp_path / ".config" / "nsls2" + + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + cfg = Config.read() + + assert cfg.get("api", "base_url") == "http://127.0.0.1:8080" + assert cfg.get("api", "token") == "abc" + assert not legacy.exists() + new = tmp_path / ".config" / "nsls2" / "api" / "cli.toml" + assert new.exists() From 7721954ac2750d42d32ffce31019e3731d3ac59c Mon Sep 17 00:00:00 2001 From: Padraic Shafer Date: Fri, 4 Sep 2026 14:50:13 -0700 Subject: [PATCH 03/14] fix(cli): name config file cli.ini to match INI format Config uses configparser, which reads/writes INI format, not TOML. Rename cli.toml -> cli.ini so the extension reflects the actual on-disk format. No on-disk cli.toml files exist yet, so no back-compat migration is required. Assisted-by: claude-opus-4-8 Assisted-by: claude-sonnet-4-6 --- src/nsls2api/cli/settings.py | 4 ++-- src/nsls2api/tests/cli/test_settings.py | 18 +++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/nsls2api/cli/settings.py b/src/nsls2api/cli/settings.py index 48d711a8..52d9ef0f 100644 --- a/src/nsls2api/cli/settings.py +++ b/src/nsls2api/cli/settings.py @@ -25,7 +25,7 @@ class Config: @staticmethod def get_filepath() -> Path: - """Get the configuration file path ($XDG_CONFIG_HOME/nsls2/api/cli.toml). + """Get the configuration file path ($XDG_CONFIG_HOME/nsls2/api/cli.ini). Respects XDG_CONFIG_HOME if set; falls back to ~/.config on POSIX and %APPDATA% on Windows. @@ -36,7 +36,7 @@ def get_filepath() -> Path: else: xdg = os.environ.get("XDG_CONFIG_HOME", "").strip() base = Path(xdg) if xdg else Path.home() / ".config" - return base / "nsls2" / "api" / "cli.toml" + return base / "nsls2" / "api" / "cli.ini" @staticmethod def _legacy_filepath() -> Path: diff --git a/src/nsls2api/tests/cli/test_settings.py b/src/nsls2api/tests/cli/test_settings.py index badb61d0..e38d3cca 100644 --- a/src/nsls2api/tests/cli/test_settings.py +++ b/src/nsls2api/tests/cli/test_settings.py @@ -36,24 +36,24 @@ def _patch_home(tmp_path: Path): class TestGetFilepath: def test_default_posix_path(self, tmp_path: Path): - """Without XDG_CONFIG_HOME, resolves to ~/.config/nsls2/api/cli.toml.""" + """Without XDG_CONFIG_HOME, resolves to ~/.config/nsls2/api/cli.ini.""" with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): os.environ.pop("XDG_CONFIG_HOME", None) result = Config.get_filepath() - assert result == tmp_path / ".config" / "nsls2" / "api" / "cli.toml" + assert result == tmp_path / ".config" / "nsls2" / "api" / "cli.ini" def test_respects_xdg_config_home(self, tmp_path: Path): """XDG_CONFIG_HOME is honoured when set.""" xdg = str(tmp_path / "xdg") with _patch_home(tmp_path), patch.dict(os.environ, {"XDG_CONFIG_HOME": xdg}): result = Config.get_filepath() - assert result == tmp_path / "xdg" / "nsls2" / "api" / "cli.toml" + assert result == tmp_path / "xdg" / "nsls2" / "api" / "cli.ini" def test_blank_xdg_config_home_falls_back(self, tmp_path: Path): """A blank XDG_CONFIG_HOME is treated as unset.""" with _patch_home(tmp_path), patch.dict(os.environ, {"XDG_CONFIG_HOME": " "}): result = Config.get_filepath() - assert result == tmp_path / ".config" / "nsls2" / "api" / "cli.toml" + assert result == tmp_path / ".config" / "nsls2" / "api" / "cli.ini" # --------------------------------------------------------------------------- @@ -62,7 +62,7 @@ def test_blank_xdg_config_home_falls_back(self, tmp_path: Path): class TestSetValueRead: def test_set_creates_dirs_and_file(self, tmp_path: Path): - """set_value creates the nsls2/api/ directory and writes cli.toml.""" + """set_value creates the nsls2/api/ directory and writes cli.ini.""" with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): os.environ.pop("XDG_CONFIG_HOME", None) Config.set_value("api", ConfigKey.BASE_URL, "https://example.com") @@ -100,7 +100,7 @@ def test_migrates_legacy_file(self, tmp_path: Path): the path component that must become a directory, so staging via a temp file is required. After migration: - ~/.config/nsls2 must be a DIRECTORY (not a file) - - ~/.config/nsls2/api/cli.toml must exist with original content + - ~/.config/nsls2/api/cli.ini must exist with original content - no staging temp files remain in ~/.config/ """ legacy = _make_legacy(tmp_path) @@ -110,7 +110,7 @@ def test_migrates_legacy_file(self, tmp_path: Path): new_path = Config.migrate_legacy_config() assert new_path is not None - assert new_path == tmp_path / ".config" / "nsls2" / "api" / "cli.toml" + assert new_path == tmp_path / ".config" / "nsls2" / "api" / "cli.ini" assert new_path.exists() assert not legacy.exists() # bare file is gone … assert legacy.is_dir() # … and replaced by a directory @@ -124,7 +124,7 @@ def test_migrates_legacy_file(self, tmp_path: Path): def test_no_op_when_new_already_exists(self, tmp_path: Path): """Migration is skipped when the new config file is already present.""" legacy = _make_legacy(tmp_path) - new = tmp_path / ".config" / "nsls2" / "api" / "cli.toml" + new = tmp_path / ".config" / "nsls2" / "api" / "cli.ini" new.parent.mkdir(parents=True, exist_ok=True) new.write_text("[api]\nbase_url = https://already.here\n") @@ -189,5 +189,5 @@ def test_migration_triggered_by_read(self, tmp_path: Path): assert cfg.get("api", "base_url") == "http://127.0.0.1:8080" assert cfg.get("api", "token") == "abc" assert not legacy.exists() - new = tmp_path / ".config" / "nsls2" / "api" / "cli.toml" + new = tmp_path / ".config" / "nsls2" / "api" / "cli.ini" assert new.exists() From 68436a4a15d5bd1f6393bca64095eba732f19b1d Mon Sep 17 00:00:00 2001 From: Padraic Shafer Date: Fri, 4 Sep 2026 15:08:41 -0700 Subject: [PATCH 04/14] =?UTF-8?q?fix(cli):=20address=20PR=20review=20?= =?UTF-8?q?=E2=80=94=20robust=20migration=20error=20paths,=20legacy=20fall?= =?UTF-8?q?back?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R3: restructure the migration except-block to correctly distinguish three failure cases: (1) staging failed (legacy intact) — clean up the empty temp file, report settings remain at legacy; (2) post-staging failure with rollback — report settings remain at legacy; (3) rollback also failed — report settings at temp path for manual recovery. The previous code fell into case (3) even when legacy was intact, leaking a temp file and giving wrong recovery guidance. R4: read() now falls back to reading the legacy bare file when the new config is absent (e.g. migration was skipped because legacy was not writable), so existing base_url/token are never silently dropped. R1/EXDEV: the final tmp→new move now uses shutil.move() (copy+unlink on cross-device moves) so migration works when $XDG_CONFIG_HOME is on a different filesystem than ~/.config. The staging step (same-directory legacy→tmp) remains a Path.replace() as it cannot cross filesystems. R5: add TestGetFilepathWindowsSimulated covering the win32 branch of get_filepath() with and without APPDATA, runnable on all platforms via sys.platform patching. R6: correct _patch_home docstring — it only patches Path.home(); env vars are controlled per-test. Assisted-by: claude-opus-4-8 Assisted-by: claude-sonnet-4-6 --- src/nsls2api/cli/settings.py | 66 ++++++---- src/nsls2api/tests/cli/test_settings.py | 164 +++++++++++++++++++++++- 2 files changed, 206 insertions(+), 24 deletions(-) diff --git a/src/nsls2api/cli/settings.py b/src/nsls2api/cli/settings.py index 52d9ef0f..eaf03784 100644 --- a/src/nsls2api/cli/settings.py +++ b/src/nsls2api/cli/settings.py @@ -1,5 +1,6 @@ import configparser import os +import shutil import sys import tempfile from enum import Enum @@ -93,17 +94,11 @@ def migrate_legacy_config(cls) -> Path | None: try: legacy.replace(tmp) # free the 'nsls2' name new.parent.mkdir(parents=True, exist_ok=True) # 'nsls2' can now be a dir - tmp.replace(new) # move into final position + shutil.move(str(tmp), str(new)) # cross-device safe final move except OSError as exc: - # Best-effort rollback: restore config to its original location. - restored = False - if tmp.exists() and not legacy.exists(): - try: - tmp.replace(legacy) - restored = True - except OSError: - pass - if restored: + if legacy.exists(): + # Staging step failed — legacy is intact; clean up the empty temp file. + tmp.unlink(missing_ok=True) print( f"Warning: nsls2api config migration failed ({exc}). " f"Your settings remain at '{legacy}'. " @@ -113,26 +108,53 @@ def migrate_legacy_config(cls) -> Path | None: file=sys.stderr, ) else: - # Rollback also failed — settings are at the temp path. - print( - f"Warning: nsls2api config migration failed ({exc}) and " - f"could not be rolled back. " - f"Your settings are at '{tmp}'. " - f"To recover:\n" - f" mkdir -p '{new.parent}' && mv '{tmp}' '{new}'", - file=sys.stderr, - ) + # Staging succeeded but a later step failed. Try to restore legacy. + restored = False + try: + tmp.replace(legacy) + restored = True + except OSError: + pass + if restored: + print( + f"Warning: nsls2api config migration failed ({exc}). " + f"Your settings remain at '{legacy}'. " + f"To migrate manually:\n" + f" mv '{legacy}' '{legacy}.bak' && " + f"mkdir -p '{new.parent}' && mv '{legacy}.bak' '{new}'", + file=sys.stderr, + ) + else: + # Rollback also failed — settings are at the temp path. + print( + f"Warning: nsls2api config migration failed ({exc}) and " + f"could not be rolled back. " + f"Your settings are at '{tmp}'. " + f"To recover:\n" + f" mkdir -p '{new.parent}' && mv '{tmp}' '{new}'", + file=sys.stderr, + ) return None return new @classmethod def read(cls) -> configparser.ConfigParser: - """Read the configuration file, migrating legacy config if present.""" + """Read the configuration file, migrating legacy config if present. + + If migration was skipped or failed and the new config file does not yet + exist, fall back to reading the legacy bare file so existing settings + (base_url, token) are not silently dropped. + """ cls.migrate_legacy_config() config = configparser.ConfigParser() - config_filepath = cls.get_filepath() - config.read(config_filepath) + new = cls.get_filepath() + if new.exists(): + config.read(new) + else: + legacy = cls._legacy_filepath() + if legacy.is_file(): + config.read(legacy) # back-compat: unmigrated settings still honored return config @classmethod diff --git a/src/nsls2api/tests/cli/test_settings.py b/src/nsls2api/tests/cli/test_settings.py index e38d3cca..7ce1c298 100644 --- a/src/nsls2api/tests/cli/test_settings.py +++ b/src/nsls2api/tests/cli/test_settings.py @@ -6,7 +6,7 @@ import os import sys from pathlib import Path -from unittest.mock import patch +from unittest.mock import patch, MagicMock import pytest @@ -26,7 +26,7 @@ def _make_legacy(home: Path, content: str = "[api]\nbase_url = http://127.0.0.1: def _patch_home(tmp_path: Path): - """Context manager that redirects Path.home() and HOME to tmp_path.""" + """Redirect Path.home() to tmp_path. Env vars are controlled per-test.""" return patch.object(Path, "home", return_value=tmp_path) @@ -191,3 +191,163 @@ def test_migration_triggered_by_read(self, tmp_path: Path): assert not legacy.exists() new = tmp_path / ".config" / "nsls2" / "api" / "cli.ini" assert new.exists() + + @pytest.mark.skipif(sys.platform == "win32", reason="chmod not meaningful on Windows") + def test_staging_failure_cleans_up_temp_and_reports_legacy( + self, tmp_path: Path, capsys + ): + """If the initial legacy→tmp staging step raises, migrate_legacy_config(): + - returns None + - legacy file is still intact + - no .nsls2-migrate-* temp files are left behind + - warning says settings remain at legacy (not at a temp path) + """ + legacy = _make_legacy(tmp_path) + dot_config = tmp_path / ".config" + + original_replace = Path.replace + + call_count = 0 + + def replace_that_fails_on_first_call(self, target): + nonlocal call_count + call_count += 1 + if call_count == 1: + raise OSError("simulated staging failure") + return original_replace(self, target) + + with ( + _patch_home(tmp_path), + patch.dict(os.environ, {}, clear=False), + patch.object(Path, "replace", replace_that_fails_on_first_call), + ): + os.environ.pop("XDG_CONFIG_HOME", None) + result = Config.migrate_legacy_config() + + assert result is None + assert legacy.exists() # legacy untouched + assert legacy.read_text().startswith("[api]") + leftover = list(dot_config.glob(".nsls2-migrate-*")) + assert leftover == [], f"Temp files leaked: {leftover}" + captured = capsys.readouterr() + assert "remain at" in captured.err + assert "settings are at" not in captured.err # must NOT claim settings at temp + + def test_shutil_move_failure_triggers_rollback(self, tmp_path: Path, capsys): + """If shutil.move raises (e.g. cross-device), the legacy file is restored + and the warning says settings remain at legacy.""" + import nsls2api.cli.settings as settings_mod + + legacy = _make_legacy(tmp_path) + + with ( + _patch_home(tmp_path), + patch.dict(os.environ, {}, clear=False), + patch.object(settings_mod.shutil, "move", side_effect=OSError("EXDEV")), + ): + os.environ.pop("XDG_CONFIG_HOME", None) + result = Config.migrate_legacy_config() + + assert result is None + assert legacy.exists() # rolled back + assert legacy.read_text().startswith("[api]") + captured = capsys.readouterr() + assert "remain at" in captured.err + + +# --------------------------------------------------------------------------- +# R4 — read() legacy fallback +# --------------------------------------------------------------------------- + +class TestReadLegacyFallback: + """When migration is skipped/failed, read() must still return legacy values.""" + + @pytest.mark.skipif(sys.platform == "win32", reason="chmod not meaningful on Windows") + def test_read_falls_back_to_legacy_when_migration_skipped( + self, tmp_path: Path, capsys + ): + """Non-writable legacy → migration skipped; read() should still return + the legacy base_url rather than an empty config.""" + legacy = _make_legacy( + tmp_path, "[api]\nbase_url = http://127.0.0.1:8080\ntoken = mytoken\n" + ) + legacy.chmod(0o444) + + try: + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + cfg = Config.read() + finally: + legacy.chmod(0o644) + + assert cfg.get("api", "base_url") == "http://127.0.0.1:8080" + assert cfg.get("api", "token") == "mytoken" + + def test_read_falls_back_to_legacy_when_new_absent(self, tmp_path: Path): + """With no new config and no migration trigger, read() reads the legacy file.""" + _make_legacy( + tmp_path, "[api]\nbase_url = https://api.example.com\n" + ) + # Do not create the new-style config at all. + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + # Suppress migration (legacy is present but so is its path — we want + # to test the fallback directly, so skip the migration by marking the + # legacy as a dir is too invasive; instead just confirm read() returns + # legacy values when new is absent after a no-op migration). + cfg = Config.read() + + # Migration will have run and succeeded here (normal case), but if for any + # reason it didn't, the fallback should still work. Assert the value is present + # regardless of whether migration ran. + assert cfg.get("api", "base_url") == "https://api.example.com" + + def test_read_returns_empty_config_when_both_absent(self, tmp_path: Path): + """When neither new nor legacy config exists, read() returns an empty config.""" + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + cfg = Config.read() + assert not cfg.sections() + + +# --------------------------------------------------------------------------- +# R5 — Windows branch of get_filepath() +# --------------------------------------------------------------------------- + +@pytest.mark.skipif(sys.platform != "win32", reason="Only run on Windows") +class TestGetFilepathWindows: + """These tests patch sys.platform and run only on Windows.""" + + def test_windows_path_with_appdata(self, tmp_path: Path, monkeypatch): + """With APPDATA set, uses %APPDATA%/nsls2/api/cli.ini.""" + appdata = str(tmp_path / "AppData" / "Roaming") + monkeypatch.setenv("APPDATA", appdata) + result = Config.get_filepath() + assert result == tmp_path / "AppData" / "Roaming" / "nsls2" / "api" / "cli.ini" + + def test_windows_path_without_appdata(self, tmp_path: Path, monkeypatch): + """Without APPDATA, falls back to Path.home()/AppData/Roaming/...""" + monkeypatch.delenv("APPDATA", raising=False) + with _patch_home(tmp_path): + result = Config.get_filepath() + assert result == tmp_path / "AppData" / "Roaming" / "nsls2" / "api" / "cli.ini" + + +class TestGetFilepathWindowsSimulated: + """Simulate the Windows branch on all platforms via sys.platform patching.""" + + def test_windows_path_with_appdata_simulated(self, tmp_path: Path, monkeypatch): + """Patch sys.platform to win32; APPDATA set → uses APPDATA path.""" + appdata = str(tmp_path / "AppData" / "Roaming") + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setenv("APPDATA", appdata) + result = Config.get_filepath() + assert result == tmp_path / "AppData" / "Roaming" / "nsls2" / "api" / "cli.ini" + + def test_windows_path_without_appdata_simulated(self, tmp_path: Path, monkeypatch): + """Patch sys.platform to win32; no APPDATA → falls back to home.""" + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.delenv("APPDATA", raising=False) + with _patch_home(tmp_path): + result = Config.get_filepath() + assert result == tmp_path / "AppData" / "Roaming" / "nsls2" / "api" / "cli.ini" From e568205df8f2fa2dd916f28ccd65d608646f1563 Mon Sep 17 00:00:00 2001 From: Padraic Shafer Date: Fri, 4 Sep 2026 15:20:12 -0700 Subject: [PATCH 05/14] fix(cli): prevent config loss on post-staging migration failure Use a `staged` bool flag (set after legacy.replace(tmp) succeeds) instead of `legacy.exists()` to discriminate staging-failed vs later-failed in migrate_legacy_config(). The old check was fooled when new.parent.mkdir() created ~/.config/nsls2/ as a directory, causing tmp.unlink() to delete the only copy of user settings. Also fix two contradictory test assertions (assert not legacy.exists() vs assert legacy.is_dir()), remove unused MagicMock import (F401), and add a regression test for the data-loss scenario. Assisted-by: claude-opus-4-8 Assisted-by: claude-sonnet-4-6 --- src/nsls2api/cli/settings.py | 9 +++-- src/nsls2api/tests/cli/test_settings.py | 44 ++++++++++++++++++++++--- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/src/nsls2api/cli/settings.py b/src/nsls2api/cli/settings.py index eaf03784..a4f63240 100644 --- a/src/nsls2api/cli/settings.py +++ b/src/nsls2api/cli/settings.py @@ -91,13 +91,15 @@ def migrate_legacy_config(cls) -> Path | None: fd, tmp_name = tempfile.mkstemp(dir=legacy.parent, prefix=".nsls2-migrate-") os.close(fd) tmp = Path(tmp_name) + staged = False # True once legacy.replace(tmp) succeeds and tmp holds the data try: legacy.replace(tmp) # free the 'nsls2' name + staged = True new.parent.mkdir(parents=True, exist_ok=True) # 'nsls2' can now be a dir shutil.move(str(tmp), str(new)) # cross-device safe final move except OSError as exc: - if legacy.exists(): - # Staging step failed — legacy is intact; clean up the empty temp file. + if not staged: + # Staging step failed — legacy is still intact; discard empty temp file. tmp.unlink(missing_ok=True) print( f"Warning: nsls2api config migration failed ({exc}). " @@ -108,7 +110,8 @@ def migrate_legacy_config(cls) -> Path | None: file=sys.stderr, ) else: - # Staging succeeded but a later step failed. Try to restore legacy. + # Staging succeeded (tmp holds the data); a later step failed. + # Try to restore the legacy file so the user isn't left without config. restored = False try: tmp.replace(legacy) diff --git a/src/nsls2api/tests/cli/test_settings.py b/src/nsls2api/tests/cli/test_settings.py index 7ce1c298..dcfff95c 100644 --- a/src/nsls2api/tests/cli/test_settings.py +++ b/src/nsls2api/tests/cli/test_settings.py @@ -6,7 +6,7 @@ import os import sys from pathlib import Path -from unittest.mock import patch, MagicMock +from unittest.mock import patch import pytest @@ -112,8 +112,7 @@ def test_migrates_legacy_file(self, tmp_path: Path): assert new_path is not None assert new_path == tmp_path / ".config" / "nsls2" / "api" / "cli.ini" assert new_path.exists() - assert not legacy.exists() # bare file is gone … - assert legacy.is_dir() # … and replaced by a directory + assert legacy.is_dir() # bare file replaced by a directory content = new_path.read_text() assert "base_url" in content assert "127.0.0.1:8080" in content @@ -188,7 +187,7 @@ def test_migration_triggered_by_read(self, tmp_path: Path): assert cfg.get("api", "base_url") == "http://127.0.0.1:8080" assert cfg.get("api", "token") == "abc" - assert not legacy.exists() + assert legacy.is_dir() # bare file replaced by a directory new = tmp_path / ".config" / "nsls2" / "api" / "cli.ini" assert new.exists() @@ -254,6 +253,43 @@ def test_shutil_move_failure_triggers_rollback(self, tmp_path: Path, capsys): captured = capsys.readouterr() assert "remain at" in captured.err + def test_shutil_move_failure_after_mkdir_does_not_lose_data( + self, tmp_path: Path, capsys + ): + """Regression: when shutil.move fails AFTER mkdir has created the nsls2/ + directory, the old legacy.exists() check would see the newly-created dir + and delete the temp file (the only copy of user settings) — data loss. + + With the staged-flag fix, the code correctly identifies that staging + succeeded (tmp holds the data) and attempts rollback regardless of whether + legacy.exists() is True or False. + """ + import nsls2api.cli.settings as settings_mod + + content = "[api]\nbase_url = http://127.0.0.1:8080\ntoken = secret\n" + legacy = _make_legacy(tmp_path, content) + + # mkdir runs normally so the nsls2/ directory gets created first, + # then shutil.move raises — the old code saw legacy.exists()==True (dir!) + # and deleted the temp file (data loss). The staged-flag fix avoids this. + with ( + _patch_home(tmp_path), + patch.dict(os.environ, {}, clear=False), + patch.object(settings_mod.shutil, "move", side_effect=OSError("EXDEV after mkdir")), + ): + os.environ.pop("XDG_CONFIG_HOME", None) + result = Config.migrate_legacy_config() + + assert result is None + # The legacy file must be restored (rollback succeeded) — no data loss. + assert legacy.exists(), "Legacy config was lost after shutil.move failure!" + assert legacy.is_file(), "Legacy config should be a file after rollback" + assert "base_url" in legacy.read_text(), "Legacy config content was lost!" + assert "secret" in legacy.read_text(), "Token was lost!" + captured = capsys.readouterr() + assert "remain at" in captured.err + assert "settings are at" not in captured.err + # --------------------------------------------------------------------------- # R4 — read() legacy fallback From 7100155fbafddd2f781dfddda951e45576d90e0c Mon Sep 17 00:00:00 2001 From: Padraic Shafer Date: Fri, 4 Sep 2026 15:36:50 -0700 Subject: [PATCH 06/14] fix(cli): fix POSIX rollback after mkdir; platform-neutral migration hints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On POSIX, shutil.move failure after new.parent.mkdir() left ~/.config/nsls2 as a directory, causing tmp.replace(legacy) to raise ENOTEMPTY/EISDIR — rollback never succeeded. Fix: rmtree(new.parent) + legacy.rmdir() before tmp.replace(legacy) clears the empty dirs we created, restoring the legacy file reliably. Also replace POSIX-only mv/mkdir -p hints in all four warning messages with platform-neutral numbered step lists (addresses Windows users). Fix tests: assert legacy.is_file() after rollback, patch migrate_legacy_config to no-op in R4 fallback test, update hint-text assertions. Assisted-by: claude-opus-4-8 Assisted-by: claude-sonnet-4-6 --- src/nsls2api/cli/settings.py | 30 ++++++++++++++++++------- src/nsls2api/tests/cli/test_settings.py | 27 +++++++++++----------- 2 files changed, 35 insertions(+), 22 deletions(-) diff --git a/src/nsls2api/cli/settings.py b/src/nsls2api/cli/settings.py index a4f63240..821965cc 100644 --- a/src/nsls2api/cli/settings.py +++ b/src/nsls2api/cli/settings.py @@ -80,8 +80,9 @@ def migrate_legacy_config(cls) -> Path | None: f"Warning: nsls2api config migration skipped — " f"'{legacy}' is not writable. " f"To migrate manually:\n" - f" mv '{legacy}' '{legacy}.bak' && " - f"mkdir -p '{new.parent}' && mv '{legacy}.bak' '{new}'", + f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" + f" 2. Create the directory '{new.parent}'\n" + f" 3. Move the saved file into place as '{new}'", file=sys.stderr, ) return None @@ -105,15 +106,26 @@ def migrate_legacy_config(cls) -> Path | None: f"Warning: nsls2api config migration failed ({exc}). " f"Your settings remain at '{legacy}'. " f"To migrate manually:\n" - f" mv '{legacy}' '{legacy}.bak' && " - f"mkdir -p '{new.parent}' && mv '{legacy}.bak' '{new}'", + f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" + f" 2. Create the directory '{new.parent}'\n" + f" 3. Move the saved file into place as '{new}'", file=sys.stderr, ) else: # Staging succeeded (tmp holds the data); a later step failed. - # Try to restore the legacy file so the user isn't left without config. + # new.parent.mkdir() may have created ~/.config/nsls2/api/ and + # ~/.config/nsls2/ (which occupies the legacy name as a directory). + # Remove those empty dirs so the legacy name is free to receive the + # file again. rmtree is limited to new.parent (the api/ subtree); + # legacy.rmdir() only succeeds when the dir is empty — if it + # unexpectedly holds other content it raises and we fall through + # to the "settings at tmp" message, never deleting real data. restored = False try: + if new.parent.is_dir(): + shutil.rmtree(new.parent, ignore_errors=True) # …/nsls2/api + if legacy.is_dir(): + legacy.rmdir() # empty …/nsls2 tmp.replace(legacy) restored = True except OSError: @@ -123,8 +135,9 @@ def migrate_legacy_config(cls) -> Path | None: f"Warning: nsls2api config migration failed ({exc}). " f"Your settings remain at '{legacy}'. " f"To migrate manually:\n" - f" mv '{legacy}' '{legacy}.bak' && " - f"mkdir -p '{new.parent}' && mv '{legacy}.bak' '{new}'", + f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" + f" 2. Create the directory '{new.parent}'\n" + f" 3. Move the saved file into place as '{new}'", file=sys.stderr, ) else: @@ -134,7 +147,8 @@ def migrate_legacy_config(cls) -> Path | None: f"could not be rolled back. " f"Your settings are at '{tmp}'. " f"To recover:\n" - f" mkdir -p '{new.parent}' && mv '{tmp}' '{new}'", + f" 1. Create the directory '{new.parent}'\n" + f" 2. Move '{tmp}' into place as '{new}'", file=sys.stderr, ) return None diff --git a/src/nsls2api/tests/cli/test_settings.py b/src/nsls2api/tests/cli/test_settings.py index dcfff95c..7c2ddb02 100644 --- a/src/nsls2api/tests/cli/test_settings.py +++ b/src/nsls2api/tests/cli/test_settings.py @@ -155,7 +155,7 @@ def test_no_op_when_legacy_is_directory(self, tmp_path: Path): @pytest.mark.skipif(sys.platform == "win32", reason="chmod not meaningful on Windows") def test_warns_and_proceeds_when_not_writable(self, tmp_path: Path, capsys): - """Non-writable legacy file: warning is printed with two-step manual hint; + """Non-writable legacy file: warning is printed with platform-neutral hint; no exception raised; returns None.""" legacy = _make_legacy(tmp_path) legacy.chmod(0o444) # read-only @@ -172,8 +172,8 @@ def test_warns_and_proceeds_when_not_writable(self, tmp_path: Path, capsys): captured = capsys.readouterr() assert "migration skipped" in captured.err assert str(legacy) in captured.err - # Hint must be the correct two-step sequence, not the old impossible mv. - assert "mkdir -p" in captured.err + # Hint must use platform-neutral numbered steps, not POSIX shell commands. + assert "Create the directory" in captured.err assert str(new_path.parent) in captured.err def test_migration_triggered_by_read(self, tmp_path: Path): @@ -248,7 +248,7 @@ def test_shutil_move_failure_triggers_rollback(self, tmp_path: Path, capsys): result = Config.migrate_legacy_config() assert result is None - assert legacy.exists() # rolled back + assert legacy.is_file(), "Legacy config must be a file after rollback" assert legacy.read_text().startswith("[api]") captured = capsys.readouterr() assert "remain at" in captured.err @@ -320,22 +320,21 @@ def test_read_falls_back_to_legacy_when_migration_skipped( assert cfg.get("api", "token") == "mytoken" def test_read_falls_back_to_legacy_when_new_absent(self, tmp_path: Path): - """With no new config and no migration trigger, read() reads the legacy file.""" + """When migration is suppressed and no new config exists, read() falls back + to the legacy file (R4 fallback path in read()).""" _make_legacy( tmp_path, "[api]\nbase_url = https://api.example.com\n" ) - # Do not create the new-style config at all. - with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + # Patch migrate_legacy_config to a no-op so migration never runs and the + # new config file is never created — this isolates the read() fallback. + with ( + _patch_home(tmp_path), + patch.dict(os.environ, {}, clear=False), + patch.object(Config, "migrate_legacy_config", return_value=None), + ): os.environ.pop("XDG_CONFIG_HOME", None) - # Suppress migration (legacy is present but so is its path — we want - # to test the fallback directly, so skip the migration by marking the - # legacy as a dir is too invasive; instead just confirm read() returns - # legacy values when new is absent after a no-op migration). cfg = Config.read() - # Migration will have run and succeeded here (normal case), but if for any - # reason it didn't, the fallback should still work. Assert the value is present - # regardless of whether migration ran. assert cfg.get("api", "base_url") == "https://api.example.com" def test_read_returns_empty_config_when_both_absent(self, tmp_path: Path): From e8754406146f37ad0afa7ef7be26c793159edf04 Mon Sep 17 00:00:00 2001 From: Padraic Shafer Date: Fri, 4 Sep 2026 15:54:11 -0700 Subject: [PATCH 07/14] refactor(cli): copy-first migration; no rmtree; fix permission check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the staged-rename + rmtree-rollback migration with a safer copy-first approach: - shutil.copy2(legacy, tmp) preserves content before any destructive op - legacy.unlink() frees the name only when new is under legacy (no-XDG collision), since tmp already holds a full copy - On shutil.move failure the legacy name is free; restore via move(tmp, legacy) - shutil.rmtree removed entirely — destination tree is never touched - XDG case: legacy left untouched until after new file is confirmed written Fix permission pre-check: os.access(legacy.parent, W_OK|X_OK) matches what rename/unlink actually require (parent dir write+execute, not file write). A read-only file in a writable parent now migrates successfully. Update tests: copy2 staging, parent-dir chmod, new tests for read-only file migration and XDG destination-tree preservation on failure. Vestigial staged-flag / rmtree / rollback comments removed. Assisted-by: claude-opus-4-8 Assisted-by: claude-sonnet-4-6 --- src/nsls2api/cli/settings.py | 123 ++++++++++-------- src/nsls2api/tests/cli/test_settings.py | 159 +++++++++++++----------- 2 files changed, 156 insertions(+), 126 deletions(-) diff --git a/src/nsls2api/cli/settings.py b/src/nsls2api/cli/settings.py index 821965cc..4f4d73af 100644 --- a/src/nsls2api/cli/settings.py +++ b/src/nsls2api/cli/settings.py @@ -49,19 +49,20 @@ def migrate_legacy_config(cls) -> Path | None: """Migrate the legacy bare-file config to the new location, if applicable. If ``~/.config/nsls2`` exists as a **regular file** (the old format) and - the new config file does not yet exist, move the legacy file to the new - location, creating intermediate directories as needed. + the new config file does not yet exist, copy the content to the new + location and remove the legacy file only after the new file is confirmed + written. - Because the legacy file occupies the same name (``nsls2``) that must - become a *directory* for the new path, migration stages the legacy file - to a temporary sibling location first (via :func:`tempfile.mkstemp`) to - free the name before calling ``mkdir``. + In the default (no-XDG) case the legacy file occupies the path component + ``nsls2`` that must become a *directory* for the new path. The legacy + file is removed just before ``mkdir`` so the name is free; the content is + already preserved in a temporary copy made at the start of migration. Returns the new path if migration occurred, ``None`` otherwise. - Emits a warning to stderr and returns ``None`` (without raising) if the - move fails for any reason (e.g. permissions), so a migration failure - never breaks a normal CLI command. + Emits a warning to stderr and returns ``None`` (without raising) if + migration fails for any reason, so a failure never breaks a normal CLI + command. """ legacy = cls._legacy_filepath() new = cls.get_filepath() @@ -74,11 +75,12 @@ def migrate_legacy_config(cls) -> Path | None: if not legacy.is_file(): return None - # Check writability before attempting the move. - if not os.access(legacy, os.W_OK): + # Rename (the staging step) requires write+execute on the containing + # directory, not on the file itself. + if not os.access(legacy.parent, os.W_OK | os.X_OK): print( f"Warning: nsls2api config migration skipped — " - f"'{legacy}' is not writable. " + f"'{legacy.parent}' is not writable. " f"To migrate manually:\n" f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" f" 2. Create the directory '{new.parent}'\n" @@ -87,50 +89,50 @@ def migrate_legacy_config(cls) -> Path | None: ) return None - # Stage the legacy file to a temp sibling so the 'nsls2' name is freed - # before we try to create a directory with that same name. + # Step 1 — copy legacy content to a temp sibling. Legacy stays in place + # until migration succeeds; a failure here leaves everything unchanged. fd, tmp_name = tempfile.mkstemp(dir=legacy.parent, prefix=".nsls2-migrate-") os.close(fd) tmp = Path(tmp_name) - staged = False # True once legacy.replace(tmp) succeeds and tmp holds the data try: - legacy.replace(tmp) # free the 'nsls2' name - staged = True - new.parent.mkdir(parents=True, exist_ok=True) # 'nsls2' can now be a dir - shutil.move(str(tmp), str(new)) # cross-device safe final move + shutil.copy2(legacy, tmp) except OSError as exc: - if not staged: - # Staging step failed — legacy is still intact; discard empty temp file. - tmp.unlink(missing_ok=True) - print( - f"Warning: nsls2api config migration failed ({exc}). " - f"Your settings remain at '{legacy}'. " - f"To migrate manually:\n" - f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" - f" 2. Create the directory '{new.parent}'\n" - f" 3. Move the saved file into place as '{new}'", - file=sys.stderr, - ) - else: - # Staging succeeded (tmp holds the data); a later step failed. - # new.parent.mkdir() may have created ~/.config/nsls2/api/ and - # ~/.config/nsls2/ (which occupies the legacy name as a directory). - # Remove those empty dirs so the legacy name is free to receive the - # file again. rmtree is limited to new.parent (the api/ subtree); - # legacy.rmdir() only succeeds when the dir is empty — if it - # unexpectedly holds other content it raises and we fall through - # to the "settings at tmp" message, never deleting real data. - restored = False + tmp.unlink(missing_ok=True) + print( + f"Warning: nsls2api config migration failed ({exc}). " + f"Your settings remain at '{legacy}'. " + f"To migrate manually:\n" + f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" + f" 2. Create the directory '{new.parent}'\n" + f" 3. Move the saved file into place as '{new}'", + file=sys.stderr, + ) + return None + + # Step 2 — move tmp to the new location, handling the name-collision case. + # + # When XDG_CONFIG_HOME is not set, new == ~/.config/nsls2/api/cli.ini, so + # legacy (~/.config/nsls2, a file) sits exactly where the 'nsls2' directory + # component of new.parent must be created. We must remove the legacy file + # before mkdir can succeed; since tmp already holds a full copy, this is safe. + # + # When XDG_CONFIG_HOME points elsewhere, new lives in a different tree and + # there is no collision; legacy is left untouched until after the new file + # is successfully written. + new_under_legacy = legacy in new.parents + legacy_removed = False + try: + if new_under_legacy: + legacy.unlink() # free the 'nsls2' name; tmp holds the copy + legacy_removed = True + new.parent.mkdir(parents=True, exist_ok=True) + shutil.move(str(tmp), str(new)) # cross-device safe + except OSError as exc: + # Recovery: restore legacy from tmp when possible. + if legacy_removed and not legacy.exists(): + # The legacy name is free — move tmp straight back. try: - if new.parent.is_dir(): - shutil.rmtree(new.parent, ignore_errors=True) # …/nsls2/api - if legacy.is_dir(): - legacy.rmdir() # empty …/nsls2 - tmp.replace(legacy) - restored = True - except OSError: - pass - if restored: + shutil.move(str(tmp), str(legacy)) print( f"Warning: nsls2api config migration failed ({exc}). " f"Your settings remain at '{legacy}'. " @@ -140,19 +142,34 @@ def migrate_legacy_config(cls) -> Path | None: f" 3. Move the saved file into place as '{new}'", file=sys.stderr, ) - else: - # Rollback also failed — settings are at the temp path. + except OSError: print( f"Warning: nsls2api config migration failed ({exc}) and " - f"could not be rolled back. " + f"could not be recovered. " f"Your settings are at '{tmp}'. " f"To recover:\n" f" 1. Create the directory '{new.parent}'\n" f" 2. Move '{tmp}' into place as '{new}'", file=sys.stderr, ) + else: + # Legacy was not removed (XDG case, or copy step) — settings intact. + tmp.unlink(missing_ok=True) + print( + f"Warning: nsls2api config migration failed ({exc}). " + f"Your settings remain at '{legacy}'. " + f"To migrate manually:\n" + f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" + f" 2. Create the directory '{new.parent}'\n" + f" 3. Move the saved file into place as '{new}'", + file=sys.stderr, + ) return None + # Step 3 — migration succeeded. + # XDG case: legacy was not removed in step 2; delete it now. + if not new_under_legacy: + legacy.unlink(missing_ok=True) return new @classmethod diff --git a/src/nsls2api/tests/cli/test_settings.py b/src/nsls2api/tests/cli/test_settings.py index 7c2ddb02..3d254d2e 100644 --- a/src/nsls2api/tests/cli/test_settings.py +++ b/src/nsls2api/tests/cli/test_settings.py @@ -94,14 +94,15 @@ def test_get_value_missing_returns_none(self, tmp_path: Path): class TestMigrateLegacyConfig: def test_migrates_legacy_file(self, tmp_path: Path): - """Legacy bare file is moved to the new location; content is preserved. + """Legacy bare file is migrated to the new location; content is preserved. - This is the key self-collision regression test: the legacy file occupies - the path component that must become a directory, so staging via a temp - file is required. After migration: + In the default (no-XDG) case the legacy file occupies the path component + that must become a directory. Migration copies the content to a temp file + first, removes the legacy file to free the name, then moves the temp file + to the new path. After migration: - ~/.config/nsls2 must be a DIRECTORY (not a file) - ~/.config/nsls2/api/cli.ini must exist with original content - - no staging temp files remain in ~/.config/ + - no temp files remain in ~/.config/ """ legacy = _make_legacy(tmp_path) dot_config = tmp_path / ".config" @@ -112,11 +113,11 @@ def test_migrates_legacy_file(self, tmp_path: Path): assert new_path is not None assert new_path == tmp_path / ".config" / "nsls2" / "api" / "cli.ini" assert new_path.exists() - assert legacy.is_dir() # bare file replaced by a directory + assert legacy.is_dir() # legacy path is now a directory content = new_path.read_text() assert "base_url" in content assert "127.0.0.1:8080" in content - # No staging temp files left behind. + # No temp files left behind. leftover = list(dot_config.glob(".nsls2-migrate-*")) assert leftover == [], f"Unexpected temp files left over: {leftover}" @@ -154,11 +155,12 @@ def test_no_op_when_legacy_is_directory(self, tmp_path: Path): assert result is None @pytest.mark.skipif(sys.platform == "win32", reason="chmod not meaningful on Windows") - def test_warns_and_proceeds_when_not_writable(self, tmp_path: Path, capsys): - """Non-writable legacy file: warning is printed with platform-neutral hint; - no exception raised; returns None.""" + def test_warns_and_skips_when_parent_dir_not_writable(self, tmp_path: Path, capsys): + """Non-writable parent directory: warning is printed with platform-neutral + hint; no exception raised; returns None; legacy file untouched.""" legacy = _make_legacy(tmp_path) - legacy.chmod(0o444) # read-only + dot_config = tmp_path / ".config" + dot_config.chmod(0o555) # remove write+execute from the containing dir try: with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): @@ -166,16 +168,38 @@ def test_warns_and_proceeds_when_not_writable(self, tmp_path: Path, capsys): new_path = Config.get_filepath() result = Config.migrate_legacy_config() finally: - legacy.chmod(0o644) # restore so tmp_path cleanup works + dot_config.chmod(0o755) # restore so tmp_path cleanup works assert result is None captured = capsys.readouterr() assert "migration skipped" in captured.err - assert str(legacy) in captured.err + # Warning identifies the non-writable directory, not the file. + assert str(dot_config) in captured.err # Hint must use platform-neutral numbered steps, not POSIX shell commands. assert "Create the directory" in captured.err assert str(new_path.parent) in captured.err + @pytest.mark.skipif(sys.platform == "win32", reason="chmod not meaningful on Windows") + def test_readonly_file_in_writable_dir_migrates(self, tmp_path: Path): + """A read-only legacy file in a writable parent directory migrates + successfully. The permission check gates on the parent dir (needed for + rename/unlink), not on the file itself.""" + legacy = _make_legacy(tmp_path) + legacy.chmod(0o444) # file read-only, but parent dir is writable + + try: + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + new_path = Config.migrate_legacy_config() + finally: + # If migration succeeded the file is gone; restore only if still there. + if legacy.is_file(): + legacy.chmod(0o644) + + assert new_path is not None + assert new_path.exists() + assert "base_url" in new_path.read_text() + def test_migration_triggered_by_read(self, tmp_path: Path): """Calling read() auto-migrates the legacy file and returns its contents.""" _make_legacy(tmp_path, "[api]\nbase_url = http://127.0.0.1:8080\ntoken = abc\n") @@ -187,57 +211,49 @@ def test_migration_triggered_by_read(self, tmp_path: Path): assert cfg.get("api", "base_url") == "http://127.0.0.1:8080" assert cfg.get("api", "token") == "abc" - assert legacy.is_dir() # bare file replaced by a directory + assert legacy.is_dir() # legacy path is now a directory new = tmp_path / ".config" / "nsls2" / "api" / "cli.ini" assert new.exists() - @pytest.mark.skipif(sys.platform == "win32", reason="chmod not meaningful on Windows") - def test_staging_failure_cleans_up_temp_and_reports_legacy( + def test_copy_failure_cleans_up_temp_and_reports_legacy( self, tmp_path: Path, capsys ): - """If the initial legacy→tmp staging step raises, migrate_legacy_config(): + """If the initial copy step raises, migrate_legacy_config(): - returns None - legacy file is still intact - no .nsls2-migrate-* temp files are left behind - - warning says settings remain at legacy (not at a temp path) + - warning says settings remain at legacy """ + import nsls2api.cli.settings as settings_mod + legacy = _make_legacy(tmp_path) dot_config = tmp_path / ".config" - original_replace = Path.replace - - call_count = 0 - - def replace_that_fails_on_first_call(self, target): - nonlocal call_count - call_count += 1 - if call_count == 1: - raise OSError("simulated staging failure") - return original_replace(self, target) - with ( _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False), - patch.object(Path, "replace", replace_that_fails_on_first_call), + patch.object(settings_mod.shutil, "copy2", side_effect=OSError("disk full")), ): os.environ.pop("XDG_CONFIG_HOME", None) result = Config.migrate_legacy_config() assert result is None - assert legacy.exists() # legacy untouched + assert legacy.is_file() # legacy untouched assert legacy.read_text().startswith("[api]") leftover = list(dot_config.glob(".nsls2-migrate-*")) assert leftover == [], f"Temp files leaked: {leftover}" captured = capsys.readouterr() assert "remain at" in captured.err - assert "settings are at" not in captured.err # must NOT claim settings at temp + assert "settings are at" not in captured.err - def test_shutil_move_failure_triggers_rollback(self, tmp_path: Path, capsys): - """If shutil.move raises (e.g. cross-device), the legacy file is restored - and the warning says settings remain at legacy.""" + def test_shutil_move_failure_restores_legacy(self, tmp_path: Path, capsys): + """If shutil.move raises after legacy has been unlinked (no-XDG collision + case), the legacy file is restored from the temp copy and the warning says + settings remain at legacy — no data loss.""" import nsls2api.cli.settings as settings_mod - legacy = _make_legacy(tmp_path) + content = "[api]\nbase_url = http://127.0.0.1:8080\ntoken = secret\n" + legacy = _make_legacy(tmp_path, content) with ( _patch_home(tmp_path), @@ -248,47 +264,46 @@ def test_shutil_move_failure_triggers_rollback(self, tmp_path: Path, capsys): result = Config.migrate_legacy_config() assert result is None - assert legacy.is_file(), "Legacy config must be a file after rollback" - assert legacy.read_text().startswith("[api]") + assert legacy.is_file(), "Legacy config must be restored as a file" + assert "base_url" in legacy.read_text(), "Legacy config content was lost!" + assert "secret" in legacy.read_text(), "Token was lost!" captured = capsys.readouterr() assert "remain at" in captured.err + assert "settings are at" not in captured.err - def test_shutil_move_failure_after_mkdir_does_not_lose_data( + def test_xdg_destination_tree_not_deleted_on_move_failure( self, tmp_path: Path, capsys ): - """Regression: when shutil.move fails AFTER mkdir has created the nsls2/ - directory, the old legacy.exists() check would see the newly-created dir - and delete the temp file (the only copy of user settings) — data loss. - - With the staged-flag fix, the code correctly identifies that staging - succeeded (tmp holds the data) and attempts rollback regardless of whether - legacy.exists() is True or False. - """ + """Regression: when XDG_CONFIG_HOME is set, new.parent may be a + pre-existing directory unrelated to the legacy path. A shutil.move + failure must NOT delete that directory. No rmtree of the destination + tree is performed.""" import nsls2api.cli.settings as settings_mod - content = "[api]\nbase_url = http://127.0.0.1:8080\ntoken = secret\n" - legacy = _make_legacy(tmp_path, content) + xdg = tmp_path / "xdg" + xdg.mkdir() + # Pre-create destination tree with a sentinel file. + existing_api_dir = xdg / "nsls2" / "api" + existing_api_dir.mkdir(parents=True) + sentinel = existing_api_dir / "sentinel.txt" + sentinel.write_text("do not delete me") + + legacy = _make_legacy(tmp_path) - # mkdir runs normally so the nsls2/ directory gets created first, - # then shutil.move raises — the old code saw legacy.exists()==True (dir!) - # and deleted the temp file (data loss). The staged-flag fix avoids this. with ( _patch_home(tmp_path), - patch.dict(os.environ, {}, clear=False), - patch.object(settings_mod.shutil, "move", side_effect=OSError("EXDEV after mkdir")), + patch.dict(os.environ, {"XDG_CONFIG_HOME": str(xdg)}, clear=False), + patch.object(settings_mod.shutil, "move", side_effect=OSError("EXDEV")), ): - os.environ.pop("XDG_CONFIG_HOME", None) result = Config.migrate_legacy_config() assert result is None - # The legacy file must be restored (rollback succeeded) — no data loss. - assert legacy.exists(), "Legacy config was lost after shutil.move failure!" - assert legacy.is_file(), "Legacy config should be a file after rollback" - assert "base_url" in legacy.read_text(), "Legacy config content was lost!" - assert "secret" in legacy.read_text(), "Token was lost!" + assert sentinel.exists(), "Destination tree was deleted on move failure!" + assert sentinel.read_text() == "do not delete me" + # Legacy file must still be present (XDG case: legacy not unlinked before move). + assert legacy.is_file() captured = capsys.readouterr() assert "remain at" in captured.err - assert "settings are at" not in captured.err # --------------------------------------------------------------------------- @@ -298,23 +313,21 @@ def test_shutil_move_failure_after_mkdir_does_not_lose_data( class TestReadLegacyFallback: """When migration is skipped/failed, read() must still return legacy values.""" - @pytest.mark.skipif(sys.platform == "win32", reason="chmod not meaningful on Windows") def test_read_falls_back_to_legacy_when_migration_skipped( - self, tmp_path: Path, capsys + self, tmp_path: Path ): - """Non-writable legacy → migration skipped; read() should still return + """When migration is skipped (returns None), read() should still return the legacy base_url rather than an empty config.""" - legacy = _make_legacy( + _make_legacy( tmp_path, "[api]\nbase_url = http://127.0.0.1:8080\ntoken = mytoken\n" ) - legacy.chmod(0o444) - - try: - with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): - os.environ.pop("XDG_CONFIG_HOME", None) - cfg = Config.read() - finally: - legacy.chmod(0o644) + with ( + _patch_home(tmp_path), + patch.dict(os.environ, {}, clear=False), + patch.object(Config, "migrate_legacy_config", return_value=None), + ): + os.environ.pop("XDG_CONFIG_HOME", None) + cfg = Config.read() assert cfg.get("api", "base_url") == "http://127.0.0.1:8080" assert cfg.get("api", "token") == "mytoken" From 389066278b747b9539000288fc1568147f4e8f59 Mon Sep 17 00:00:00 2001 From: Padraic Shafer Date: Fri, 4 Sep 2026 16:00:11 -0700 Subject: [PATCH 08/14] fix(cli): treat XDG_CONFIG_HOME=~/.config as the name-collision case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new_under_legacy check was lexical (legacy in new.parents), so XDG_CONFIG_HOME values that resolve to ~/.config via a different spelling (trailing slash, '..' segment, or a symlink) were incorrectly treated as the no-collision XDG branch, causing migration to fail silently when mkdir encountered the legacy file blocking the 'nsls2' directory name. Fix: compare resolved paths so any XDG spelling that resolves to ~/.config takes the collision branch (unlink legacy before mkdir, restore from tmp copy on failure) — identical mechanism and effect to the no-XDG default. Add parametrized regression tests covering exact, trailing-slash, and '..' forms of XDG=~/.config, plus a move-failure/restore test. Assisted-by: claude-opus-4-8 Assisted-by: claude-sonnet-4-6 --- src/nsls2api/cli/settings.py | 24 ++++++--- src/nsls2api/tests/cli/test_settings.py | 65 +++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 8 deletions(-) diff --git a/src/nsls2api/cli/settings.py b/src/nsls2api/cli/settings.py index 4f4d73af..b19d8308 100644 --- a/src/nsls2api/cli/settings.py +++ b/src/nsls2api/cli/settings.py @@ -111,15 +111,23 @@ def migrate_legacy_config(cls) -> Path | None: # Step 2 — move tmp to the new location, handling the name-collision case. # - # When XDG_CONFIG_HOME is not set, new == ~/.config/nsls2/api/cli.ini, so - # legacy (~/.config/nsls2, a file) sits exactly where the 'nsls2' directory - # component of new.parent must be created. We must remove the legacy file - # before mkdir can succeed; since tmp already holds a full copy, this is safe. + # When new is located under the legacy path (e.g. XDG_CONFIG_HOME unset, or + # set to ~/.config), legacy (~/.config/nsls2, a file) sits exactly where the + # 'nsls2' directory component of new.parent must be created. We must remove + # the legacy file before mkdir can succeed; since tmp already holds a full + # copy, this is safe. # - # When XDG_CONFIG_HOME points elsewhere, new lives in a different tree and - # there is no collision; legacy is left untouched until after the new file - # is successfully written. - new_under_legacy = legacy in new.parents + # When XDG_CONFIG_HOME points to a genuinely different directory, new lives + # in a separate tree and there is no collision; legacy is left untouched + # until after the new file is successfully written. + # + # Paths are compared after resolution so that XDG_CONFIG_HOME values that + # refer to ~/.config via an alternative spelling (trailing slash, '..' segment, + # or a symlink) are still recognised as the collision case. + legacy_resolved = legacy.resolve() + new_under_legacy = any( + legacy_resolved == parent.resolve() for parent in new.parents + ) legacy_removed = False try: if new_under_legacy: diff --git a/src/nsls2api/tests/cli/test_settings.py b/src/nsls2api/tests/cli/test_settings.py index 3d254d2e..d3d7a194 100644 --- a/src/nsls2api/tests/cli/test_settings.py +++ b/src/nsls2api/tests/cli/test_settings.py @@ -305,6 +305,71 @@ def test_xdg_destination_tree_not_deleted_on_move_failure( captured = capsys.readouterr() assert "remain at" in captured.err + @pytest.mark.parametrize("xdg_suffix", [ + "", # exact: XDG_CONFIG_HOME=/tmp/.../home/.config + "/", # trailing slash + "/../.config" # unnormalized '..' segment resolving to the same dir + ]) + def test_migrates_when_xdg_equals_config_dir( + self, tmp_path: Path, xdg_suffix: str + ): + """When XDG_CONFIG_HOME resolves to ~/.config (via exact match, trailing + slash, or '..' segment), migration takes the collision branch and produces + the same result as when XDG_CONFIG_HOME is unset: + - new config written to ~/.config/nsls2/api/cli.ini + - legacy path becomes a directory + - no temp files left behind + """ + dot_config = tmp_path / ".config" + dot_config.mkdir(parents=True, exist_ok=True) + legacy = _make_legacy(tmp_path) + xdg_value = str(dot_config) + xdg_suffix + + with ( + _patch_home(tmp_path), + patch.dict(os.environ, {"XDG_CONFIG_HOME": xdg_value}, clear=False), + ): + new_path = Config.migrate_legacy_config() + + assert new_path is not None + assert new_path == tmp_path / ".config" / "nsls2" / "api" / "cli.ini" + assert new_path.exists() + assert "base_url" in new_path.read_text() + assert legacy.is_dir(), "Legacy path must become a directory after migration" + leftover = list(dot_config.glob(".nsls2-migrate-*")) + assert leftover == [], f"Unexpected temp files left over: {leftover}" + + def test_xdg_equals_config_dir_move_failure_restores_legacy( + self, tmp_path: Path, capsys + ): + """When XDG_CONFIG_HOME=~/.config and shutil.move fails, the legacy file + is restored (collision branch recovery), not silently discarded.""" + import nsls2api.cli.settings as settings_mod + + dot_config = tmp_path / ".config" + dot_config.mkdir(parents=True, exist_ok=True) + content = "[api]\nbase_url = http://127.0.0.1:8080\ntoken = secret\n" + legacy = _make_legacy(tmp_path, content) + + with ( + _patch_home(tmp_path), + patch.dict( + os.environ, + {"XDG_CONFIG_HOME": str(dot_config)}, + clear=False, + ), + patch.object(settings_mod.shutil, "move", side_effect=OSError("EXDEV")), + ): + result = Config.migrate_legacy_config() + + assert result is None + assert legacy.is_file(), "Legacy config must be restored as a file" + assert "base_url" in legacy.read_text() + assert "secret" in legacy.read_text() + captured = capsys.readouterr() + assert "remain at" in captured.err + assert "settings are at" not in captured.err + # --------------------------------------------------------------------------- # R4 — read() legacy fallback From e26f9652976ab8635bb78aa70d5db5b74cf3fd57 Mon Sep 17 00:00:00 2001 From: Padraic Shafer Date: Fri, 4 Sep 2026 16:12:01 -0700 Subject: [PATCH 09/14] fix(cli): repair legacy restore after move failure; fix two tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recovery branch in migrate_legacy_config() reused the fragile `not legacy.exists()` check to decide whether to restore the legacy file. When shutil.move failed _after_ new.parent.mkdir() had run, mkdir had already created a directory at the legacy path (collision case), so `legacy.exists()` returned True and the restore was skipped — discarding the tmp copy and misreporting the config location. Same class of bug as round 3, resurfaced in the error path. Fix: before mkdir, record exactly which ancestor directories don't yet exist (innermost-first). On OSError, rmdir only those empty directories (never recursive), then restore based on `legacy_removed` rather than `legacy.exists()`. Also fix two test regressions on commit 3890662: - test_no_op_when_new_already_exists: removed the contradictory _make_legacy() call; the new.exists() short-circuit fires before any legacy handling so the test only needs `new` in place. - test_migrates_when_xdg_equals_config_dir[/../.config]: compare new_path.resolve() to the expected resolved path — XDG values with '..' segments produce an unresolved return value. Assisted-by: claude-opus-4-8 Assisted-by: claude-sonnet-4-6 --- src/nsls2api/cli/settings.py | 61 +++++++++++++++++++------ src/nsls2api/tests/cli/test_settings.py | 15 ++++-- 2 files changed, 57 insertions(+), 19 deletions(-) diff --git a/src/nsls2api/cli/settings.py b/src/nsls2api/cli/settings.py index b19d8308..60e267e8 100644 --- a/src/nsls2api/cli/settings.py +++ b/src/nsls2api/cli/settings.py @@ -129,28 +129,61 @@ def migrate_legacy_config(cls) -> Path | None: legacy_resolved == parent.resolve() for parent in new.parents ) legacy_removed = False + # Record which ancestor directories of new.parent don't yet exist so that, + # on failure, we can undo exactly what mkdir created — and no more. This + # list is computed after any legacy.unlink() so the freed 'nsls2' name is + # included when it is part of the path that must be created. + created_dirs: list[Path] = [] try: if new_under_legacy: legacy.unlink() # free the 'nsls2' name; tmp holds the copy legacy_removed = True + # Collect innermost-first ancestors that mkdir will create. + p = new.parent + while not p.exists(): + created_dirs.append(p) + p = p.parent new.parent.mkdir(parents=True, exist_ok=True) shutil.move(str(tmp), str(new)) # cross-device safe except OSError as exc: # Recovery: restore legacy from tmp when possible. - if legacy_removed and not legacy.exists(): - # The legacy name is free — move tmp straight back. - try: - shutil.move(str(tmp), str(legacy)) - print( - f"Warning: nsls2api config migration failed ({exc}). " - f"Your settings remain at '{legacy}'. " - f"To migrate manually:\n" - f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" - f" 2. Create the directory '{new.parent}'\n" - f" 3. Move the saved file into place as '{new}'", - file=sys.stderr, - ) - except OSError: + if legacy_removed: + # The legacy name was freed; mkdir may have created directory + # entries in its place. Remove only the empty directories we + # created (innermost-first, rmdir only — never recursive) so + # that the legacy name is available again for restore. + for d in created_dirs: + try: + if d.is_dir(): + d.rmdir() + except OSError: + break + if not legacy.exists(): + # The legacy name is free — move tmp straight back. + try: + shutil.move(str(tmp), str(legacy)) + print( + f"Warning: nsls2api config migration failed ({exc}). " + f"Your settings remain at '{legacy}'. " + f"To migrate manually:\n" + f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" + f" 2. Create the directory '{new.parent}'\n" + f" 3. Move the saved file into place as '{new}'", + file=sys.stderr, + ) + except OSError: + print( + f"Warning: nsls2api config migration failed ({exc}) and " + f"could not be recovered. " + f"Your settings are at '{tmp}'. " + f"To recover:\n" + f" 1. Create the directory '{new.parent}'\n" + f" 2. Move '{tmp}' into place as '{new}'", + file=sys.stderr, + ) + else: + # Created dirs couldn't all be removed; legacy name still + # occupied. Settings safe in tmp. print( f"Warning: nsls2api config migration failed ({exc}) and " f"could not be recovered. " diff --git a/src/nsls2api/tests/cli/test_settings.py b/src/nsls2api/tests/cli/test_settings.py index d3d7a194..79d4d186 100644 --- a/src/nsls2api/tests/cli/test_settings.py +++ b/src/nsls2api/tests/cli/test_settings.py @@ -122,8 +122,11 @@ def test_migrates_legacy_file(self, tmp_path: Path): assert leftover == [], f"Unexpected temp files left over: {leftover}" def test_no_op_when_new_already_exists(self, tmp_path: Path): - """Migration is skipped when the new config file is already present.""" - legacy = _make_legacy(tmp_path) + """Migration is skipped when the new config file is already present. + + The new.exists() short-circuit fires before any legacy handling, so + this test only needs the new file to be in place — no legacy required. + """ new = tmp_path / ".config" / "nsls2" / "api" / "cli.ini" new.parent.mkdir(parents=True, exist_ok=True) new.write_text("[api]\nbase_url = https://already.here\n") @@ -133,7 +136,6 @@ def test_no_op_when_new_already_exists(self, tmp_path: Path): result = Config.migrate_legacy_config() assert result is None - assert legacy.exists() # legacy untouched assert new.read_text().startswith("[api]\nbase_url = https://already.here") def test_no_op_when_legacy_absent(self, tmp_path: Path): @@ -160,7 +162,7 @@ def test_warns_and_skips_when_parent_dir_not_writable(self, tmp_path: Path, caps hint; no exception raised; returns None; legacy file untouched.""" legacy = _make_legacy(tmp_path) dot_config = tmp_path / ".config" - dot_config.chmod(0o555) # remove write+execute from the containing dir + dot_config.chmod(0o555) # remove write permission from the containing dir try: with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): @@ -332,7 +334,10 @@ def test_migrates_when_xdg_equals_config_dir( new_path = Config.migrate_legacy_config() assert new_path is not None - assert new_path == tmp_path / ".config" / "nsls2" / "api" / "cli.ini" + # Compare resolved paths: XDG values with trailing slash or '..' segments + # produce an unresolved new_path; normalise both sides before comparing. + expected = (tmp_path / ".config" / "nsls2" / "api" / "cli.ini").resolve() + assert new_path.resolve() == expected assert new_path.exists() assert "base_url" in new_path.read_text() assert legacy.is_dir(), "Legacy path must become a directory after migration" From f415e25a738fc9eae1fde5e0b85bf7c45d8e31ec Mon Sep 17 00:00:00 2001 From: Padraic Shafer Date: Fri, 4 Sep 2026 16:20:32 -0700 Subject: [PATCH 10/14] test(cli): fix restore-test mocks to raise on first move only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both test_shutil_move_failure_restores_legacy and test_xdg_equals_config_dir_move_failure_restores_legacy patched shutil.move with a blanket side_effect=OSError("EXDEV"), which caused every shutil.move call — including the recovery restore — to raise. This made it impossible for the restore branch to succeed, causing legacy.is_file() to be False even after the product-side fix in e26f965. Replace the blanket mock with a closure (_move_fails_once) that raises only on the first call and delegates to the real shutil.move on subsequent calls, accurately simulating a cross-device rename failure on the migration move while allowing the restore move to complete. Assisted-by: claude-opus-4-8 Assisted-by: claude-sonnet-4-6 --- src/nsls2api/tests/cli/test_settings.py | 26 +++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/src/nsls2api/tests/cli/test_settings.py b/src/nsls2api/tests/cli/test_settings.py index 79d4d186..52a56c1e 100644 --- a/src/nsls2api/tests/cli/test_settings.py +++ b/src/nsls2api/tests/cli/test_settings.py @@ -257,10 +257,21 @@ def test_shutil_move_failure_restores_legacy(self, tmp_path: Path, capsys): content = "[api]\nbase_url = http://127.0.0.1:8080\ntoken = secret\n" legacy = _make_legacy(tmp_path, content) + # Simulate a cross-device rename failure: the migration move raises, but + # the restore move (second call) must succeed so recovery can complete. + _real_move = settings_mod.shutil.move + _calls = [0] + + def _move_fails_once(src, dst, *args, **kwargs): + _calls[0] += 1 + if _calls[0] == 1: + raise OSError("EXDEV") + return _real_move(src, dst, *args, **kwargs) + with ( _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False), - patch.object(settings_mod.shutil, "move", side_effect=OSError("EXDEV")), + patch.object(settings_mod.shutil, "move", side_effect=_move_fails_once), ): os.environ.pop("XDG_CONFIG_HOME", None) result = Config.migrate_legacy_config() @@ -356,6 +367,17 @@ def test_xdg_equals_config_dir_move_failure_restores_legacy( content = "[api]\nbase_url = http://127.0.0.1:8080\ntoken = secret\n" legacy = _make_legacy(tmp_path, content) + # Simulate a cross-device rename failure: the migration move raises, but + # the restore move (second call) must succeed so recovery can complete. + _real_move = settings_mod.shutil.move + _calls = [0] + + def _move_fails_once(src, dst, *args, **kwargs): + _calls[0] += 1 + if _calls[0] == 1: + raise OSError("EXDEV") + return _real_move(src, dst, *args, **kwargs) + with ( _patch_home(tmp_path), patch.dict( @@ -363,7 +385,7 @@ def test_xdg_equals_config_dir_move_failure_restores_legacy( {"XDG_CONFIG_HOME": str(dot_config)}, clear=False, ), - patch.object(settings_mod.shutil, "move", side_effect=OSError("EXDEV")), + patch.object(settings_mod.shutil, "move", side_effect=_move_fails_once), ): result = Config.migrate_legacy_config() From 21b8c29839bb22180acdf7cd68dff59a4983975b Mon Sep 17 00:00:00 2001 From: Padraic Shafer Date: Fri, 4 Sep 2026 16:40:22 -0700 Subject: [PATCH 11/14] refactor(cli): extract legacy migration into settings_migration.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit settings.py shrinks from 303 → 127 lines. All migration and legacy fallback logic moves to a new isolated module so that future removal of legacy support is a single-file deletion: New: src/nsls2api/cli/settings_migration.py - legacy_filepath() — canonical legacy path - migrate_legacy_config(config_filepath) — one-time migration - read_legacy_fallback(config, config_filepath) — back-compat read Changed: src/nsls2api/cli/settings.py - Remove _legacy_filepath() and migrate_legacy_config() from Config - Remove shutil/tempfile imports (no longer needed here) - Import settings_migration; rewrite read() to call the three helpers explicitly with config_filepath = cls.get_filepath() New: src/nsls2api/tests/cli/test_settings_migration.py - TestMigrateLegacyConfig and TestReadLegacyFallback moved here - Patch targets retargeted to nsls2api.cli.settings_migration.* - Local copies of _make_legacy/_patch_home (intentionally self-contained for symmetrical deletion later) Changed: src/nsls2api/tests/cli/test_settings.py - Remove the two moved test classes - Remove unused _make_legacy helper - Update module docstring No behaviour change; all existing tests preserved. Assisted-by: claude-opus-4-8 Assisted-by: claude-sonnet-4-6 --- src/nsls2api/cli/settings.py | 190 +------- src/nsls2api/cli/settings_migration.py | 220 ++++++++++ src/nsls2api/tests/cli/test_settings.py | 377 +--------------- .../tests/cli/test_settings_migration.py | 404 ++++++++++++++++++ 4 files changed, 636 insertions(+), 555 deletions(-) create mode 100644 src/nsls2api/cli/settings_migration.py create mode 100644 src/nsls2api/tests/cli/test_settings_migration.py diff --git a/src/nsls2api/cli/settings.py b/src/nsls2api/cli/settings.py index 60e267e8..570c7f24 100644 --- a/src/nsls2api/cli/settings.py +++ b/src/nsls2api/cli/settings.py @@ -1,12 +1,12 @@ import configparser import os -import shutil import sys -import tempfile from enum import Enum from pathlib import Path from typing import Any +from nsls2api.cli import settings_migration + class ApiEnvironment(str, Enum): PRODUCTION = "https://api.nsls2.bnl.gov" @@ -39,180 +39,6 @@ def get_filepath() -> Path: base = Path(xdg) if xdg else Path.home() / ".config" return base / "nsls2" / "api" / "cli.ini" - @staticmethod - def _legacy_filepath() -> Path: - """Returns the old (pre-migration) config path: ~/.config/nsls2 (a bare file).""" - return Path.home() / ".config" / "nsls2" - - @classmethod - def migrate_legacy_config(cls) -> Path | None: - """Migrate the legacy bare-file config to the new location, if applicable. - - If ``~/.config/nsls2`` exists as a **regular file** (the old format) and - the new config file does not yet exist, copy the content to the new - location and remove the legacy file only after the new file is confirmed - written. - - In the default (no-XDG) case the legacy file occupies the path component - ``nsls2`` that must become a *directory* for the new path. The legacy - file is removed just before ``mkdir`` so the name is free; the content is - already preserved in a temporary copy made at the start of migration. - - Returns the new path if migration occurred, ``None`` otherwise. - - Emits a warning to stderr and returns ``None`` (without raising) if - migration fails for any reason, so a failure never breaks a normal CLI - command. - """ - legacy = cls._legacy_filepath() - new = cls.get_filepath() - - # Already migrated (or user already has a new-style config) — nothing to do. - if new.exists(): - return None - - # Only migrate a plain FILE. If legacy is a directory, skip silently. - if not legacy.is_file(): - return None - - # Rename (the staging step) requires write+execute on the containing - # directory, not on the file itself. - if not os.access(legacy.parent, os.W_OK | os.X_OK): - print( - f"Warning: nsls2api config migration skipped — " - f"'{legacy.parent}' is not writable. " - f"To migrate manually:\n" - f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" - f" 2. Create the directory '{new.parent}'\n" - f" 3. Move the saved file into place as '{new}'", - file=sys.stderr, - ) - return None - - # Step 1 — copy legacy content to a temp sibling. Legacy stays in place - # until migration succeeds; a failure here leaves everything unchanged. - fd, tmp_name = tempfile.mkstemp(dir=legacy.parent, prefix=".nsls2-migrate-") - os.close(fd) - tmp = Path(tmp_name) - try: - shutil.copy2(legacy, tmp) - except OSError as exc: - tmp.unlink(missing_ok=True) - print( - f"Warning: nsls2api config migration failed ({exc}). " - f"Your settings remain at '{legacy}'. " - f"To migrate manually:\n" - f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" - f" 2. Create the directory '{new.parent}'\n" - f" 3. Move the saved file into place as '{new}'", - file=sys.stderr, - ) - return None - - # Step 2 — move tmp to the new location, handling the name-collision case. - # - # When new is located under the legacy path (e.g. XDG_CONFIG_HOME unset, or - # set to ~/.config), legacy (~/.config/nsls2, a file) sits exactly where the - # 'nsls2' directory component of new.parent must be created. We must remove - # the legacy file before mkdir can succeed; since tmp already holds a full - # copy, this is safe. - # - # When XDG_CONFIG_HOME points to a genuinely different directory, new lives - # in a separate tree and there is no collision; legacy is left untouched - # until after the new file is successfully written. - # - # Paths are compared after resolution so that XDG_CONFIG_HOME values that - # refer to ~/.config via an alternative spelling (trailing slash, '..' segment, - # or a symlink) are still recognised as the collision case. - legacy_resolved = legacy.resolve() - new_under_legacy = any( - legacy_resolved == parent.resolve() for parent in new.parents - ) - legacy_removed = False - # Record which ancestor directories of new.parent don't yet exist so that, - # on failure, we can undo exactly what mkdir created — and no more. This - # list is computed after any legacy.unlink() so the freed 'nsls2' name is - # included when it is part of the path that must be created. - created_dirs: list[Path] = [] - try: - if new_under_legacy: - legacy.unlink() # free the 'nsls2' name; tmp holds the copy - legacy_removed = True - # Collect innermost-first ancestors that mkdir will create. - p = new.parent - while not p.exists(): - created_dirs.append(p) - p = p.parent - new.parent.mkdir(parents=True, exist_ok=True) - shutil.move(str(tmp), str(new)) # cross-device safe - except OSError as exc: - # Recovery: restore legacy from tmp when possible. - if legacy_removed: - # The legacy name was freed; mkdir may have created directory - # entries in its place. Remove only the empty directories we - # created (innermost-first, rmdir only — never recursive) so - # that the legacy name is available again for restore. - for d in created_dirs: - try: - if d.is_dir(): - d.rmdir() - except OSError: - break - if not legacy.exists(): - # The legacy name is free — move tmp straight back. - try: - shutil.move(str(tmp), str(legacy)) - print( - f"Warning: nsls2api config migration failed ({exc}). " - f"Your settings remain at '{legacy}'. " - f"To migrate manually:\n" - f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" - f" 2. Create the directory '{new.parent}'\n" - f" 3. Move the saved file into place as '{new}'", - file=sys.stderr, - ) - except OSError: - print( - f"Warning: nsls2api config migration failed ({exc}) and " - f"could not be recovered. " - f"Your settings are at '{tmp}'. " - f"To recover:\n" - f" 1. Create the directory '{new.parent}'\n" - f" 2. Move '{tmp}' into place as '{new}'", - file=sys.stderr, - ) - else: - # Created dirs couldn't all be removed; legacy name still - # occupied. Settings safe in tmp. - print( - f"Warning: nsls2api config migration failed ({exc}) and " - f"could not be recovered. " - f"Your settings are at '{tmp}'. " - f"To recover:\n" - f" 1. Create the directory '{new.parent}'\n" - f" 2. Move '{tmp}' into place as '{new}'", - file=sys.stderr, - ) - else: - # Legacy was not removed (XDG case, or copy step) — settings intact. - tmp.unlink(missing_ok=True) - print( - f"Warning: nsls2api config migration failed ({exc}). " - f"Your settings remain at '{legacy}'. " - f"To migrate manually:\n" - f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" - f" 2. Create the directory '{new.parent}'\n" - f" 3. Move the saved file into place as '{new}'", - file=sys.stderr, - ) - return None - - # Step 3 — migration succeeded. - # XDG case: legacy was not removed in step 2; delete it now. - if not new_under_legacy: - legacy.unlink(missing_ok=True) - return new - @classmethod def read(cls) -> configparser.ConfigParser: """Read the configuration file, migrating legacy config if present. @@ -221,15 +47,13 @@ def read(cls) -> configparser.ConfigParser: exist, fall back to reading the legacy bare file so existing settings (base_url, token) are not silently dropped. """ - cls.migrate_legacy_config() + config_filepath = cls.get_filepath() + settings_migration.migrate_legacy_config(config_filepath) config = configparser.ConfigParser() - new = cls.get_filepath() - if new.exists(): - config.read(new) + if config_filepath.exists(): + config.read(config_filepath) else: - legacy = cls._legacy_filepath() - if legacy.is_file(): - config.read(legacy) # back-compat: unmigrated settings still honored + settings_migration.read_legacy_fallback(config, config_filepath) return config @classmethod diff --git a/src/nsls2api/cli/settings_migration.py b/src/nsls2api/cli/settings_migration.py new file mode 100644 index 00000000..f7da974b --- /dev/null +++ b/src/nsls2api/cli/settings_migration.py @@ -0,0 +1,220 @@ +"""Legacy config migration for nsls2api CLI. + +This module handles the one-time migration of the old bare-file config +(``~/.config/nsls2``) to the current XDG-compliant location. It is +intentionally isolated so that it can be removed entirely — along with +``test_settings_migration.py`` — once legacy configs are no longer in +the wild. + +Callers (``settings.Config.read``) interact only via the three public +functions: + +- :func:`legacy_filepath` — canonical legacy path. +- :func:`migrate_legacy_config` — perform the one-time migration. +- :func:`read_legacy_fallback` — back-compat read when migration is not + possible. +""" + +import configparser +import os +import shutil +import sys +import tempfile +from pathlib import Path + + +def legacy_filepath() -> Path: + """Return the old (pre-migration) config path: ``~/.config/nsls2``. + + The legacy config was written as a **bare file** (not a directory), + which prevents other nsls2 tools from using ``~/.config/nsls2/`` as a + directory. This path is always ``~/.config/nsls2`` regardless of + ``XDG_CONFIG_HOME``, because the legacy writer hard-coded it. + """ + return Path.home() / ".config" / "nsls2" + + +def migrate_legacy_config(config_filepath: Path) -> Path | None: + """Migrate the legacy bare-file config to *config_filepath*, if needed. + + If ``~/.config/nsls2`` exists as a **regular file** (the old format) + and *config_filepath* does not yet exist, copies the content to the + new location and removes the legacy file only after the new file is + confirmed written. + + In the default (no-XDG) case the legacy file occupies the path + component ``nsls2`` that must become a *directory* for the new path. + The legacy file is removed just before ``mkdir`` so the name is free; + content is already preserved in a temporary copy made at the start of + migration. + + Returns the new path if migration occurred, ``None`` otherwise. + + Emits a warning to stderr and returns ``None`` (without raising) if + migration fails for any reason, so a failure never breaks a normal CLI + command. + """ + legacy = legacy_filepath() + new = config_filepath + + # Already migrated (or user already has a new-style config) — nothing to do. + if new.exists(): + return None + + # Only migrate a plain FILE. If legacy is a directory, skip silently. + if not legacy.is_file(): + return None + + # Rename (the staging step) requires write+execute on the containing + # directory, not on the file itself. + if not os.access(legacy.parent, os.W_OK | os.X_OK): + print( + f"Warning: nsls2api config migration skipped — " + f"'{legacy.parent}' is not writable. " + f"To migrate manually:\n" + f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" + f" 2. Create the directory '{new.parent}'\n" + f" 3. Move the saved file into place as '{new}'", + file=sys.stderr, + ) + return None + + # Step 1 — copy legacy content to a temp sibling. Legacy stays in place + # until migration succeeds; a failure here leaves everything unchanged. + fd, tmp_name = tempfile.mkstemp(dir=legacy.parent, prefix=".nsls2-migrate-") + os.close(fd) + tmp = Path(tmp_name) + try: + shutil.copy2(legacy, tmp) + except OSError as exc: + tmp.unlink(missing_ok=True) + print( + f"Warning: nsls2api config migration failed ({exc}). " + f"Your settings remain at '{legacy}'. " + f"To migrate manually:\n" + f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" + f" 2. Create the directory '{new.parent}'\n" + f" 3. Move the saved file into place as '{new}'", + file=sys.stderr, + ) + return None + + # Step 2 — move tmp to the new location, handling the name-collision case. + # + # When new is located under the legacy path (e.g. XDG_CONFIG_HOME unset, or + # set to ~/.config), legacy (~/.config/nsls2, a file) sits exactly where the + # 'nsls2' directory component of new.parent must be created. We must remove + # the legacy file before mkdir can succeed; since tmp already holds a full + # copy, this is safe. + # + # When XDG_CONFIG_HOME points to a genuinely different directory, new lives + # in a separate tree and there is no collision; legacy is left untouched + # until after the new file is successfully written. + # + # Paths are compared after resolution so that XDG_CONFIG_HOME values that + # refer to ~/.config via an alternative spelling (trailing slash, '..' segment, + # or a symlink) are still recognised as the collision case. + legacy_resolved = legacy.resolve() + new_under_legacy = any( + legacy_resolved == parent.resolve() for parent in new.parents + ) + legacy_removed = False + # Record which ancestor directories of new.parent don't yet exist so that, + # on failure, we can undo exactly what mkdir created — and no more. This + # list is computed after any legacy.unlink() so the freed 'nsls2' name is + # included when it is part of the path that must be created. + created_dirs: list[Path] = [] + try: + if new_under_legacy: + legacy.unlink() # free the 'nsls2' name; tmp holds the copy + legacy_removed = True + # Collect innermost-first ancestors that mkdir will create. + p = new.parent + while not p.exists(): + created_dirs.append(p) + p = p.parent + new.parent.mkdir(parents=True, exist_ok=True) + shutil.move(str(tmp), str(new)) # cross-device safe + except OSError as exc: + # Recovery: restore legacy from tmp when possible. + if legacy_removed: + # The legacy name was freed; mkdir may have created directory + # entries in its place. Remove only the empty directories we + # created (innermost-first, rmdir only — never recursive) so + # that the legacy name is available again for restore. + for d in created_dirs: + try: + if d.is_dir(): + d.rmdir() + except OSError: + break + if not legacy.exists(): + # The legacy name is free — move tmp straight back. + try: + shutil.move(str(tmp), str(legacy)) + print( + f"Warning: nsls2api config migration failed ({exc}). " + f"Your settings remain at '{legacy}'. " + f"To migrate manually:\n" + f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" + f" 2. Create the directory '{new.parent}'\n" + f" 3. Move the saved file into place as '{new}'", + file=sys.stderr, + ) + except OSError: + print( + f"Warning: nsls2api config migration failed ({exc}) and " + f"could not be recovered. " + f"Your settings are at '{tmp}'. " + f"To recover:\n" + f" 1. Create the directory '{new.parent}'\n" + f" 2. Move '{tmp}' into place as '{new}'", + file=sys.stderr, + ) + else: + # Created dirs couldn't all be removed; legacy name still + # occupied. Settings safe in tmp. + print( + f"Warning: nsls2api config migration failed ({exc}) and " + f"could not be recovered. " + f"Your settings are at '{tmp}'. " + f"To recover:\n" + f" 1. Create the directory '{new.parent}'\n" + f" 2. Move '{tmp}' into place as '{new}'", + file=sys.stderr, + ) + else: + # Legacy was not removed (XDG case, or copy step) — settings intact. + tmp.unlink(missing_ok=True) + print( + f"Warning: nsls2api config migration failed ({exc}). " + f"Your settings remain at '{legacy}'. " + f"To migrate manually:\n" + f" 1. Move '{legacy}' aside (e.g. rename it to '{legacy}.bak')\n" + f" 2. Create the directory '{new.parent}'\n" + f" 3. Move the saved file into place as '{new}'", + file=sys.stderr, + ) + return None + + # Step 3 — migration succeeded. + # XDG case: legacy was not removed in step 2; delete it now. + if not new_under_legacy: + legacy.unlink(missing_ok=True) + return new + + +def read_legacy_fallback( + config: configparser.ConfigParser, config_filepath: Path +) -> None: + """Populate *config* from the legacy file when *config_filepath* is absent. + + Called by ``Config.read()`` when migration was skipped or failed and + the new config file still does not exist. This preserves back-compat: + existing settings (base_url, token) are not silently dropped. + + Modifies *config* in-place; returns ``None``. + """ + legacy = legacy_filepath() + if legacy.is_file(): + config.read(legacy) diff --git a/src/nsls2api/tests/cli/test_settings.py b/src/nsls2api/tests/cli/test_settings.py index 52a56c1e..d88676f7 100644 --- a/src/nsls2api/tests/cli/test_settings.py +++ b/src/nsls2api/tests/cli/test_settings.py @@ -1,4 +1,7 @@ -"""Tests for nsls2api.cli.settings — config path, read/write, and legacy migration.""" +"""Tests for nsls2api.cli.settings — config path and read/write. + +Migration and legacy fallback tests live in test_settings_migration.py. +""" from __future__ import annotations @@ -17,14 +20,6 @@ # Helpers # --------------------------------------------------------------------------- -def _make_legacy(home: Path, content: str = "[api]\nbase_url = http://127.0.0.1:8080\n") -> Path: - """Create the legacy bare-file config at /.config/nsls2.""" - legacy = home / ".config" / "nsls2" - legacy.parent.mkdir(parents=True, exist_ok=True) - legacy.write_text(content) - return legacy - - def _patch_home(tmp_path: Path): """Redirect Path.home() to tmp_path. Env vars are controlled per-test.""" return patch.object(Path, "home", return_value=tmp_path) @@ -89,369 +84,7 @@ def test_get_value_missing_returns_none(self, tmp_path: Path): # --------------------------------------------------------------------------- -# migrate_legacy_config -# --------------------------------------------------------------------------- - -class TestMigrateLegacyConfig: - def test_migrates_legacy_file(self, tmp_path: Path): - """Legacy bare file is migrated to the new location; content is preserved. - - In the default (no-XDG) case the legacy file occupies the path component - that must become a directory. Migration copies the content to a temp file - first, removes the legacy file to free the name, then moves the temp file - to the new path. After migration: - - ~/.config/nsls2 must be a DIRECTORY (not a file) - - ~/.config/nsls2/api/cli.ini must exist with original content - - no temp files remain in ~/.config/ - """ - legacy = _make_legacy(tmp_path) - dot_config = tmp_path / ".config" - with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): - os.environ.pop("XDG_CONFIG_HOME", None) - new_path = Config.migrate_legacy_config() - - assert new_path is not None - assert new_path == tmp_path / ".config" / "nsls2" / "api" / "cli.ini" - assert new_path.exists() - assert legacy.is_dir() # legacy path is now a directory - content = new_path.read_text() - assert "base_url" in content - assert "127.0.0.1:8080" in content - # No temp files left behind. - leftover = list(dot_config.glob(".nsls2-migrate-*")) - assert leftover == [], f"Unexpected temp files left over: {leftover}" - - def test_no_op_when_new_already_exists(self, tmp_path: Path): - """Migration is skipped when the new config file is already present. - - The new.exists() short-circuit fires before any legacy handling, so - this test only needs the new file to be in place — no legacy required. - """ - new = tmp_path / ".config" / "nsls2" / "api" / "cli.ini" - new.parent.mkdir(parents=True, exist_ok=True) - new.write_text("[api]\nbase_url = https://already.here\n") - - with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): - os.environ.pop("XDG_CONFIG_HOME", None) - result = Config.migrate_legacy_config() - - assert result is None - assert new.read_text().startswith("[api]\nbase_url = https://already.here") - - def test_no_op_when_legacy_absent(self, tmp_path: Path): - """Migration is a no-op when the legacy file doesn't exist.""" - with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): - os.environ.pop("XDG_CONFIG_HOME", None) - result = Config.migrate_legacy_config() - assert result is None - - def test_no_op_when_legacy_is_directory(self, tmp_path: Path): - """Migration skips silently if ~/.config/nsls2 is already a directory.""" - legacy_dir = tmp_path / ".config" / "nsls2" - legacy_dir.mkdir(parents=True, exist_ok=True) - (legacy_dir / "cli").mkdir() - - with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): - os.environ.pop("XDG_CONFIG_HOME", None) - result = Config.migrate_legacy_config() - assert result is None - - @pytest.mark.skipif(sys.platform == "win32", reason="chmod not meaningful on Windows") - def test_warns_and_skips_when_parent_dir_not_writable(self, tmp_path: Path, capsys): - """Non-writable parent directory: warning is printed with platform-neutral - hint; no exception raised; returns None; legacy file untouched.""" - legacy = _make_legacy(tmp_path) - dot_config = tmp_path / ".config" - dot_config.chmod(0o555) # remove write permission from the containing dir - - try: - with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): - os.environ.pop("XDG_CONFIG_HOME", None) - new_path = Config.get_filepath() - result = Config.migrate_legacy_config() - finally: - dot_config.chmod(0o755) # restore so tmp_path cleanup works - - assert result is None - captured = capsys.readouterr() - assert "migration skipped" in captured.err - # Warning identifies the non-writable directory, not the file. - assert str(dot_config) in captured.err - # Hint must use platform-neutral numbered steps, not POSIX shell commands. - assert "Create the directory" in captured.err - assert str(new_path.parent) in captured.err - - @pytest.mark.skipif(sys.platform == "win32", reason="chmod not meaningful on Windows") - def test_readonly_file_in_writable_dir_migrates(self, tmp_path: Path): - """A read-only legacy file in a writable parent directory migrates - successfully. The permission check gates on the parent dir (needed for - rename/unlink), not on the file itself.""" - legacy = _make_legacy(tmp_path) - legacy.chmod(0o444) # file read-only, but parent dir is writable - - try: - with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): - os.environ.pop("XDG_CONFIG_HOME", None) - new_path = Config.migrate_legacy_config() - finally: - # If migration succeeded the file is gone; restore only if still there. - if legacy.is_file(): - legacy.chmod(0o644) - - assert new_path is not None - assert new_path.exists() - assert "base_url" in new_path.read_text() - - def test_migration_triggered_by_read(self, tmp_path: Path): - """Calling read() auto-migrates the legacy file and returns its contents.""" - _make_legacy(tmp_path, "[api]\nbase_url = http://127.0.0.1:8080\ntoken = abc\n") - legacy = tmp_path / ".config" / "nsls2" - - with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): - os.environ.pop("XDG_CONFIG_HOME", None) - cfg = Config.read() - - assert cfg.get("api", "base_url") == "http://127.0.0.1:8080" - assert cfg.get("api", "token") == "abc" - assert legacy.is_dir() # legacy path is now a directory - new = tmp_path / ".config" / "nsls2" / "api" / "cli.ini" - assert new.exists() - - def test_copy_failure_cleans_up_temp_and_reports_legacy( - self, tmp_path: Path, capsys - ): - """If the initial copy step raises, migrate_legacy_config(): - - returns None - - legacy file is still intact - - no .nsls2-migrate-* temp files are left behind - - warning says settings remain at legacy - """ - import nsls2api.cli.settings as settings_mod - - legacy = _make_legacy(tmp_path) - dot_config = tmp_path / ".config" - - with ( - _patch_home(tmp_path), - patch.dict(os.environ, {}, clear=False), - patch.object(settings_mod.shutil, "copy2", side_effect=OSError("disk full")), - ): - os.environ.pop("XDG_CONFIG_HOME", None) - result = Config.migrate_legacy_config() - - assert result is None - assert legacy.is_file() # legacy untouched - assert legacy.read_text().startswith("[api]") - leftover = list(dot_config.glob(".nsls2-migrate-*")) - assert leftover == [], f"Temp files leaked: {leftover}" - captured = capsys.readouterr() - assert "remain at" in captured.err - assert "settings are at" not in captured.err - - def test_shutil_move_failure_restores_legacy(self, tmp_path: Path, capsys): - """If shutil.move raises after legacy has been unlinked (no-XDG collision - case), the legacy file is restored from the temp copy and the warning says - settings remain at legacy — no data loss.""" - import nsls2api.cli.settings as settings_mod - - content = "[api]\nbase_url = http://127.0.0.1:8080\ntoken = secret\n" - legacy = _make_legacy(tmp_path, content) - - # Simulate a cross-device rename failure: the migration move raises, but - # the restore move (second call) must succeed so recovery can complete. - _real_move = settings_mod.shutil.move - _calls = [0] - - def _move_fails_once(src, dst, *args, **kwargs): - _calls[0] += 1 - if _calls[0] == 1: - raise OSError("EXDEV") - return _real_move(src, dst, *args, **kwargs) - - with ( - _patch_home(tmp_path), - patch.dict(os.environ, {}, clear=False), - patch.object(settings_mod.shutil, "move", side_effect=_move_fails_once), - ): - os.environ.pop("XDG_CONFIG_HOME", None) - result = Config.migrate_legacy_config() - - assert result is None - assert legacy.is_file(), "Legacy config must be restored as a file" - assert "base_url" in legacy.read_text(), "Legacy config content was lost!" - assert "secret" in legacy.read_text(), "Token was lost!" - captured = capsys.readouterr() - assert "remain at" in captured.err - assert "settings are at" not in captured.err - - def test_xdg_destination_tree_not_deleted_on_move_failure( - self, tmp_path: Path, capsys - ): - """Regression: when XDG_CONFIG_HOME is set, new.parent may be a - pre-existing directory unrelated to the legacy path. A shutil.move - failure must NOT delete that directory. No rmtree of the destination - tree is performed.""" - import nsls2api.cli.settings as settings_mod - - xdg = tmp_path / "xdg" - xdg.mkdir() - # Pre-create destination tree with a sentinel file. - existing_api_dir = xdg / "nsls2" / "api" - existing_api_dir.mkdir(parents=True) - sentinel = existing_api_dir / "sentinel.txt" - sentinel.write_text("do not delete me") - - legacy = _make_legacy(tmp_path) - - with ( - _patch_home(tmp_path), - patch.dict(os.environ, {"XDG_CONFIG_HOME": str(xdg)}, clear=False), - patch.object(settings_mod.shutil, "move", side_effect=OSError("EXDEV")), - ): - result = Config.migrate_legacy_config() - - assert result is None - assert sentinel.exists(), "Destination tree was deleted on move failure!" - assert sentinel.read_text() == "do not delete me" - # Legacy file must still be present (XDG case: legacy not unlinked before move). - assert legacy.is_file() - captured = capsys.readouterr() - assert "remain at" in captured.err - - @pytest.mark.parametrize("xdg_suffix", [ - "", # exact: XDG_CONFIG_HOME=/tmp/.../home/.config - "/", # trailing slash - "/../.config" # unnormalized '..' segment resolving to the same dir - ]) - def test_migrates_when_xdg_equals_config_dir( - self, tmp_path: Path, xdg_suffix: str - ): - """When XDG_CONFIG_HOME resolves to ~/.config (via exact match, trailing - slash, or '..' segment), migration takes the collision branch and produces - the same result as when XDG_CONFIG_HOME is unset: - - new config written to ~/.config/nsls2/api/cli.ini - - legacy path becomes a directory - - no temp files left behind - """ - dot_config = tmp_path / ".config" - dot_config.mkdir(parents=True, exist_ok=True) - legacy = _make_legacy(tmp_path) - xdg_value = str(dot_config) + xdg_suffix - - with ( - _patch_home(tmp_path), - patch.dict(os.environ, {"XDG_CONFIG_HOME": xdg_value}, clear=False), - ): - new_path = Config.migrate_legacy_config() - - assert new_path is not None - # Compare resolved paths: XDG values with trailing slash or '..' segments - # produce an unresolved new_path; normalise both sides before comparing. - expected = (tmp_path / ".config" / "nsls2" / "api" / "cli.ini").resolve() - assert new_path.resolve() == expected - assert new_path.exists() - assert "base_url" in new_path.read_text() - assert legacy.is_dir(), "Legacy path must become a directory after migration" - leftover = list(dot_config.glob(".nsls2-migrate-*")) - assert leftover == [], f"Unexpected temp files left over: {leftover}" - - def test_xdg_equals_config_dir_move_failure_restores_legacy( - self, tmp_path: Path, capsys - ): - """When XDG_CONFIG_HOME=~/.config and shutil.move fails, the legacy file - is restored (collision branch recovery), not silently discarded.""" - import nsls2api.cli.settings as settings_mod - - dot_config = tmp_path / ".config" - dot_config.mkdir(parents=True, exist_ok=True) - content = "[api]\nbase_url = http://127.0.0.1:8080\ntoken = secret\n" - legacy = _make_legacy(tmp_path, content) - - # Simulate a cross-device rename failure: the migration move raises, but - # the restore move (second call) must succeed so recovery can complete. - _real_move = settings_mod.shutil.move - _calls = [0] - - def _move_fails_once(src, dst, *args, **kwargs): - _calls[0] += 1 - if _calls[0] == 1: - raise OSError("EXDEV") - return _real_move(src, dst, *args, **kwargs) - - with ( - _patch_home(tmp_path), - patch.dict( - os.environ, - {"XDG_CONFIG_HOME": str(dot_config)}, - clear=False, - ), - patch.object(settings_mod.shutil, "move", side_effect=_move_fails_once), - ): - result = Config.migrate_legacy_config() - - assert result is None - assert legacy.is_file(), "Legacy config must be restored as a file" - assert "base_url" in legacy.read_text() - assert "secret" in legacy.read_text() - captured = capsys.readouterr() - assert "remain at" in captured.err - assert "settings are at" not in captured.err - - -# --------------------------------------------------------------------------- -# R4 — read() legacy fallback -# --------------------------------------------------------------------------- - -class TestReadLegacyFallback: - """When migration is skipped/failed, read() must still return legacy values.""" - - def test_read_falls_back_to_legacy_when_migration_skipped( - self, tmp_path: Path - ): - """When migration is skipped (returns None), read() should still return - the legacy base_url rather than an empty config.""" - _make_legacy( - tmp_path, "[api]\nbase_url = http://127.0.0.1:8080\ntoken = mytoken\n" - ) - with ( - _patch_home(tmp_path), - patch.dict(os.environ, {}, clear=False), - patch.object(Config, "migrate_legacy_config", return_value=None), - ): - os.environ.pop("XDG_CONFIG_HOME", None) - cfg = Config.read() - - assert cfg.get("api", "base_url") == "http://127.0.0.1:8080" - assert cfg.get("api", "token") == "mytoken" - - def test_read_falls_back_to_legacy_when_new_absent(self, tmp_path: Path): - """When migration is suppressed and no new config exists, read() falls back - to the legacy file (R4 fallback path in read()).""" - _make_legacy( - tmp_path, "[api]\nbase_url = https://api.example.com\n" - ) - # Patch migrate_legacy_config to a no-op so migration never runs and the - # new config file is never created — this isolates the read() fallback. - with ( - _patch_home(tmp_path), - patch.dict(os.environ, {}, clear=False), - patch.object(Config, "migrate_legacy_config", return_value=None), - ): - os.environ.pop("XDG_CONFIG_HOME", None) - cfg = Config.read() - - assert cfg.get("api", "base_url") == "https://api.example.com" - - def test_read_returns_empty_config_when_both_absent(self, tmp_path: Path): - """When neither new nor legacy config exists, read() returns an empty config.""" - with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): - os.environ.pop("XDG_CONFIG_HOME", None) - cfg = Config.read() - assert not cfg.sections() - - -# --------------------------------------------------------------------------- -# R5 — Windows branch of get_filepath() +# Windows branch of get_filepath() # --------------------------------------------------------------------------- @pytest.mark.skipif(sys.platform != "win32", reason="Only run on Windows") diff --git a/src/nsls2api/tests/cli/test_settings_migration.py b/src/nsls2api/tests/cli/test_settings_migration.py new file mode 100644 index 00000000..7acb0e9d --- /dev/null +++ b/src/nsls2api/tests/cli/test_settings_migration.py @@ -0,0 +1,404 @@ +"""Tests for nsls2api.cli.settings_migration — legacy config migration and fallback.""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest + +from nsls2api.cli import settings_migration +from nsls2api.cli.settings import Config + + +# --------------------------------------------------------------------------- +# Helpers (duplicated from test_settings.py — both are intentionally +# self-contained so this module can be deleted independently when legacy +# support is dropped) +# --------------------------------------------------------------------------- + +def _make_legacy(home: Path, content: str = "[api]\nbase_url = http://127.0.0.1:8080\n") -> Path: + """Create the legacy bare-file config at /.config/nsls2.""" + legacy = home / ".config" / "nsls2" + legacy.parent.mkdir(parents=True, exist_ok=True) + legacy.write_text(content) + return legacy + + +def _patch_home(tmp_path: Path): + """Redirect Path.home() to tmp_path. Env vars are controlled per-test.""" + return patch.object(Path, "home", return_value=tmp_path) + + +# --------------------------------------------------------------------------- +# migrate_legacy_config +# --------------------------------------------------------------------------- + +class TestMigrateLegacyConfig: + def test_migrates_legacy_file(self, tmp_path: Path): + """Legacy bare file is migrated to the new location; content is preserved. + + In the default (no-XDG) case the legacy file occupies the path component + that must become a directory. Migration copies the content to a temp file + first, removes the legacy file to free the name, then moves the temp file + to the new path. After migration: + - ~/.config/nsls2 must be a DIRECTORY (not a file) + - ~/.config/nsls2/api/cli.ini must exist with original content + - no temp files remain in ~/.config/ + """ + legacy = _make_legacy(tmp_path) + dot_config = tmp_path / ".config" + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + config_filepath = Config.get_filepath() + new_path = settings_migration.migrate_legacy_config(config_filepath) + + assert new_path is not None + assert new_path == tmp_path / ".config" / "nsls2" / "api" / "cli.ini" + assert new_path.exists() + assert legacy.is_dir() # legacy path is now a directory + content = new_path.read_text() + assert "base_url" in content + assert "127.0.0.1:8080" in content + # No temp files left behind. + leftover = list(dot_config.glob(".nsls2-migrate-*")) + assert leftover == [], f"Unexpected temp files left over: {leftover}" + + def test_no_op_when_new_already_exists(self, tmp_path: Path): + """Migration is skipped when the new config file is already present. + + The new.exists() short-circuit fires before any legacy handling, so + this test only needs the new file to be in place — no legacy required. + """ + new = tmp_path / ".config" / "nsls2" / "api" / "cli.ini" + new.parent.mkdir(parents=True, exist_ok=True) + new.write_text("[api]\nbase_url = https://already.here\n") + + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + config_filepath = Config.get_filepath() + result = settings_migration.migrate_legacy_config(config_filepath) + + assert result is None + assert new.read_text().startswith("[api]\nbase_url = https://already.here") + + def test_no_op_when_legacy_absent(self, tmp_path: Path): + """Migration is a no-op when the legacy file doesn't exist.""" + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + config_filepath = Config.get_filepath() + result = settings_migration.migrate_legacy_config(config_filepath) + assert result is None + + def test_no_op_when_legacy_is_directory(self, tmp_path: Path): + """Migration skips silently if ~/.config/nsls2 is already a directory.""" + legacy_dir = tmp_path / ".config" / "nsls2" + legacy_dir.mkdir(parents=True, exist_ok=True) + (legacy_dir / "cli").mkdir() + + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + config_filepath = Config.get_filepath() + result = settings_migration.migrate_legacy_config(config_filepath) + assert result is None + + @pytest.mark.skipif(sys.platform == "win32", reason="chmod not meaningful on Windows") + def test_warns_and_skips_when_parent_dir_not_writable(self, tmp_path: Path, capsys): + """Non-writable parent directory: warning is printed with platform-neutral + hint; no exception raised; returns None; legacy file untouched.""" + legacy = _make_legacy(tmp_path) + dot_config = tmp_path / ".config" + dot_config.chmod(0o555) # remove write permission (execute retained) from the containing dir + + try: + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + new_path = Config.get_filepath() + result = settings_migration.migrate_legacy_config(new_path) + finally: + dot_config.chmod(0o755) # restore so tmp_path cleanup works + + assert result is None + captured = capsys.readouterr() + assert "migration skipped" in captured.err + # Warning identifies the non-writable directory, not the file. + assert str(dot_config) in captured.err + # Hint must use platform-neutral numbered steps, not POSIX shell commands. + assert "Create the directory" in captured.err + assert str(new_path.parent) in captured.err + + @pytest.mark.skipif(sys.platform == "win32", reason="chmod not meaningful on Windows") + def test_readonly_file_in_writable_dir_migrates(self, tmp_path: Path): + """A read-only legacy file in a writable parent directory migrates + successfully. The permission check gates on the parent dir (needed for + rename/unlink), not on the file itself.""" + legacy = _make_legacy(tmp_path) + legacy.chmod(0o444) # file read-only, but parent dir is writable + + try: + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + config_filepath = Config.get_filepath() + new_path = settings_migration.migrate_legacy_config(config_filepath) + finally: + # If migration succeeded the file is gone; restore only if still there. + if legacy.is_file(): + legacy.chmod(0o644) + + assert new_path is not None + assert new_path.exists() + assert "base_url" in new_path.read_text() + + def test_migration_triggered_by_read(self, tmp_path: Path): + """Calling read() auto-migrates the legacy file and returns its contents.""" + _make_legacy(tmp_path, "[api]\nbase_url = http://127.0.0.1:8080\ntoken = abc\n") + legacy = tmp_path / ".config" / "nsls2" + + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + cfg = Config.read() + + assert cfg.get("api", "base_url") == "http://127.0.0.1:8080" + assert cfg.get("api", "token") == "abc" + assert legacy.is_dir() # legacy path is now a directory + new = tmp_path / ".config" / "nsls2" / "api" / "cli.ini" + assert new.exists() + + def test_copy_failure_cleans_up_temp_and_reports_legacy( + self, tmp_path: Path, capsys + ): + """If the initial copy step raises, migrate_legacy_config(): + - returns None + - legacy file is still intact + - no .nsls2-migrate-* temp files are left behind + - warning says settings remain at legacy + """ + import nsls2api.cli.settings_migration as migration_mod + + legacy = _make_legacy(tmp_path) + dot_config = tmp_path / ".config" + + with ( + _patch_home(tmp_path), + patch.dict(os.environ, {}, clear=False), + patch.object(migration_mod.shutil, "copy2", side_effect=OSError("disk full")), + ): + os.environ.pop("XDG_CONFIG_HOME", None) + config_filepath = Config.get_filepath() + result = settings_migration.migrate_legacy_config(config_filepath) + + assert result is None + assert legacy.is_file() # legacy untouched + assert legacy.read_text().startswith("[api]") + leftover = list(dot_config.glob(".nsls2-migrate-*")) + assert leftover == [], f"Temp files leaked: {leftover}" + captured = capsys.readouterr() + assert "remain at" in captured.err + assert "settings are at" not in captured.err + + def test_shutil_move_failure_restores_legacy(self, tmp_path: Path, capsys): + """If shutil.move raises after legacy has been unlinked (no-XDG collision + case), the legacy file is restored from the temp copy and the warning says + settings remain at legacy — no data loss.""" + import nsls2api.cli.settings_migration as migration_mod + + content = "[api]\nbase_url = http://127.0.0.1:8080\ntoken = secret\n" + legacy = _make_legacy(tmp_path, content) + + # Simulate a cross-device rename failure: the migration move raises, but + # the restore move (second call) must succeed so recovery can complete. + _real_move = migration_mod.shutil.move + _calls = [0] + + def _move_fails_once(src, dst, *args, **kwargs): + _calls[0] += 1 + if _calls[0] == 1: + raise OSError("EXDEV") + return _real_move(src, dst, *args, **kwargs) + + with ( + _patch_home(tmp_path), + patch.dict(os.environ, {}, clear=False), + patch.object(migration_mod.shutil, "move", side_effect=_move_fails_once), + ): + os.environ.pop("XDG_CONFIG_HOME", None) + config_filepath = Config.get_filepath() + result = settings_migration.migrate_legacy_config(config_filepath) + + assert result is None + assert legacy.is_file(), "Legacy config must be restored as a file" + assert "base_url" in legacy.read_text(), "Legacy config content was lost!" + assert "secret" in legacy.read_text(), "Token was lost!" + captured = capsys.readouterr() + assert "remain at" in captured.err + assert "settings are at" not in captured.err + + def test_xdg_destination_tree_not_deleted_on_move_failure( + self, tmp_path: Path, capsys + ): + """Regression: when XDG_CONFIG_HOME is set, new.parent may be a + pre-existing directory unrelated to the legacy path. A shutil.move + failure must NOT delete that directory. No rmtree of the destination + tree is performed.""" + import nsls2api.cli.settings_migration as migration_mod + + xdg = tmp_path / "xdg" + xdg.mkdir() + # Pre-create destination tree with a sentinel file. + existing_api_dir = xdg / "nsls2" / "api" + existing_api_dir.mkdir(parents=True) + sentinel = existing_api_dir / "sentinel.txt" + sentinel.write_text("do not delete me") + + legacy = _make_legacy(tmp_path) + + with ( + _patch_home(tmp_path), + patch.dict(os.environ, {"XDG_CONFIG_HOME": str(xdg)}, clear=False), + patch.object(migration_mod.shutil, "move", side_effect=OSError("EXDEV")), + ): + config_filepath = Config.get_filepath() + result = settings_migration.migrate_legacy_config(config_filepath) + + assert result is None + assert sentinel.exists(), "Destination tree was deleted on move failure!" + assert sentinel.read_text() == "do not delete me" + # Legacy file must still be present (XDG case: legacy not unlinked before move). + assert legacy.is_file() + captured = capsys.readouterr() + assert "remain at" in captured.err + + @pytest.mark.parametrize("xdg_suffix", [ + "", # exact: XDG_CONFIG_HOME=/tmp/.../home/.config + "/", # trailing slash + "/../.config" # unnormalized '..' segment resolving to the same dir + ]) + def test_migrates_when_xdg_equals_config_dir( + self, tmp_path: Path, xdg_suffix: str + ): + """When XDG_CONFIG_HOME resolves to ~/.config (via exact match, trailing + slash, or '..' segment), migration takes the collision branch and produces + the same result as when XDG_CONFIG_HOME is unset: + - new config written to ~/.config/nsls2/api/cli.ini + - legacy path becomes a directory + - no temp files left behind + """ + dot_config = tmp_path / ".config" + dot_config.mkdir(parents=True, exist_ok=True) + legacy = _make_legacy(tmp_path) + xdg_value = str(dot_config) + xdg_suffix + + with ( + _patch_home(tmp_path), + patch.dict(os.environ, {"XDG_CONFIG_HOME": xdg_value}, clear=False), + ): + config_filepath = Config.get_filepath() + new_path = settings_migration.migrate_legacy_config(config_filepath) + + assert new_path is not None + # Compare resolved paths: XDG values with trailing slash or '..' segments + # produce an unresolved new_path; normalise both sides before comparing. + expected = (tmp_path / ".config" / "nsls2" / "api" / "cli.ini").resolve() + assert new_path.resolve() == expected + assert new_path.exists() + assert "base_url" in new_path.read_text() + assert legacy.is_dir(), "Legacy path must become a directory after migration" + leftover = list(dot_config.glob(".nsls2-migrate-*")) + assert leftover == [], f"Unexpected temp files left over: {leftover}" + + def test_xdg_equals_config_dir_move_failure_restores_legacy( + self, tmp_path: Path, capsys + ): + """When XDG_CONFIG_HOME=~/.config and shutil.move fails, the legacy file + is restored (collision branch recovery), not silently discarded.""" + import nsls2api.cli.settings_migration as migration_mod + + dot_config = tmp_path / ".config" + dot_config.mkdir(parents=True, exist_ok=True) + content = "[api]\nbase_url = http://127.0.0.1:8080\ntoken = secret\n" + legacy = _make_legacy(tmp_path, content) + + # Simulate a cross-device rename failure: the migration move raises, but + # the restore move (second call) must succeed so recovery can complete. + _real_move = migration_mod.shutil.move + _calls = [0] + + def _move_fails_once(src, dst, *args, **kwargs): + _calls[0] += 1 + if _calls[0] == 1: + raise OSError("EXDEV") + return _real_move(src, dst, *args, **kwargs) + + with ( + _patch_home(tmp_path), + patch.dict( + os.environ, + {"XDG_CONFIG_HOME": str(dot_config)}, + clear=False, + ), + patch.object(migration_mod.shutil, "move", side_effect=_move_fails_once), + ): + config_filepath = Config.get_filepath() + result = settings_migration.migrate_legacy_config(config_filepath) + + assert result is None + assert legacy.is_file(), "Legacy config must be restored as a file" + assert "base_url" in legacy.read_text() + assert "secret" in legacy.read_text() + captured = capsys.readouterr() + assert "remain at" in captured.err + assert "settings are at" not in captured.err + + +# --------------------------------------------------------------------------- +# read() legacy fallback +# --------------------------------------------------------------------------- + +class TestReadLegacyFallback: + """When migration is skipped/failed, read() must still return legacy values.""" + + def test_read_falls_back_to_legacy_when_migration_skipped( + self, tmp_path: Path + ): + """When migration is skipped (returns None), read() should still return + the legacy base_url rather than an empty config.""" + _make_legacy( + tmp_path, "[api]\nbase_url = http://127.0.0.1:8080\ntoken = mytoken\n" + ) + with ( + _patch_home(tmp_path), + patch.dict(os.environ, {}, clear=False), + patch("nsls2api.cli.settings_migration.migrate_legacy_config", return_value=None), + ): + os.environ.pop("XDG_CONFIG_HOME", None) + cfg = Config.read() + + assert cfg.get("api", "base_url") == "http://127.0.0.1:8080" + assert cfg.get("api", "token") == "mytoken" + + def test_read_falls_back_to_legacy_when_new_absent(self, tmp_path: Path): + """When migration is suppressed and no new config exists, read() falls back + to the legacy file (fallback path in read()).""" + _make_legacy( + tmp_path, "[api]\nbase_url = https://api.example.com\n" + ) + # Patch migrate_legacy_config to a no-op so migration never runs and the + # new config file is never created — this isolates the read() fallback. + with ( + _patch_home(tmp_path), + patch.dict(os.environ, {}, clear=False), + patch("nsls2api.cli.settings_migration.migrate_legacy_config", return_value=None), + ): + os.environ.pop("XDG_CONFIG_HOME", None) + cfg = Config.read() + + assert cfg.get("api", "base_url") == "https://api.example.com" + + def test_read_returns_empty_config_when_both_absent(self, tmp_path: Path): + """When neither new nor legacy config exists, read() returns an empty config.""" + with _patch_home(tmp_path), patch.dict(os.environ, {}, clear=False): + os.environ.pop("XDG_CONFIG_HOME", None) + cfg = Config.read() + assert not cfg.sections() From a4d07a3c873c1446a9525aaed9be30fea4a056bd Mon Sep 17 00:00:00 2001 From: Padraic Shafer <76011594+padraic-shafer@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:53:57 -0700 Subject: [PATCH 12/14] Update test docstring for clarity on Windows tests Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/nsls2api/tests/cli/test_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nsls2api/tests/cli/test_settings.py b/src/nsls2api/tests/cli/test_settings.py index d88676f7..55378864 100644 --- a/src/nsls2api/tests/cli/test_settings.py +++ b/src/nsls2api/tests/cli/test_settings.py @@ -89,7 +89,7 @@ def test_get_value_missing_returns_none(self, tmp_path: Path): @pytest.mark.skipif(sys.platform != "win32", reason="Only run on Windows") class TestGetFilepathWindows: - """These tests patch sys.platform and run only on Windows.""" + """Run the Windows branch of get_filepath() on real Windows.""" def test_windows_path_with_appdata(self, tmp_path: Path, monkeypatch): """With APPDATA set, uses %APPDATA%/nsls2/api/cli.ini.""" From b57c1022969cc2efc72515fdce7c725e50a1e5a2 Mon Sep 17 00:00:00 2001 From: Padraic Shafer Date: Fri, 4 Sep 2026 17:03:03 -0700 Subject: [PATCH 13/14] fix(cli): treat whitespace-only APPDATA as unset on Windows get_filepath() stripped XDG_CONFIG_HOME on POSIX but not APPDATA on Windows, so a whitespace-only APPDATA produced a config path under a literal ' ' directory. Strip APPDATA and fall back to ~/AppData/Roaming when blank, matching the POSIX branch behaviour. Adds test_windows_blank_appdata_falls_back to TestGetFilepathWindowsSimulated, symmetric to the existing test_blank_xdg_config_home_falls_back. Assisted-by: claude-opus-4-8 Assisted-by: claude-sonnet-4-6 --- src/nsls2api/cli/settings.py | 2 +- src/nsls2api/tests/cli/test_settings.py | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/nsls2api/cli/settings.py b/src/nsls2api/cli/settings.py index 570c7f24..a77dc79b 100644 --- a/src/nsls2api/cli/settings.py +++ b/src/nsls2api/cli/settings.py @@ -32,7 +32,7 @@ def get_filepath() -> Path: %APPDATA% on Windows. """ if sys.platform == "win32": - appdata = os.environ.get("APPDATA", "") + appdata = os.environ.get("APPDATA", "").strip() base = Path(appdata) if appdata else Path.home() / "AppData" / "Roaming" else: xdg = os.environ.get("XDG_CONFIG_HOME", "").strip() diff --git a/src/nsls2api/tests/cli/test_settings.py b/src/nsls2api/tests/cli/test_settings.py index 55378864..974b8c17 100644 --- a/src/nsls2api/tests/cli/test_settings.py +++ b/src/nsls2api/tests/cli/test_settings.py @@ -124,3 +124,15 @@ def test_windows_path_without_appdata_simulated(self, tmp_path: Path, monkeypatc with _patch_home(tmp_path): result = Config.get_filepath() assert result == tmp_path / "AppData" / "Roaming" / "nsls2" / "api" / "cli.ini" + + def test_windows_blank_appdata_falls_back(self, tmp_path: Path, monkeypatch): + """Patch sys.platform to win32; whitespace-only APPDATA → falls back to home. + + Mirrors test_blank_xdg_config_home_falls_back: a blank/whitespace-only + env var should be treated as unset on both platforms. + """ + monkeypatch.setattr(sys, "platform", "win32") + monkeypatch.setenv("APPDATA", " ") + with _patch_home(tmp_path): + result = Config.get_filepath() + assert result == tmp_path / "AppData" / "Roaming" / "nsls2" / "api" / "cli.ini" From 827568d948fe8363a717884f8eba8dd15b6fe87e Mon Sep 17 00:00:00 2001 From: Padraic Shafer Date: Fri, 4 Sep 2026 17:10:30 -0700 Subject: [PATCH 14/14] fix(cli): check that config path is a file --- src/nsls2api/cli/settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nsls2api/cli/settings.py b/src/nsls2api/cli/settings.py index a77dc79b..8469de85 100644 --- a/src/nsls2api/cli/settings.py +++ b/src/nsls2api/cli/settings.py @@ -50,7 +50,7 @@ def read(cls) -> configparser.ConfigParser: config_filepath = cls.get_filepath() settings_migration.migrate_legacy_config(config_filepath) config = configparser.ConfigParser() - if config_filepath.exists(): + if config_filepath.is_file(): config.read(config_filepath) else: settings_migration.read_legacy_fallback(config, config_filepath)