fix: use XDG-compliant paths for OpenCode user scope installations#155
Open
trevor-vaughan wants to merge 1 commit into
Open
fix: use XDG-compliant paths for OpenCode user scope installations#155trevor-vaughan wants to merge 1 commit into
trevor-vaughan wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces home-directory user paths with a platformdirs-backed user config directory: adds get_user_config_dir(), updates OpenCodeTarget to build user-scope paths from that directory, updates tests to validate custom/default/mocked bases, adds platformdirs dependency, and documents user vs project install scopes. ChangesUser config & OpenCode
sequenceDiagram
participant CLI
participant OpenCodeTarget
participant get_user_config_dir
participant Filesystem
CLI->>OpenCodeTarget: request user-scope paths
OpenCodeTarget->>get_user_config_dir: call get_user_config_dir()
get_user_config_dir->>get_user_config_dir: read _PLATFORM_DIRS.user_config_dir
get_user_config_dir-->>OpenCodeTarget: return Path(base)
OpenCodeTarget->>Filesystem: append "opencode/commands|agents|AGENTS.md|opencode.json"
OpenCodeTarget-->>CLI: return absolute paths
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lola/config.py (1)
34-36: ⚡ Quick winVerify XDG_CONFIG_HOME is an absolute path.
The XDG Base Directory Specification requires
$XDG_CONFIG_HOMEto be an absolute path. If a relative path is provided, the current implementation will use it as-is, which violates the spec and could lead to unexpected behavior.🛡️ Proposed fix to validate absolute path
def get_xdg_config_home() -> Path: """Get XDG config home directory. Returns $XDG_CONFIG_HOME if set, otherwise $HOME/.config """ xdg_config = os.environ.get("XDG_CONFIG_HOME") if xdg_config: - return Path(xdg_config) + path = Path(xdg_config) + if path.is_absolute(): + return path return Path.home() / ".config"As per coding guidelines, use Python 3.13 with modern type hints and pathlib for file operations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lola/config.py` around lines 34 - 36, Validate that the XDG_CONFIG_HOME value is an absolute path before returning it: when reading the xdg_config environment variable, convert it to a pathlib.Path and check .is_absolute(); only return Path(xdg_config) if it is absolute, otherwise treat it as unset and fall through to the existing fallback logic (do not return or use a relative path). Update the code around the xdg_config handling (the xdg_config variable and its return) to perform this check using pathlib.Path and modern type hints where appropriate.tests/test_opencode_target.py (1)
3-3: ⚡ Quick winRemove unused import.
The
osmodule is imported but never used in this file. All environment variable operations usemonkeypatchand all path operations usepathlib.Path.♻️ Remove unused import
-import os from pathlib import Path🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_opencode_target.py` at line 3, Remove the unused top-level import "import os" from the test module — the file only uses monkeypatch and pathlib.Path for env and path operations, so delete the "import os" line to eliminate the unused import warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lola/config.py`:
- Around line 34-36: Validate that the XDG_CONFIG_HOME value is an absolute path
before returning it: when reading the xdg_config environment variable, convert
it to a pathlib.Path and check .is_absolute(); only return Path(xdg_config) if
it is absolute, otherwise treat it as unset and fall through to the existing
fallback logic (do not return or use a relative path). Update the code around
the xdg_config handling (the xdg_config variable and its return) to perform this
check using pathlib.Path and modern type hints where appropriate.
In `@tests/test_opencode_target.py`:
- Line 3: Remove the unused top-level import "import os" from the test module —
the file only uses monkeypatch and pathlib.Path for env and path operations, so
delete the "import os" line to eliminate the unused import warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a8e3d48-d4c2-406d-8fa9-e3e942de8de1
📒 Files selected for processing (3)
src/lola/config.pysrc/lola/targets/opencode.pytests/test_opencode_target.py
trevor-vaughan
force-pushed
the
fix/opencode_paths
branch
from
440a4e4 to
4d06cca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lola/config.py`:
- Around line 29-37: get_xdg_config_home currently accepts any truthy
XDG_CONFIG_HOME (including blank/relative values); update it to only use
XDG_CONFIG_HOME if it's a non-empty, non-whitespace string and represents an
absolute path. In the get_xdg_config_home function, read
os.environ.get("XDG_CONFIG_HOME"), ensure you call .strip() to reject
whitespace-only values and then construct a Path and check .is_absolute(); if
both checks pass return that Path, otherwise fall back to Path.home() /
".config".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34280a46-4b1f-4405-b419-c5893efc00d6
📒 Files selected for processing (3)
src/lola/config.pysrc/lola/targets/opencode.pytests/test_opencode_target.py
trevor-vaughan
force-pushed
the
fix/opencode_paths
branch
from
4d06cca to
da93494
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lola/config.py (1)
8-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun Ruff formatting for this file before merge.
CI is currently failing
ruff format --checkforsrc/lola/config.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lola/config.py` around lines 8 - 41, CI fails ruff format for src/lola/config.py; run ruff's auto-formatter to fix style issues. Run `ruff format src/lola/config.py` (or `ruff format .`) and commit the resulting changes, making sure the module-level constants (LOLA_HOME, MODULES_DIR, INSTALLED_FILE, MARKET_DIR, CACHE_DIR, SKILL_FILE, MCPS_FILE, _PLATFORM_DIRS) and the get_user_config_dir function/docstring remain semantically identical after formatting; then re-run `ruff check` to confirm the file passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lola/config.py`:
- Around line 34-39: Update the docstring for the function that returns the user
configuration directory (the one whose docstring begins "Get user configuration
directory using platform conventions") to state that the function returns an
application-specific config path (it appends the app folder "opencode" to the
platform config root), and replace the generic platform root examples with
app-specific examples (e.g., Linux: ~/.config/opencode, macOS:
~/Library/Application Support/opencode, Windows: %APPDATA%\\opencode) so the
description matches the actual return value.
In `@tests/test_opencode_target.py`:
- Around line 4-159: The file tests/test_opencode_target.py fails ruff
formatting; run ruff format (or your project's pre-commit formatter) on that
file to fix style issues and re-run checks. Open tests/test_opencode_target.py
(containing tests like test_get_user_config_dir_with_xdg_env_set, the
reload_config fixture, and the parameterized test_opencode_paths_cross_platform)
and apply ruff format --fix or ruff format tests/test_opencode_target.py, then
commit the reformatted file so Ruff's --check will pass in CI.
---
Outside diff comments:
In `@src/lola/config.py`:
- Around line 8-41: CI fails ruff format for src/lola/config.py; run ruff's
auto-formatter to fix style issues. Run `ruff format src/lola/config.py` (or
`ruff format .`) and commit the resulting changes, making sure the module-level
constants (LOLA_HOME, MODULES_DIR, INSTALLED_FILE, MARKET_DIR, CACHE_DIR,
SKILL_FILE, MCPS_FILE, _PLATFORM_DIRS) and the get_user_config_dir
function/docstring remain semantically identical after formatting; then re-run
`ruff check` to confirm the file passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83751c98-a30f-4631-8188-ce68d2db1f88
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
README.mdpyproject.tomlsrc/lola/config.pysrc/lola/targets/opencode.pytests/test_opencode_target.py
✅ Files skipped from review due to trivial changes (1)
- README.md
Comment on lines
+34
to
+39
| """Get user configuration directory using platform conventions. | ||
|
|
||
| Returns platform-appropriate config directory: | ||
| - Linux/Unix: ~/.config | ||
| - macOS: ~/Library/Application Support | ||
| - Windows: %APPDATA% |
There was a problem hiding this comment.
Correct the return-path description in this docstring.
The function returns an app-specific directory (includes opencode), but the examples currently describe only parent config roots.
Suggested docstring fix
def get_user_config_dir() -> Path:
"""Get user configuration directory using platform conventions.
-
- Returns platform-appropriate config directory:
- - Linux/Unix: ~/.config
- - macOS: ~/Library/Application Support
- - Windows: %APPDATA%
+
+ Returns platform-appropriate app config directory for OpenCode:
+ - Linux/Unix: ~/.config/opencode
+ - macOS: ~/Library/Application Support/opencode
+ - Windows: %APPDATA%\\opencode
"""
return Path(_PLATFORM_DIRS.user_config_dir)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lola/config.py` around lines 34 - 39, Update the docstring for the
function that returns the user configuration directory (the one whose docstring
begins "Get user configuration directory using platform conventions") to state
that the function returns an application-specific config path (it appends the
app folder "opencode" to the platform config root), and replace the generic
platform root examples with app-specific examples (e.g., Linux:
~/.config/opencode, macOS: ~/Library/Application Support/opencode, Windows:
%APPDATA%\\opencode) so the description matches the actual return value.
da93494 to
6f9954e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
requirements.txt (1)
1-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegenerate exported requirements artifacts to clear CI blocker.
CI is currently failing because
requirements.txt/requirements-dev.txtare not in sync with lock data. Please re-export and commit both generated files.Regeneration commands
uv export --frozen --no-dev --output-file=requirements.txt uv export --frozen --output-file=requirements-dev.txtAlso applies to: 32-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@requirements.txt` around lines 1 - 3, CI is failing because the committed requirements artifacts are out of sync with the lockfile; re-export both dependency artifacts and commit them: run "uv export --frozen --no-dev --output-file=requirements.txt" to regenerate requirements.txt and "uv export --frozen --output-file=requirements-dev.txt" to regenerate requirements-dev.txt (as suggested in the review), verify the diffs, and commit the updated requirements.txt and requirements-dev.txt so the repo and CI use the updated lock data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_opencode_target.py`:
- Line 100: The tests use string endswith with forward slashes
(str(path).endswith("opencode/commands")), which breaks on Windows; update the
assertions to be path-separator-agnostic by using Path semantics (e.g., compare
tail segments via path.parts or construct a Path for the suffix). Replace the
current endswith assertion on the variable path with something like
tuple(path.parts[-2:]) == ("opencode", "commands") (and similarly for the other
three occurrences) so the tests pass on all platforms.
---
Outside diff comments:
In `@requirements.txt`:
- Around line 1-3: CI is failing because the committed requirements artifacts
are out of sync with the lockfile; re-export both dependency artifacts and
commit them: run "uv export --frozen --no-dev --output-file=requirements.txt" to
regenerate requirements.txt and "uv export --frozen
--output-file=requirements-dev.txt" to regenerate requirements-dev.txt (as
suggested in the review), verify the diffs, and commit the updated
requirements.txt and requirements-dev.txt so the repo and CI use the updated
lock data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1c26724-30ee-47e9-9e92-aef8ef8b525c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
README.mdpyproject.tomlrequirements.txtsrc/lola/config.pysrc/lola/targets/opencode.pytests/test_opencode_target.py
✅ Files skipped from review due to trivial changes (2)
- pyproject.toml
- README.md
| path = target.get_command_path("/home/user/project", "user") | ||
| assert path == Path.home() / ".opencode" / "commands" | ||
| # Should use platformdirs default - ends with opencode/commands | ||
| assert str(path).endswith("opencode/commands") |
There was a problem hiding this comment.
Make platform-default path assertions separator-agnostic.
Using str(path).endswith("opencode/...") is fragile on Windows. Line 100, Line 111, Line 124, and Line 135 should assert with Path semantics instead of hardcoded /.
Suggested test-safe assertion update
- assert str(path).endswith("opencode/commands")
+ assert path.parts[-2:] == ("opencode", "commands")
@@
- assert str(path).endswith("opencode/agents")
+ assert path.parts[-2:] == ("opencode", "agents")
@@
- assert str(path).endswith("opencode/AGENTS.md")
+ assert path.parts[-2:] == ("opencode", "AGENTS.md")
@@
- assert str(path).endswith("opencode/opencode.json")
+ assert path.parts[-2:] == ("opencode", "opencode.json")Also applies to: 111-111, 124-124, 135-135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_opencode_target.py` at line 100, The tests use string endswith
with forward slashes (str(path).endswith("opencode/commands")), which breaks on
Windows; update the assertions to be path-separator-agnostic by using Path
semantics (e.g., compare tail segments via path.parts or construct a Path for
the suffix). Replace the current endswith assertion on the variable path with
something like tuple(path.parts[-2:]) == ("opencode", "commands") (and similarly
for the other three occurrences) so the tests pass on all platforms.
6f9954e to
42daaec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@requirements.txt`:
- Around line 32-35: The CI is failing because exported requirements are stale;
re-run the dependency export/update step to regenerate the pinned lockfiles so
requirements.txt (this file) and requirements-dev.txt match the current
pyproject.toml, then commit the updated files; ensure you run the same export
command used by the repo’s check-requirements job (or the project’s export
script) so platformdirs and other hashes are recalculated and the
check-requirements CI job passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f0d3813-e58a-4d79-baca-23372ebb707e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
README.mdpyproject.tomlrequirements-dev.txtrequirements.txtsrc/lola/config.pysrc/lola/targets/opencode.pytests/test_opencode_target.py
✅ Files skipped from review due to trivial changes (2)
- README.md
- requirements-dev.txt
trevor-vaughan
force-pushed
the
fix/opencode_paths
branch
from
42daaec to
9245c40
Compare
Replace hardcoded ~/.opencode/ paths with platform-native config directories. User-scope installations now land in ~/.config/opencode/ on Linux, ~/Library/Application Support/opencode/ on macOS, and %APPDATA%\opencode\ on Windows. - Add platformdirs>=4.0.0 dependency and get_user_config_dir() helper in config.py - Update OpenCodeTarget path methods to use platformdirs for user scope - Fix mod rm not passing scope to get_skill_path(), which made user-scope skill uninstalls target the wrong directory - Rewrite test_opencode_target.py with XDG override, platform default, and cross-platform parametrized cases - Add test verifying mod rm forwards scope correctly Details: Build/project config: - Add build, test, install, uninstall targets to Makefile - Fix hatch-vcs git_describe_command to match only v*.*.* tags Documentation: - Document --scope user flag and platform-specific paths in README - Add Installation Scopes section with per-platform path table Fixes: LobsterTrap#153 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
trevor-vaughan
force-pushed
the
fix/opencode_paths
branch
from
9245c40 to
3f619f9
Compare
mrbrandao
added
documentation
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
2 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.
Changes:
Fixes: #153
Checklist
pytest)ruff check src tests)ty check)AI Disclosure
AI Usage - Claude Sonnet 4.5 noreply@anthropic.com
Summary by CodeRabbit
New Features
Documentation
Tests