Fix config path mismatch: honor documented ~/.config on every OS - #55
Merged
Merged
Conversation
os.UserConfigDir() resolves to an OS-native directory (e.g. "~/Library/ Application Support" on macOS, %AppData% on Windows) that never matched the ~/.config/lazydeck path this project's README and starter-file comments have always documented. Users hand-editing the documented path on macOS/Windows had their changes silently ignored, since lazydeck read a different, hidden file. - configDir() now resolves $XDG_CONFIG_HOME/lazydeck, falling back to ~/.config/lazydeck, regardless of OS. - migrateLegacyPath() does a one-time, best-effort migration of any config already sitting at the old OS-native location into the documented path, printing a one-line stderr notice. It never overwrites a file already present at the documented path. - Added tests for XDG override, ~/.config fallback, legacy migration, and non-clobbering of an existing documented-path file. - Updated README to explain the canonical path and migration behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 27bb5a8d-6d4c-48ff-88b0-3a109a4bff0c
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
New tests can be unsafe/flaky on Windows because they don’t fully sandbox the environment variables that control os.UserHomeDir()/os.UserConfigDir(), risking reads/writes (and deletion) in real user config locations during local runs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aligns lazydeck’s actual config file location with the long-documented ~/.config/lazydeck path (or $XDG_CONFIG_HOME/lazydeck), adds a one-time migration from Go’s os.UserConfigDir() OS-native location, and updates documentation/tests accordingly.
Changes:
- Centralized config path resolution to always use
$XDG_CONFIG_HOME/lazydeckor~/.config/lazydeckacross all OSes, with legacy migration fromos.UserConfigDir(). - Added migration-focused tests for XDG override,
~/.configfallback, migration behavior, and non-clobbering. - Updated README to document the canonical location and migration notice.
File summaries
| File | Description |
|---|---|
| README.md | Documents the canonical config path and the one-time migration behavior. |
| internal/config/config.go | Implements canonical cross-platform config dir resolution and legacy-path migration. |
| internal/config/config_test.go | Adds tests covering XDG behavior, fallback, migration, and non-overwrite guarantees. |
Review details
Suppressed comments (3)
internal/config/config_test.go:323
- These path-resolution tests assume os.UserHomeDir() will honor HOME, but on Windows it uses USERPROFILE (and os.UserConfigDir uses APPDATA). Without setting those too, this test can be flaky or can accidentally touch real user directories on Windows.
t.Setenv("XDG_CONFIG_HOME", "")
home := t.TempDir()
t.Setenv("HOME", home)
internal/config/config_test.go:345
- This test sets HOME to a temp dir but still calls os.UserConfigDir(); on Windows that uses APPDATA/LOCALAPPDATA and can create/delete files in the real user profile. Set USERPROFILE/APPDATA/LOCALAPPDATA too so the legacy path is fully sandboxed.
t.Setenv("XDG_CONFIG_HOME", "")
home := t.TempDir()
t.Setenv("HOME", home)
internal/config/config_test.go:403
- Same sandboxing issue as other tests: setting HOME alone does not reliably redirect os.UserHomeDir/os.UserConfigDir on Windows. Set USERPROFILE/APPDATA/LOCALAPPDATA so this test can't touch or delete real user config during local runs on Windows.
t.Setenv("XDG_CONFIG_HOME", "")
home := t.TempDir()
t.Setenv("HOME", home)
- Files reviewed: 3/3 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.
Comment on lines
+249
to
+251
| if _, err := os.Stat(newPath); err == nil { | ||
| return | ||
| } |
Comment on lines
+302
to
+306
| // migrateLegacyPath calls os.UserConfigDir(), which reads $HOME | ||
| // directly; isolate it to an empty temp dir so this test can never | ||
| // read (and delete) the real user's actual legacy config file. | ||
| t.Setenv("HOME", t.TempDir()) | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
os.UserConfigDir()resolves to an OS-native directory (e.g.~/Library/Application Supporton macOS,%AppData%on Windows) — not~/.config. But README.md and the starter-file comments have always documented~/.config/lazydeck/devices.toml/config.yml.On macOS/Windows this meant a user hand-editing the documented path had their changes silently ignored, since lazydeck actually read/wrote a different, hidden file.
Fix
configDir()now resolves$XDG_CONFIG_HOME/lazydeck, falling back to~/.config/lazydeck, on every OS.migrateLegacyPath()does a one-time, best-effort migration: if a config already exists at the old OS-native location and nothing exists yet at the documented path, it's moved into place (never overwrites an existing documented-path file), with a one-line stderr notice.~/.configfallback, legacy migration, and non-clobbering.Testing
go build ./... && go vet ./... && go test ./...all pass.