fix: Use platform-native path separators in config.json #429
Conversation
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
- Add check_rclone_installed() helper function - Call check before all rclone operations (bisync, sync, check, ls) - Provide helpful error directing users to 'bm cloud setup' - Improve Windows installation error message with detailed steps - Add package manager installation instructions (winget, choco, scoop) Fixes #426
- Add tests for check_rclone_installed() function - Add tests verifying all command functions check for rclone - Update existing tests to mock is_rclone_installed - All 28 tests pass with 99% coverage for rclone_commands.py New tests: - test_check_rclone_installed_success - test_check_rclone_installed_not_found - test_project_sync_checks_rclone_installed - test_project_bisync_checks_rclone_installed - test_project_check_checks_rclone_installed - test_project_ls_checks_rclone_installed
Fixes #428 - Replace .as_posix() with str() for platform-native separators - Windows paths now use backslashes instead of forward slashes - Unix paths continue to use forward slashes - Add comprehensive tests for platform-specific path handling Changes: - config.py: Replace .as_posix() in 3 locations - test_config.py: Add 3 new tests for path separator behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Updated tests to compare Path objects instead of strings with hardcoded forward slashes. This fixes 10 failing tests on Windows CI: - tests/test_config.py (4 tests) - test_default_behavior_without_basic_memory_home - test_respects_basic_memory_home_environment_variable - test_model_post_init_respects_basic_memory_home_creates_main - test_basic_memory_home_with_relative_path - tests/services/test_project_service.py (4 tests) - test_project_operations_sync_methods - test_add_project_async - test_move_project - test_move_project_db_mismatch - tests/services/test_project_service_operations.py (1 test) - test_add_project_to_config - tests/api/test_project_router.py (1 test) - test_update_project_path_endpoint Changed: - Removed .as_posix() calls when creating test paths - Use Path() objects for comparison instead of string equality - Updated mkdir/os.makedirs calls to use Path.mkdir() This ensures tests work correctly with platform-native separators (backslashes on Windows, forward slashes on Unix). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Drew Cain <[email protected]>
groksrc
force-pushed
the
fix/windows-config-path-separators
branch
from
fbff5e2 to
5bb6715
Compare
…parison This test was failing on Windows CI because it was comparing string paths directly. After the config.py fix to use platform-native path separators, Windows correctly returns backslashes but the test expected forward slashes. Changed to compare Path objects which handles path separator differences correctly across platforms. Signed-off-by: Drew Cain <[email protected]>
jope-bm
reviewed
Nov 12, 2025
| projects: Dict[str, str] = Field( | ||
| default_factory=lambda: { | ||
| "main": Path(os.getenv("BASIC_MEMORY_HOME", Path.home() / "basic-memory")).as_posix() | ||
| "main": str(Path(os.getenv("BASIC_MEMORY_HOME", Path.home() / "basic-memory"))) |
Contributor
There was a problem hiding this comment.
is this still using a forward slash between Path.home() and "basic-memory"?
Contributor
Author
There was a problem hiding this comment.
When you use / between Path objects (or between a Path and a string), it creates a new path by joining them together. So Path.home() / "basic-memory" is equivalent to joining the user's home directory with the "basic-memory" subdirectory.
This is syntactic sugar that pathlib provides to make path construction more readable. It's roughly equivalent to:
pythonPath.home().joinpath("basic-memory")
phernandez
approved these changes
Nov 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Fixes #428
On Windows, the
config.jsonfile stores project paths using forward slashes (/) instead of platform-native backslashes (\). This causes issues when paths are used by Windows tools that expect backslash separators.Example issue on Windows:
{ "projects": { "main": "C:/Users/username/basic-memory" // ❌ Wrong on Windows } }Expected on Windows:
{ "projects": { "main": "C:\\Users\\username\\basic-memory" // ✅ Correct } }Root Cause
The code used
.as_posix()in three locations, which forces POSIX-style (Unix) forward slash separators regardless of platform:model_post_initmethodadd_projectmethodSolution
Replaced
.as_posix()withstr()to preserve platform-native path separators:\)/)Changes
src/basic_memory/config.py
.as_posix()str(path)for platform-native separatorstests/test_config.py
Testing
uv run pytest tests/test_config.py -v # 27 passed in 1.54sTest Coverage:
test_project_paths_use_platform_native_separators_in_config- Verifies saved config uses correct separatorstest_add_project_uses_platform_native_separators- Tests ConfigManager.add_project()test_model_post_init_uses_platform_native_separators- Tests default project creationImpact
This fix ensures:
Related