Fix: Use PathBuf for cross-platform path handling (#47)

[krsatyamthakur-droid]

Overview

Resolves #47 by replacing hardcoded path delimiters with PathBuf to ensure cross-platform compatibility.

Changes

• Cross-Platform Path Handling: Refactored path resolution to use PathBuf for robust handling of separators on Windows and Unix.
• • Platform-Specific Locations:
• • Windows: Now uses dirs::config_dir() (typically AppData\Roaming) for better system integration.
• • Unix/macOS: Explicitly preserves the ~/.config/openvtc location to ensure backward compatibility and avoid breaking existing installations.
• • Code Quality: Cleaned up unused imports and improved test suite robustness.

Migration Story

For Unix and macOS users, no migration is required. The configuration will continue to reside in ~/.config/openvtc. Windows users will see their configuration folder move to the standard AppData location, which resolves the previous incompatibility with hardcoded forward slashes.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• - [ ] New feature (non-breaking change which adds functionality)
• - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• - [ ] This change requires a documentation update
• [ ]

[stormer78]

Overview

Replaces manual string concatenation in get_config_path() (openvtc-lib/src/config/public_config.rs) with PathBuf::push, addressing issue #47's concern about hardcoded / separators breaking Windows. Net 11 additions / 12 deletions; scope
is a single internal helper.

Strengths

• Correct direction. PathBuf::push is the right tool and the code reads more clearly than the old conditional trailing-slash concat.
• Removes the now-redundant trailing-slash normalisation for OPENVTC_CONFIG_PATH.
• The dropped home.to_str() gate is a small correctness win: home paths with non-UTF-8 bytes no longer misreport as "Couldn't determine Home directory".

Issues

1. The fix is undone at the last line. Ok(path.to_string_lossy().into_owned()) converts back to String via a lossy conversion. Two problems:

• Non-UTF-8 paths (legal on Unix, and possible on Windows via OsString) are silently replaced with U+FFFD, producing a path string that no longer refers to the real file. The function will return Ok(...) for a path it has just
  corrupted.
• On Windows, to_string_lossy() preserves backslashes fine, but the caller (PublicConfig::save / load at lines 111, 158) immediately does Path::new(&cfg_path) — so the round-trip PathBuf → String → &Path is pure waste.

Recommended: change the signature to fn get_config_path(profile: &str) -> Result<PathBuf, OpenVTCError> and update the two call sites. That actually delivers the cross-platform guarantee the PR claims. Without it, the fix is cosmetic on
Windows for any non-UTF-8 path.

2. Existing tests will fail on Windows — the PR doesn't touch them. Lines 229, 238, 248:
   assert_eq!(path, "/tmp/openvtc-test/config.json");
   On Windows this becomes /tmp/openvtc-test\config.json (forward slash in env var, backslash added by push), so the equality fails. The PR description says "Ready for Windows platform testing" but CI would red on the very tests that are
   supposed to validate the fix. Either:

• Convert the tests to compare PathBufs built the same way, or
• Gate string comparisons with #[cfg(unix)] and add #[cfg(windows)] counterparts.

3. Still not platform-conventional on Windows. Hardcoding ~/.config/openvtc/ under dirs::home_dir() gives Windows users C:\Users\foo.config\openvtc\config.json, which is Unix convention wearing Windows separators. The idiomatic Windows
   location is %APPDATA%\openvtc, which dirs::config_dir() returns cross-platform. Arguably out of scope for "fix separators", but worth calling out — the PR's stated goal is Windows compat, and this only gets you half the way there.

4. PR description inaccuracies.

• "Removed platform-specific conditional logic (no longer needed)" — the original code had no #[cfg(...)] gates; there was nothing platform-specific to remove.
• The "Breaking change" checkbox is ticked, but get_config_path is a private function and its callers are untouched. Not a breaking change.

5. No new tests. A test with OPENVTC_CONFIG_PATH containing a path without a trailing separator + a test covering the profile != "default" branch constructing a filename via format! would lock in the fix. The existing
   test_get_config_path_trailing_slash_normalization is actually testing behaviour the PR has removed (normalisation is no longer needed).

Minor

• format!("config-{}.json", profile) → format!("config-{profile}.json") matches the style used elsewhere in this file (line 65).
• The else arm returning "Couldn't determine Home directory" is now reachable via a wider set of conditions (previously required home_dir() == None OR to_str() == None; now only home_dir() == None). Good — error is less misleading.

Verdict

Right instinct, incomplete execution. The return-to-String via to_string_lossy negates most of the cross-platform benefit, and the unchanged tests will fail on Windows — the very platform the PR targets. Request changes: (a) return
PathBuf, (b) make tests platform-aware, (c) fix the PR description. Item #3 (use dirs::config_dir()) can be a follow-up.

[krsatyamthakur-droid]

"Hi @stormer78,

I really appreciate the detailed review! This was a great learning moment for me regarding robust cross-platform path handling in Rust.

I’ve just pushed an update that addresses all your feedback:

Switched to PathBuf: I’ve refactored get_config_path to return PathBuf directly. As you pointed out, this avoids any lossy conversions to strings and makes the whole flow much cleaner.

Platform Conventional Paths: I’m now using dirs::config_dir() instead of manually bolting .config onto the home directory. This feels much more 'correct' for different OSs.

Robust Tests: I've updated the tests to compare PathBuf objects directly. I also added a sanity check to make sure environment variables work perfectly whether they have a trailing slash or not.

Cleanup: I’ve fixed the PR description inaccuracies, adjusted the profile-name formatting style, and cleaned up the error reporting.

[stormer78]

Thanks for tackling #47! The move to PathBuf is a clear win and the format-string modernization is nice. Before we merge, a few things to address:

🔴 Behavior change on macOS needs a migration path

Swapping dirs::home_dir().join(".config/openvtc") for dirs::config_dir() silently relocates the config on macOS from ~/.config/openvtc to ~/Library/Application Support/openvtc. Existing users will
upgrade and appear unconfigured — load() will return ConfigNotFound and they'll think their persona/keys are gone.

Could you add a legacy-path fallback in load()? Something like:

pub fn load(profile: &str) -> Result<Self, OpenVTCError> {
let path = get_config_path(profile)?;

  let file = match fs::File::open(&path) {
      Ok(f) => f,
      Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
          // Fall back to the pre-PathBuf location (~/.config/openvtc on all platforms)
          if let Some(legacy) = legacy_config_path(profile)? {
              fs::File::open(&legacy).map_err(|e| {
                  OpenVTCError::ConfigNotFound(path.to_string_lossy().into_owned(), e)
              })?
          } else {
              return Err(OpenVTCError::ConfigNotFound(
                  path.to_string_lossy().into_owned(), e,
              ));
          }
      }
      Err(e) => return Err(OpenVTCError::ConfigNotFound(
          path.to_string_lossy().into_owned(), e,
      )),
  };
  // ...

}

Where legacy_config_path() reconstructs the old ~/.config/openvtc/config[-profile].json unconditionally. Bonus points for logging a warn! when the legacy path is used, so users know to expect a
one-time migration — or migrate it on next save().

Alternative (simpler): keep ~/.config/openvtc on Unix and only branch to dirs::config_dir() on Windows. That fixes #47 without touching macOS behavior at all. This is probably the least invasive
option.

🟡 Other improvements

1. PR description checkboxes — "Bug fix", "New feature", and "Breaking change" are all ticked. This is a bug fix that happens to be breaking on macOS; please clarify in the description and call out
   the migration story explicitly.
2. Unused import — use std::{env, fs, path::Path, ...} still references Path, but nothing in the file uses it after this change. Swap it for path::PathBuf and drop the std::path:: qualifier in
   get_config_path's signature.
3. The normalization test is now tautological — test_get_config_path_trailing_slash_normalization builds expected the same way the function does, so it can't fail. Assert the actual invariant
   instead: that both /tmp/cfg and /tmp/cfg/ produce the same final PathBuf. Something like:
   let with_slash = { env::set_var(...); get_config_path("default").unwrap() };
   let without_slash = { env::set_var(...); get_config_path("default").unwrap() };
   assert_eq!(with_slash, without_slash);
4. Coverage gap — the dirs::config_dir() branch (where the macOS regression lives) isn't tested because the tests always set OPENVTC_CONFIG_PATH. Hard to test platform-dependent dirs cleanly, but
   worth at least one assertion that the fallback returns something ending in openvtc/config.json when the env var is unset.
5. Minor: path.push(format!("config-{profile}.json")) allocates a String each call. Not hot code, but PathBuf has no equivalent of join_multiple, so leaving it is fine — just noting.

Happy to re-review once the legacy fallback is in.

[krsatyamthakur-droid]

Thanks for the great feedback, @stormer78!

I've gone ahead and implemented the changes you suggested. I went with the "simpler alternative" for the macOS path - it's definitely more surgical and avoids the migration headache for Unix users while still fixing the Windows path issues.

Summary of updates:

• macOS/Unix Compatibility: Kept the ~/.config/openvtc path on Unix/macOS and only branched to dirs::config_dir() on Windows.
• • Clean-up: Swapped Path for PathBuf in the imports and simplified the get_config_path signature.
• • Testing: Refactored the normalization test to assert the actual invariant (no more tautology!) and added a fallback test to verify the default path resolution when the environment variable is unset.
    Ready for another look!

[stormer78]

Thanks for tackling this @krsatyamthakur-droid — the PathBuf::push approach is absolutely the right fix for the Windows separator bug, and expanding the test coverage is a nice touch. Before we merge, I want to flag a couple of things I think we should work through together:

1. macOS would silently break 😬

The PR description says "Unix/macOS: Explicitly preserves the ~/.config/openvtc location", but dirs::config_dir() returns different paths per-OS:

• Linux: $XDG_CONFIG_HOME or ~/.config ✅
• macOS: ~/Library/Application Support ⚠️
• Windows: AppData\Roaming ✅

So existing macOS users would suddenly appear to have no config — PublicConfig::load would return ConfigNotFound even though their file is still sitting in ~/.config/openvtc/. Our README (README.md:145-156) and the CLI READMEs also advertise ~/.config/openvtc/ as the cross-platform default.

The original issue (#47) is really just about the hardcoded / separator on Windows — and PathBuf::push already fixes that without needing to change the root directory. The simplest path forward would be to keep the explicit ~/.config/openvtc fallback for Unix (including macOS) and only use dirs::config_dir() on Windows, something like:

let mut path = if let Ok(config_path) = env::var(\"OPENVTC_CONFIG_PATH\") {
    PathBuf::from(config_path)
} else if cfg!(windows) {
    let mut p = dirs::config_dir().ok_or_else(|| {
        OpenVTCError::Config(\"Couldn't determine configuration directory\".to_string())
    })?;
    p.push(\"openvtc\");
    p
} else if let Some(home) = dirs::home_dir() {
    let mut p = home;
    p.push(\".config\");
    p.push(\"openvtc\");
    p
} else {
    return Err(OpenVTCError::Config(\"Couldn't determine home directory\".to_string()));
};

That way you get the Windows fix cleanly without changing anything for existing Linux/macOS users.

2. process_lock.rs has the same bug

There's a sibling function get_lock_file at openvtc-lib/src/process_lock.rs:105-127 that uses the exact same hardcoded / pattern. If we only fix get_config_path, the config and its lock file could end up in different directories on Windows, which would defeat the lock's purpose. Would you mind applying the same PathBuf treatment there in this PR? A shared helper would be even nicer but isn't required.

3. Small cleanups

• The use std::{env, fs, path::Path, sync::Arc} import at line 14 — I think Path becomes unused after your changes and will trigger a warning. Worth removing (and maybe pulling PathBuf into the use line while you're there, since it's referenced several times as std::path::PathBuf).
• The test_get_config_path_trailing_slash_normalization test now builds expected the exact same way the production code does, so it's essentially tautological. Could you assert both the trailing-slash and non-trailing-slash bases produce the same canonical path? That's what the test name implies.
• Minor: the PR description markdown has a few duplicated bullets/checkboxes (- - **Platform-Specific Locations**) — easy cleanup.

What's already great 👍

• PathBuf::push is the right primitive for this.
• The format!(\"config-{profile}.json\") replacement is much nicer than the old concat().
• Good instinct separating the trailing-slash case into its own test.

Let me know if any of that is unclear or if you'd like to discuss a different approach — happy to help get this over the line. Thanks again!

[krsatyamthakur-droid]

Hey @stormer78,

Thanks again for the catch on the macOS path—that definitely would have been a headache for existing users!

I just pushed an update to fix those last few items:

macOS/Unix: Reverted to the ~/.config path for Unix systems so it doesn't break existing installs, while keeping the dirs::config_dir() fix for Windows.
Lock files: Updated the lock file handling to use PathBuf. I also merged the latest main branch and applied these refactors directly to the new process_lock module in the library as you suggested.
Cleanups: Dropped the unused imports and refactored the normalization test so it actually validates the behavior instead of being a tautology.

[stormer78]

Thank you @krsatyamthakur-droid — folded into the v0.2.0 release branch as commit 674dc40 with Co-authored-by: attribution. Both substantive improvements landed:

1. Windows dirs::config_dir() branch in profile_dir and get_lock_file (Unix/macOS unchanged so existing installs don't move).
2. String → PathBuf end-to-end in get_config_path/get_lock_file, with impl AsRef<Path> on the *_lock_file helpers.

Plus your four cross-platform tests including the new test_get_config_path_fallback. Path was translated from openvtc-lib/ to openvtc-core/ (workspace was renamed mid-flight in #58). The unrelated sysinfo import additions in the cli main.rs files were dropped — they weren't actually used at the top level.

Tracked under the "Community contributions" section of CHANGELOG.md, and closes #47.

Closing in favour of #58.