Skip to content

feat: use XDG base directories in fluid-cli - #67

Merged
aspectrr merged 7 commits into
mainfrom
feat/xdg-base-directories
Feb 22, 2026
Merged

feat: use XDG base directories in fluid-cli#67
aspectrr merged 7 commits into
mainfrom
feat/xdg-base-directories

Conversation

@aspectrr

Copy link
Copy Markdown
Owner

Summary

  • Add fluid-cli/internal/paths package for centralized XDG-compliant path resolution
  • Replace hardcoded ~/.fluid/ with $XDG_CONFIG_HOME/fluid (or ~/.config/fluid on Unix, %AppData%/fluid on Windows)
  • Update config, store, updater, and CLI entry point to use new paths package

Test plan

  • cd fluid-cli && go build ./... compiles clean
  • cd fluid-cli && make test all tests pass
  • Verify XDG_CONFIG_HOME=/tmp/test-fluid fluid --help shows correct default path

🤖 Generated with Claude Code

Replace hardcoded ~/.fluid/ with XDG-compliant paths. New resolution
order: $XDG_CONFIG_HOME/fluid, %AppData%/fluid (Windows), ~/.config/fluid.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 22, 2026 15:15
@claude

claude Bot commented Feb 22, 2026

Copy link
Copy Markdown

Code Review

Good goal — XDG compliance is the right direction for a CLI tool. The implementation is clean, but there are several issues worth addressing before merging.


Breaking change: no migration from ~/.fluid/

This is the biggest concern. Existing users have config at ~/.fluid/config.yaml and state at ~/.fluid/state.db. After this change they silently get a blank slate at ~/.config/fluid/. No warning, no migration.

At minimum: either add startup migration logic that copies the old directory to the new path, or print a one-time warning pointing users to where their data moved. A silent data loss on upgrade is a bad user experience.


Missing tests

CLAUDE.md is explicit: "Every code change needs tests." The new paths package has no paths_test.go. At minimum, test:

  • ConfigDir() with XDG_CONFIG_HOME set
  • ConfigDir() with XDG_CONFIG_HOME unset (should return ~/.config/fluid)
  • ConfigFile() and StateDB() return the expected suffixes

state.db belongs in XDG data dir, not config dir

The XDG spec separates concerns:

  • $XDG_CONFIG_HOME (~/.config) — configuration files
  • $XDG_DATA_HOME (~/.local/share) — persistent application data
  • $XDG_STATE_HOME (~/.local/state) — state files (runtime state, logs, history)

state.db is application state, not config. If you're going to the trouble of XDG compliance, StateDB() should use $XDG_DATA_HOME/fluid/state.db (defaulting to ~/.local/share/fluid/state.db), not the config dir. Mixing them defeats the purpose of the spec.


Error handling regression in sqlite.go

The old code properly surfaced a UserHomeDir failure:

home, err := os.UserHomeDir()
if err != nil {
    return nil, fmt.Errorf("sqlite: get home dir: %w", err)
}

The new paths.StateDB() silently ignores the error (home, _ := os.UserHomeDir()), returning a path like /state.db on failure. paths.ConfigDir() and paths.StateDB() should return (string, error) so callers can handle failures, or the package should panic with a clear message — but silently discarding the error is worse than either.


Windows fallback is wrong

if runtime.GOOS == "windows" {
    if dir, err := os.UserConfigDir(); err == nil {
        return filepath.Join(dir, "fluid")
    }
}
// falls through to:
home, _ := os.UserHomeDir()
return filepath.Join(home, ".config", "fluid")

If os.UserConfigDir() fails on Windows, the code falls through and returns ~/.config/fluid — a Unix-style path that doesn't make sense on Windows. The Windows fallback should either return an error or use a Windows-appropriate default (e.g., %USERPROFILE%\AppData\Roaming\fluid).


macOS uses ~/.config instead of ~/Library/Application Support

This is a design choice, not a bug — many dev tools prefer ~/.config on macOS for cross-platform consistency. Just worth being intentional: os.UserConfigDir() on macOS returns ~/Library/Application Support, which is more "native." If macOS native behavior matters, consider handling it the same way Windows is handled.


PR scope

The API @BasePath change (moving /v1 into each route annotation) is unrelated to XDG paths. Bundling it here makes the diff harder to review and the commit history less clear. Consider splitting it into a separate PR — the OpenAPI change does look correct, it's just a scope concern.


Minor: .gitignore cleanup

Removing fluid-cli from .gitignore is fine — looks like a leftover from when the binary was built in the repo root rather than bin/.


Summary

Issue Severity
No migration from ~/.fluid/ High
Missing tests for paths package High (per CLAUDE.md)
state.db in wrong XDG directory Medium
Error handling regression in paths.go Medium
Windows fallback to Unix path Low
Unrelated API @BasePath change bundled in Low

The core approach is sound — a centralized paths package is clearly better than scattered filepath.Join(home, ".fluid", ...) calls. Address the migration story and add tests and this will be in good shape.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request implements XDG Base Directory Specification support for the fluid-cli configuration system, replacing hardcoded ~/.fluid/ paths with platform-appropriate configuration directories. The PR also updates API documentation to consistently include the /v1 prefix in route paths rather than relying on a BasePath annotation.

Changes:

  • Introduced fluid-cli/internal/paths package for centralized, XDG-compliant path resolution across Unix, macOS, and Windows
  • Refactored config, store, updater, and CLI entry points to use the new paths package
  • Updated OpenAPI specification and Swagger annotations to include /v1 in route paths with BasePath changed from /v1 to /

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
fluid-cli/internal/paths/paths.go New package providing XDG-compliant path resolution with platform-specific defaults
fluid-cli/internal/config/config.go Updated to use paths.ConfigDir() instead of hardcoded home directory path
fluid-cli/internal/store/sqlite/sqlite.go Updated to use paths.StateDB() for database path resolution
fluid-cli/internal/updater/updater.go Updated to use paths.ConfigDir() for cache directory
fluid-cli/cmd/fluid-cli/main.go Updated to use paths.ConfigFile() and revised help text to mention XDG
fluid-cli/.gitignore Removed fluid-cli entry (binary is actually bin/fluid, already ignored)
web/src/components/docs/api-endpoint-card.tsx Removed /v1 prefix prepending since paths now include it
web/src/content/blog/how-does-tunneling-work.mdx Minor content flow improvement
api/internal/rest/*_handlers.go Updated Swagger @Router annotations to include /v1 prefix
api/docs/openapi.yaml Updated server URL and all route paths to include /v1 prefix
api/cmd/server/main.go Updated @BasePath annotation from /v1 to /

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1 to +36
package paths

import (
"os"
"path/filepath"
"runtime"
)

// ConfigDir returns the fluid configuration directory.
//
// Resolution order:
// 1. $XDG_CONFIG_HOME/fluid (if set)
// 2. %AppData%/fluid (Windows)
// 3. ~/.config/fluid (macOS, Linux)
func ConfigDir() string {
if xdg := os.Getenv("XDG_CONFIG_HOME"); xdg != "" {
return filepath.Join(xdg, "fluid")
}
if runtime.GOOS == "windows" {
if dir, err := os.UserConfigDir(); err == nil {
return filepath.Join(dir, "fluid")
}
}
home, _ := os.UserHomeDir()
return filepath.Join(home, ".config", "fluid")
}

// ConfigFile returns the path to the config.yaml file.
func ConfigFile() string {
return filepath.Join(ConfigDir(), "config.yaml")
}

// StateDB returns the path to the SQLite state database.
func StateDB() string {
return filepath.Join(ConfigDir(), "state.db")
}

Copilot AI Feb 22, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new paths package lacks test coverage. Given that path resolution is critical for the application's ability to find its configuration and state, tests should be added to verify:

  1. XDG_CONFIG_HOME environment variable handling
  2. Platform-specific defaults (Windows vs Unix)
  3. Behavior when os.UserHomeDir() fails
  4. That ConfigFile() and StateDB() return correct paths relative to ConfigDir()

This is especially important since other parts of the codebase (like updater.go) check for empty string returns from ConfigDir(), suggesting there may be edge cases to handle.

Copilot uses AI. Check for mistakes.
Comment thread fluid-cli/internal/paths/paths.go Outdated
Comment on lines +24 to +25
home, _ := os.UserHomeDir()
return filepath.Join(home, ".config", "fluid")

Copilot AI Feb 22, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error from os.UserHomeDir() is silently ignored, which means that if it fails, the function will return .config/fluid (relative path) instead of an absolute path. This could lead to unexpected behavior where the config directory is created relative to the current working directory rather than a proper fallback location.

Consider handling this error explicitly, either by checking if home is empty and providing a better fallback, or by returning an error from ConfigDir() that callers can handle appropriately.

Suggested change
home, _ := os.UserHomeDir()
return filepath.Join(home, ".config", "fluid")
home, err := os.UserHomeDir()
if err == nil && home != "" {
return filepath.Join(home, ".config", "fluid")
}
// Fallback: use the system temporary directory to ensure an absolute path.
return filepath.Join(os.TempDir(), "fluid")

Copilot uses AI. Check for mistakes.

func init() {
rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", "config file (default ~/.fluid/config.yaml)")
rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", "config file (default $XDG_CONFIG_HOME/fluid/config.yaml)")

Copilot AI Feb 22, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The help text says "default $XDG_CONFIG_HOME/fluid/config.yaml", but this is not entirely accurate. The actual default depends on the platform and environment:

  • If XDG_CONFIG_HOME is set: $XDG_CONFIG_HOME/fluid/config.yaml
  • On Windows: %AppData%/fluid/config.yaml
  • On Unix (when XDG_CONFIG_HOME is not set): ~/.config/fluid/config.yaml

Consider updating the help text to be more accurate, such as: "config file (default: XDG config dir or ~/.config/fluid/config.yaml)" or similar wording that acknowledges the platform-specific behavior without being overly verbose.

Suggested change
rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", "config file (default $XDG_CONFIG_HOME/fluid/config.yaml)")
rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", "config file (default: XDG config dir or ~/.config/fluid/config.yaml)")

Copilot uses AI. Check for mistakes.
- Return (string, error) from all paths functions to surface failures
- Add DataDir() using $XDG_DATA_HOME for state.db, history, cache
- Add MaybeMigrate() to copy files from legacy ~/.fluid/ on first run
- Fix Windows fallback (return error instead of falling through to Unix)
- Fix stale ~/.fluid references in onboarding and comments
- Add comprehensive tests for paths and migration

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Feb 22, 2026

Copy link
Copy Markdown

PR Review: feat: use XDG base directories in fluid-cli

Overall this is a solid improvement — better cross-platform defaults, proper error handling where os.UserHomeDir() was silently ignored, and good test coverage. A few issues worth addressing before merging.


Bug: History file migration destination doesn't match where the app reads it

MaybeMigrate() copies history to the data dir (~/.local/share/fluid/history), but HistoryPath() in tui/history.go derives the history path from the config file location:

func HistoryPath(configPath string) string {
    return filepath.Join(filepath.Dir(configPath), "history")
}

After migration, configPath points to ~/.config/fluid/config.yaml, so the app will look for history at ~/.config/fluid/history — not where it was migrated. History will be silently lost.

Either:

  • Move history to configFiles in MaybeMigrate() so it lands in the config dir, OR
  • Update HistoryPath() to use paths.DataDir() directly and migrate history to the data dir (semantically more correct, since history is runtime data)

Partial migration is silent and unrecoverable

Errors from copyFile and copyDir are silently discarded:

for _, name := range configFiles {
    _ = copyFile(filepath.Join(oldDir, name), filepath.Join(configDir, name))
}

If the copy fails mid-migration (disk full, permissions, etc.), the new config dir will exist (migration created it) but files won't all be there. On the next startup, MaybeMigrate() will see the config dir exists and skip migration entirely, leaving the user stuck with a partial state and no explanation.

Suggested fix: collect errors, report them to stderr, and consider not creating the config dir until all copies succeed (or use a sentinel file like .migrated).


Migration not triggered in doctorCmd

MaybeMigrate() is called in runTUI() and runMCP(), but doctorCmd calls paths.ConfigFile() without migrating first. If a user's first interaction is fluid doctor, they'll get the new config path but their files won't have been migrated yet.


log.Printf in a TUI application

mustConfigDir() in config/config.go uses log.Printf for warnings:

log.Printf("Warning: could not determine config dir: %v", err)

log writes to stderr, which can corrupt TUI rendering. This should either use fmt.Fprintf(os.Stderr, ...) (same as the rest of the codebase) or be passed through the app's logging infrastructure.


Migration condition is too coarse

The migration check:

if _, err := os.Stat(configDir); err == nil {
    return nil // assume migration was done
}

...only checks the config dir. If the config dir exists but the data dir doesn't (e.g., XDG_DATA_HOME changed, or a fresh install created the config dir before migration ran), data files (state.db, history) won't be migrated. A sentinel file like ~/.config/fluid/.migrated-from-dot-fluid would be more reliable.


Unrelated changes bundled in this PR

The API documentation changes (@BasePath /v1@BasePath / and updating all route annotations) and the web component change in api-endpoint-card.tsx appear to be a separate fix unrelated to XDG paths. Bundling them makes the PR harder to review and reason about. Consider splitting these into a separate PR.


Minor: .gitignore removes fluid-cli exclusion

The removed line excluded a binary named fluid-cli from the fluid-cli directory root. The Makefile builds to bin/fluid so this is probably safe, but worth confirming no build script produces a fluid-cli binary at the package root.


What's good

  • The paths package design is clean and the XDG resolution order is correct
  • Error handling is properly added where os.UserHomeDir() was previously silently ignored
  • Non-destructive migration (keeping ~/.fluid) is the right UX decision
  • Test coverage is solid — TestMaybeMigrate_AlreadyMigrated and TestMaybeMigrate_FreshInstall cover the important edge cases
  • The XDG_CONFIG_HOME/XDG_DATA_HOME env var overrides make the behavior testable and configurable

🤖 Generated with Claude Code

- HistoryPath() now uses paths.DataDir() directly instead of deriving
  from configPath, fixing history silently lost after migration
- Use sentinel file (.migrated-from-dot-fluid) instead of config dir
  existence check, making partial migration failures retriable
- Collect and report copy errors to stderr instead of silently discarding
- Add MaybeMigrate() call to doctorCmd so first-run migration works
- Replace log.Printf with fmt.Fprintf in mustConfigDir() to avoid
  TUI corruption from timestamp-prefixed log output

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Feb 22, 2026

Copy link
Copy Markdown

Code Review

The XDG migration is well thought-out — non-destructive copy, sentinel file to avoid re-running, good test coverage of edge cases. A few issues worth addressing:


Bug: SQLite WAL files not included in migration

migrate.go copies state.db but not state.db-wal or state.db-shm:

dataFiles := []string{"state.db", "history"}

If the previous session was killed mid-transaction while SQLite's WAL mode was active, the WAL file holds uncommitted data that makes the main database file incomplete. Copying state.db alone in that case gives a corrupted database at the new location. The easiest fix is to add them:

dataFiles := []string{"state.db", "state.db-wal", "state.db-shm", "history"}

Missing files are already silently skipped (!os.IsNotExist(err)), so this is safe to add.


Code duplication: MaybeMigrate() called in three places

It's called identically in runTUI(), runMCP(), and doctorCmd.RunE. If a fourth entry point is added, it'll be easy to forget. A PersistentPreRunE on the root command runs it once regardless of subcommand:

rootCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
    if err := paths.MaybeMigrate(); err != nil {
        fmt.Fprintf(os.Stderr, "Warning: migration failed: %v\n", err)
    }
    return nil
}

Windows: DataDir() uses %APPDATA% (Roaming) instead of %LOCALAPPDATA%

// DataDir on Windows
dir, err := os.UserConfigDir() // returns %APPDATA% (Roaming)

os.UserConfigDir() on Windows returns %APPDATA%\Roaming, which is the right place for config that should sync across machines. Application data like state.db belongs in %LOCALAPPDATA% (local, non-roaming). You can get it via os.UserCacheDir() or os.Getenv("LOCALAPPDATA"). This only affects Windows users, but it's worth getting right.


copyDir doesn't handle directory symlinks

entry.IsDir() returns false for symlinks to directories. Those entries fall through to copyFile, which will call os.Open() on a directory — that succeeds on Linux but io.Copy will return an error, causing the whole directory migration to fail. Very unlikely in ~/.fluid in practice, but a defensive entry.Type() & fs.ModeSymlink != 0 check (skip or dereference explicitly) would be cleaner.


Unrelated changes bundled in the PR

The OpenAPI spec changes (@BasePath /v1@BasePath /, adding /v1/ prefix to every route annotation) and the api-endpoint-card.tsx change have nothing to do with XDG paths. The blog post edit in how-does-tunneling-work.mdx is unrelated as well. These mix review concerns and make the git history harder to bisect. Worth splitting into separate PRs.


Minor: migration failure message could be more actionable

Currently:

Warning: migration failed: 1 file(s) failed to copy

The individual file errors are printed to stderr immediately before this line, so the user gets a list. But they have no guidance on what to do next. Something like appending "You can manually copy files from ~/.fluid to %s" would help.


What's good

  • Sentinel file approach for idempotent migration is correct and handles partial failures cleanly
  • Error collection (not early-exit) in migration means partial copies still succeed for what they can
  • Non-destructive migration (preserving ~/.fluid) is the right UX choice
  • TestMaybeMigrate_RetriableAfterPartialFailure is a particularly good edge case test
  • Separating config/data into different XDG dirs is semantically correct — good that history moved to data dir
  • XDG_CONFIG_HOME/XDG_DATA_HOME env overrides make the behavior properly testable

🤖 Generated with Claude Code

- Include SQLite WAL/SHM sidecar files in migration to prevent DB corruption
- Deduplicate MaybeMigrate() calls via PersistentPreRunE on rootCmd
- Fix Windows DataDir to use %LOCALAPPDATA% instead of %APPDATA%
- Skip symlinks in copyDir to avoid io.Copy failures on Linux

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Feb 22, 2026

Copy link
Copy Markdown

Code Review

The previous review feedback has been addressed well: WAL files are now included in the migration, PersistentPreRunE consolidates the migration call, Windows correctly uses %LOCALAPPDATA%, and symlinks are skipped. A few remaining issues:


Stale comment in DataDir()

The godoc comment still says os.UserConfigDir()/fluid (Windows - same as config, standard practice) but the code was updated (correctly) to use %LOCALAPPDATA%:

// Resolution order:
//  1. $XDG_DATA_HOME/fluid (if set)
//  2. os.UserConfigDir()/fluid (Windows - same as config, standard practice)  ← wrong
//  3. ~/.local/share/fluid (macOS, Linux)

Should be:

//  2. %LOCALAPPDATA%/fluid (Windows)

Potential migration gap: ansible vs .ansible

The migration copies a directory named ansible from the legacy dir:

configDirs := []string{"ssh-ca", "sandbox-keys", "ansible"}

But fluid-cli/.gitignore ignores .ansible/ (with a leading dot). If users actually have ~/.fluid/.ansible/ rather than ~/.fluid/ansible/, this entry silently does nothing and their Ansible data isn't migrated. Worth confirming which name the runtime actually creates, and migrating both if there's ambiguity.


copyFile doesn't fsync before closing

out, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, info.Mode())
// ...
_, err = io.Copy(out, in)
return err

There's no out.Sync() before defer out.Close(). If the process crashes after the copy completes but before the sentinel is written, the destination file could be partially flushed (kernel buffer not yet on disk), while MaybeMigrate sees no sentinel and re-runs — this time overwriting with the partially-written version. Low probability, but for files like state.db it matters. Adding out.Sync() before returning is the fix:

if err = out.Sync(); err \!= nil {
    return err
}
return nil

Unrelated changes still bundled

The OpenAPI @BasePath refactor, the api-endpoint-card.tsx change, and the blog post edit are all unrelated to XDG paths. These are fine changes individually but bundling them makes the PR harder to bisect and review. Worth splitting for a cleaner git history.


What's good

  • All previously raised issues addressed: WAL files, PersistentPreRunE, %LOCALAPPDATA%, symlink skip
  • Retry-after-partial-failure via sentinel logic is correct
  • Separating config vs. data dirs (state.db in DataDir, history in DataDir) is semantically right
  • Test coverage is thorough, especially TestMaybeMigrate_RetriableAfterPartialFailure
  • Non-destructive migration with a clear user-facing message is the right UX

🤖 Generated with Claude Code

Ensures kernel buffers are flushed to disk after io.Copy, preventing
partially-written state.db if a crash occurs before the sentinel is written.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Feb 22, 2026

Copy link
Copy Markdown

Code Review

Two of the three issues from the previous review are still unresolved, and one was fixed. Here's the status:


Fixed: copyFile now syncs before returning

return out.Sync()

Good — this correctly protects against a partial-write leaving the destination in a bad state before the sentinel is written.


Still open: stale comment in DataDir() godoc

The implementation correctly uses %LOCALAPPDATA% on Windows, but the comment still says the old thing:

// Resolution order:
//  1. $XDG_DATA_HOME/fluid (if set)
//  2. os.UserConfigDir()/fluid (Windows - same as config, standard practice)  ← stale
//  3. ~/.local/share/fluid (macOS, Linux)

Should be:

//  2. %LOCALAPPDATA%/fluid (Windows)

Still open: ansible vs .ansible in migration

The migration copies ansible from the legacy dir:

configDirs := []string{"ssh-ca", "sandbox-keys", "ansible"}

But fluid-cli/.gitignore ignores .ansible/ (leading dot). If the runtime creates .ansible/ the migration silently skips it with no error. Worth confirming which name is actually written and either correcting the entry or migrating both names.


Still open: unrelated changes bundled

The OpenAPI @BasePath refactor, api-endpoint-card.tsx fix, and blog post edit are independent changes. They're all correct, but mixing them into a XDG paths PR makes the git history harder to bisect. Consider splitting in a follow-up if that matters to you — it's not a blocker.


Overall

This is close. The two concrete items (godoc comment and ansible path name) are small fixes. Once those are addressed this looks good to merge.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Feb 22, 2026

Copy link
Copy Markdown

Code Review

This is a well-structured change — XDG compliance is the right direction and the implementation is largely solid. The migration logic and test coverage are good. Here are the issues I found, roughly in order of severity.


Bug: Copying SQLite WAL files is unsafe

In migrate.go, the migration copies state.db-wal and state.db-shm alongside state.db:

dataFiles := []string{"state.db", "state.db-wal", "state.db-shm", "history"}

WAL and SHM files are only meaningful alongside their originating database connection. Copying all three with a file-level io.Copy (no database lock, no checkpoint) can produce a corrupted database at the destination if the source database had an open write transaction. The safe approach is to copy only state.db and omit the WAL files. SQLite will start a fresh WAL at the new location on first open.


PersistentPreRunE runs migration on every invocation, including --help

rootCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
    if err := paths.MaybeMigrate(); err != nil {
        fmt.Fprintf(os.Stderr, "Warning: migration failed: %v\n", err)
    }
    return nil
}

Two concerns:

  1. Running a filesystem check on every fluid --help or fluid --version is unnecessary overhead.
  2. If any subcommand sets its own PersistentPreRunE, this assignment silently replaces it. Cobra does not chain PersistentPreRunE — only the deepest-set one runs. The safer pattern is to call MaybeMigrate directly inside runTUI/runMCP where state is actually needed.

Double stderr output on migration failure

When copyErrors is non-empty, MaybeMigrate prints a per-file warning to stderr, then returns an error. The caller in main.go prints another warning on top. Users see duplicated output. Either collect errors silently and return them (letting the caller format), or print once inside MaybeMigrate and return nil.


mustConfigDir fallback is logically redundant

func mustConfigDir() string {
    dir, err := paths.ConfigDir()
    if err != nil {
        fmt.Fprintf(os.Stderr, "Warning: could not determine config dir: %v\n", err)
        home, _ := os.UserHomeDir()                          // same call that just failed
        return filepath.Join(home, ".config", "fluid")
    }
    return dir
}

paths.ConfigDir() only errors when os.UserHomeDir() fails (on non-Windows, without XDG_CONFIG_HOME). The fallback immediately calls os.UserHomeDir() again — the same call that just failed — and produces /.config/fluid. The fallback is effectively unreachable in practice. Simplify by removing it or replacing with a panic/fatal.


Minor: symlink skip in copyDir should have a comment

if entry.Type()&os.ModeSymlink != 0 {
    continue
}

This is a reasonable guard against symlink traversal, but it is not obvious from the code. A one-line comment clarifying the intent would help future readers.


Minor: onboarding message could show the resolved path

Error: "Add hosts to config.yaml (run 'fluid --help' for config location)",

Since the path is now resolved at runtime, calling paths.ConfigFile() here and displaying the actual path is more directly useful than asking the user to run --help.


API BasePath refactor looks correct

The @BasePath / + /v1/... route annotation change is consistent and the matching removal of the manual /v1 prepend in api-endpoint-card.tsx is correct. No issues here.


Paths package and tests are solid

The XDG resolution order is correct, tests use t.Setenv for proper isolation, the migration sentinel approach is clean, and the non-destructive "copy but don't delete" strategy is the right call for a first migration.

@claude

claude Bot commented Feb 22, 2026

Copy link
Copy Markdown

Code Review

The XDG migration approach is sound — migration sentinel, non-destructive copy, good test coverage for the main scenarios. The previous review captured the key issues well; here are a few additional points not yet mentioned.


Bug: copyFile overwrites destination on retry — potential data loss

copyFile uses O_CREATE|O_WRONLY|O_TRUNC, which silently overwrites an existing destination. If migration partially fails (no sentinel written), and the user has already modified their config at the new XDG location before the next run retries, the retry will overwrite their changes with the old file. Consider skipping files that already exist at the destination during retry:

out, err := os.OpenFile(dst, os.O_CREATE|os.O_EXCL|os.O_WRONLY, info.Mode())
if os.IsExist(err) {
    return nil // already present, skip
}

Tests will fail on Windows

The default-path tests assert Unix-specific suffixes without checking the OS:

// paths_test.go
if \!strings.HasSuffix(dir, filepath.Join(".config", "fluid")) { ... }
if \!strings.HasSuffix(dir, filepath.Join(".local", "share", "fluid")) { ... }

On Windows, ConfigDir() returns a path under %AppData% and DataDir() returns a path under %LocalAppData%. Both assertions will always fail there. Add if runtime.GOOS == "windows" { t.Skip(...) } or branch on GOOS in the assertion.


configPath parameter in NewModel may now be orphaned

Before this PR, configPath was passed to HistoryPath(configPath). After the change HistoryPath() takes no arguments. If configPath is no longer used anywhere else in NewModel, it is now an unused parameter — remove it and update all callers. (The build passing doesn't catch unused parameters in Go.)


Unrelated blog post changes

web/src/content/blog/how-does-tunneling-work.mdx has content edits (rephrasing paragraphs, removing "the battle-tested workhorse", etc.) that are unrelated to XDG path migration. These should be in a separate commit or PR to keep history readable.


Confirming the previous review's key points

The earlier review correctly identified:

  • WAL file copying is unsafe — omit state.db-wal and state.db-shm from the migration list
  • PersistentPreRunE runs on --help and silently clobbers any child-command PersistentPreRunE; call MaybeMigrate directly in runTUI/runMCP instead
  • Double stderr output when copy errors occur
  • mustConfigDir fallback is unreachable (calls os.UserHomeDir() after that same call just failed)

All four of those should be addressed before merging.


Overall

The core implementation (XDG resolution order, sentinel-based migration, separation of config vs data vs history) is well-designed. Fixing the WAL safety issue and the O_TRUNC overwrite-on-retry behavior are the most important correctness concerns. The rest are polish.

🤖 Generated with Claude Code

@aspectrr
aspectrr merged commit 4cc91eb into main Feb 22, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants