fix(cli): migrate config from bare file to XDG-compliant path

[Padraic Shafer (padraic-shafer)]

The CLI config was historically 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.ini (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.

Testing

# Before migration
$ ls -la ~/.config/nsls2
-rw-r--r--@ 1 pshafer  pshafer  40 May  9  2025 ~/.config/nsls2

$ cat ~/.config/nsls2
[api]
base_url = http://127.0.0.1:8080

# Auto-migration when running the CLI tool
$ uv run nsls2api api status
...

# After migration
$ ls -la ~/.config/nsls2
total 0
drwxr-xr-x@  3 pshafer  pshafer   96 Sep  4 14:50 .
drwxr-xr-x  15 pshafer  pshafer  480 Sep  4 14:50 ..
drwxr-xr-x@  3 pshafer  pshafer   96 Sep  4 14:50 api

$ cat ~/.config/nsls2/api/cli.ini
[api]
base_url = http://127.0.0.1:8080

[Copilot]

🟡 Changes recommended

The migration uses Path.replace() (can fail across filesystems) and the new filename suggests TOML while the code still reads/writes INI via configparser, both of which are likely to cause user-impacting issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the CLI’s config storage to avoid the historical collision where ~/.config/nsls2 was used as a plain file, preventing other nsls2 tools from using ~/.config/nsls2/ as a directory. It introduces an XDG-compliant per-app config file path and attempts a one-time migration of the legacy config file on first read.

Changes:

• Move config location to an XDG-compliant path ($XDG_CONFIG_HOME on POSIX, %APPDATA% on Windows).
• Add legacy-config migration logic that relocates the old bare-file config into the new directory structure.
• Trigger migration automatically on config reads, with warnings (stderr) on migration failures.

File summaries

File	Description
src/nsls2api/cli/settings.py	Implements new config path resolution and legacy config migration on first use.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

There are confirmed backward-compatibility and error-path issues in the migration/read logic that can lose existing settings and emit incorrect recovery guidance.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/nsls2api/cli/settings.py:39

• PR description/title say the new config location is $XDG_CONFIG_HOME/nsls2/api/cli.toml, but the implementation (and tests) use cli.ini via configparser. Please reconcile this (either update the PR description/docs, or switch the code to TOML and adjust migration/tests accordingly) to avoid confusing users and future maintainers.

        """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.
        """
        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.ini"

• Files reviewed: 2/3 changed files
• Comments generated: 4
• Review effort level: Lite

[Padraic Shafer (padraic-shafer)]

Follow-up addressing the Copilot review (commit 68436a4):

• cli.ini (not cli.toml) — matches configparser INI format
• Migration error paths: staging-failure now cleans up the temp file and reports settings remain at legacy (no more misleading temp-path message / leaked file)
• read() falls back to the legacy config when the new file is absent, so base_url/token are never silently dropped
• Final migration move uses shutil.move() for cross-device (EXDEV) safety
• Added simulated Windows (sys.platform=win32, ±APPDATA) tests for get_filepath()
• Corrected the _patch_home test-helper docstring

[Copilot]

🟡 Changes recommended

The current migration failure handling can delete the only copy of the user’s config on certain errors, and a couple of new tests contain assertions that will fail as written.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/nsls2api/tests/cli/test_settings.py:193

• After a successful migration triggered by Config.read(), the legacy path should no longer be a file (it becomes a directory for the new layout). assert not legacy.exists() will fail because the directory exists.

        assert not legacy.exists()
        new = tmp_path / ".config" / "nsls2" / "api" / "cli.ini"
        assert new.exists()

• Files reviewed: 2/3 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The migration rollback logic can’t reliably restore the legacy file after directory creation on POSIX, and several warnings include POSIX-only manual commands that are misleading on Windows.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (2) — in code that hasn't changed since the last review.

src/nsls2api/tests/cli/test_settings.py:339

• This test’s docstring/comments claim there is “no migration trigger”, but Config.read() always calls migrate_legacy_config(), so the test usually exercises the migration path rather than the legacy-read fallback. Patch migrate_legacy_config to a no-op so the test reliably covers the intended fallback behavior.
  src/nsls2api/cli/settings.py:84
• The manual migration hint uses POSIX shell commands (mv, mkdir -p), which will be confusing/incorrect for Windows users who are most likely to hit the legacy-file migration path (older versions wrote ~/.config/nsls2 even on Windows). Consider using a platform-neutral step list (or a Windows-specific hint) in the warning text.

This issue also appears in the following locations of the same file:

• line 107
• line 125
• line 136

src/nsls2api/cli/settings.py:109

• This warning’s manual migration hint uses POSIX shell commands (mv, mkdir -p), which are not appropriate on Windows. A platform-neutral step list avoids shell-specific instructions.

                    f"To migrate manually:\n"
                    f"  mv '{legacy}' '{legacy}.bak' && "
                    f"mkdir -p '{new.parent}' && mv '{legacy}.bak' '{new}'",

src/nsls2api/cli/settings.py:127

• This warning repeats a POSIX-only manual migration command sequence (mv, mkdir -p). Prefer a platform-neutral step list (or a Windows-specific hint) so the message is accurate across supported platforms.

                        f"To migrate manually:\n"
                        f"  mv '{legacy}' '{legacy}.bak' && "
                        f"mkdir -p '{new.parent}' && mv '{legacy}.bak' '{new}'",

src/nsls2api/cli/settings.py:137

• The recovery hint uses POSIX shell commands (mkdir -p, mv), which are not valid in the default Windows shells. Consider a platform-neutral step list to avoid misleading instructions.

                        f"To recover:\n"
                        f"  mkdir -p '{new.parent}' && mv '{tmp}' '{new}'",

• Files reviewed: 2/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Padraic Shafer (padraic-shafer)]

Addressed in 7100155 (suppressed Copilot comments from the previous review round):

• POSIX rollback (settings.py:120): rollback now clears the empty dirs mkdir created (rmtree(new.parent) + legacy.rmdir()) before restoring the legacy file — tmp.replace(legacy) can succeed on POSIX. Tests assert legacy.is_file() after rollback.
• Platform-neutral hints (settings.py:84, 109, 127, 137): all four warning messages now use a numbered step list instead of POSIX mv/mkdir -p shell commands. Hint assertions updated in tests.
• Weak read-fallback test (test_settings.py:322): test_read_falls_back_to_legacy_when_new_absent now patches Config.migrate_legacy_config to a no-op, cleanly isolating the R4 fallback path without migration side-effects.

[Copilot]

🟡 Changes recommended

The rollback logic can recursively delete an existing $XDG_CONFIG_HOME/nsls2/api directory on migration failure, which is a real data-loss risk.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/3 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The migration failure-recovery branch can incorrectly delete the temp copy (and fail to restore legacy) after mkdir() recreates the legacy path as a directory, risking config loss and likely breaking the added rollback tests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/3 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The migration logic is covered by thorough tests (including failure/rollback cases), and the implementation matches the stated behavior without introducing evident regressions.

Review details

• Files reviewed: 2/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The migration logic is well-covered by tests and appears safe, with only a minor warning-message clarity adjustment requested.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/nsls2api/cli/settings.py:85

• In the "legacy parent not writable" warning, step 1 currently tells the user to move/rename the legacy file even though the message just stated that the containing directory is not writable (so rename/move in-place will fail). Consider updating step 1 to first instruct making the directory writable (or running with appropriate permissions) before attempting the rename.

• Files reviewed: 2/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Padraic Shafer (padraic-shafer)]

Refactor of PR changes

After responding to multiple rounds of copilot review, the migration logic for the config filepath grew quite heavy...nearly 70% of settings.py was devoted to migration, even though migration has a finite lifespan.

Extracting this new code into settings_migration.py makes the deprecation path a single-file deletion later, and keeps settings.py focused.

[Copilot]

🟢 Approval recommended

The migration and fallback logic is well-tested and the only remaining feedback is a minor test docstring accuracy nit.

Review details

• Files reviewed: 4/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Config.get_filepath() should treat whitespace-only APPDATA as unset on Windows to avoid producing an invalid base path.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/nsls2api/cli/settings.py:36

• On Windows, APPDATA is treated as set even if it contains only whitespace, which would produce a config path under a literal ' ' directory. Since the POSIX branch already treats a blank XDG_CONFIG_HOME as unset, it’d be more robust/consistent to strip() APPDATA as well before deciding the base path.

• Files reviewed: 4/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

There is a confirmed correctness issue in Config.read() using exists() instead of is_file(), and additional path-hardening fixes are needed to avoid writing config to unintended relative locations.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

src/nsls2api/cli/settings.py:36

• APPDATA is used directly as a Path even if it is a relative path. A misconfigured relative APPDATA would cause the CLI to write config relative to the current working directory rather than a user config directory.

This issue also appears on line 38 of the same file.
src/nsls2api/cli/settings.py:56

• Config.read() checks config_filepath.exists() rather than is_file(). If the path exists but is not a regular file (e.g., a directory named cli.ini), configparser.ConfigParser.read() will silently skip it and the legacy fallback will not run, returning an empty config even when the legacy file still has settings.

src/nsls2api/cli/settings.py:39

• XDG_CONFIG_HOME is used directly as a Path even if it is a relative path. A relative value would cause config to be written relative to the current working directory, which is surprising and can break tooling that expects absolute XDG paths.

            xdg = os.environ.get("XDG_CONFIG_HOME", "").strip()
            base = Path(xdg) if xdg else Path.home() / ".config"

• Files reviewed: 4/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Padraic Shafer (padraic-shafer)]

I'm declining to handle copilot's muted suggestion of handling the pathological case where someone has set XDG_CONFIG_HOME or APPDATA variables to a relative path. Standard accepted practice is that their values must be absolute paths.

[Copilot]

🔵 Needs a closer look

There are unhandled OSError cases in the migration flow (tempfile creation and post-success legacy cleanup) that can still break normal CLI commands despite the intended “warn and continue” contract.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/nsls2api/cli/settings_migration.py:86

• tempfile.mkstemp(...) can raise OSError (disk full, permissions, etc.) before entering the try: that handles migration failures. That exception would currently propagate and can break normal CLI commands, contradicting the function contract to warn and return None on migration failure. Wrap the mkstemp/close step in a try/except OSError and emit the same warning+manual migration instructions used for other failure modes.
  src/nsls2api/cli/settings_migration.py:204
• If legacy deletion fails after a successful migration in the non-collision (XDG elsewhere) case, legacy.unlink(missing_ok=True) will raise and can break the CLI even though the new config was written correctly. This cleanup step should be best-effort: catch OSError, warn to stderr, and continue returning new.

• Files reviewed: 4/5 changed files
• Comments generated: 0 new
• Review effort level: Lite