fix(guard): honor client config home overrides - #444
Open
lbeurerkellner wants to merge 3 commits into
Open
Conversation
PR Summary by QodoFix guard/discovery to resolve client config dirs at runtime (CLAUDE_CONFIG_DIR/CODEX_HOME)
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Relative override path writes
|
![qodo-merge-etso[bot]](https://avatars.githubusercontent.com/u/12959162?s=60&v=4){kind=small}
Comment on lines
+23
to
+26
| value = os.environ.get(env_var) | ||
| if not value: | ||
| return None | ||
| return Path(value).expanduser() |
There was a problem hiding this comment.
1. Relative override path writes 🐞 Bug ⛨ Security
If CLAUDE_CONFIG_DIR/CODEX_HOME is set to a relative path, resolve_user_client_dir() returns it as-is and guard builds config/script paths relative to the current working directory, causing hook install/uninstall/status to read/write the wrong location. This can create or delete files in unexpected directories and makes behavior dependent on the process CWD.
Agent Prompt
### Issue description
`user_client_dir_override()` returns `Path(value).expanduser()` without ensuring the result is absolute. When the env var is set to a relative path (e.g. `CODEX_HOME=custom-codex`), downstream code (notably `guard._config_path()` and `_copy_hook_script()`) will create directories and write files relative to the current working directory.
### Issue Context
This PR newly makes guard honor `CLAUDE_CONFIG_DIR` and `CODEX_HOME` via `resolve_user_client_dir()`, so the relative-path edge case now affects guard install/uninstall/status behavior.
### Fix Focus Areas
- src/agent_scan/client_paths.py[15-52]
- src/agent_scan/guard.py[1195-1206]
### Suggested fix
- In `user_client_dir_override()` (or in `resolve_user_client_dir()` right after reading `configured`), normalize the path:
- `p = Path(value).expanduser()`
- If `not p.is_absolute()`: either
- raise a `ValueError` with a clear message requiring an absolute path, **or**
- treat it as relative to the scanning user’s home (e.g. `p = (Path.home() / p)`), then return `p`.
- Add a unit test for guard ensuring a relative `CODEX_HOME`/`CLAUDE_CONFIG_DIR` is rejected or anchored deterministically (depending on chosen behavior).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+44
to
+48
| _CLIENT_CONFIG_FILENAMES = { | ||
| "claude": "settings.json", | ||
| "cursor": "hooks.json", | ||
| "codex": "hooks.json", | ||
| } |
There was a problem hiding this comment.
2. Client path metadata duplication 🐞 Bug ⚙ Maintainability
Client directory/default metadata now exists in multiple places (_CLIENT_DIR_SETTINGS and guard’s _CLIENT_CONFIG_FILENAMES), which can drift and cause runtime ValueError/KeyError if a client is added/renamed in only one mapping. This increases maintenance risk for future client additions or path changes.
Agent Prompt
### Issue description
Client path configuration is split across multiple dicts:
- `client_paths._CLIENT_DIR_SETTINGS` (env var + default dir)
- `guard._CLIENT_CONFIG_FILENAMES` (config filename)
This duplication can drift, causing failures when one mapping is updated without the other.
### Issue Context
This PR introduced `client_paths.py` specifically to centralize runtime directory resolution, but guard still carries a separate client mapping for filenames.
### Fix Focus Areas
- src/agent_scan/client_paths.py[8-52]
- src/agent_scan/guard.py[44-48]
### Suggested fix
- Create a single source of truth (e.g. a `ClientPathSpec` dataclass) in `client_paths.py` that includes env var, default dir, and (optionally) config filename.
- Import and use that spec in guard (and optionally in discoverers) to avoid parallel mappings.
- Add a small assertion test ensuring all clients supported by guard are present in the shared spec (and vice versa, if appropriate).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Summary
Resolve Claude and Codex user configuration directories at runtime, honoring
CLAUDE_CONFIG_DIRandCODEX_HOMEfor discovery and guard setup when they are the only installed location.Enforcement safety
When both an overridden directory and the standard client directory exist, guard installation now prefers the standard location so an environment override cannot redirect
hooksConfiguredenforcement into an unused directory. Added end-to-end guard tests for both clients and both path-selection cases.Validation
The full guard unit suite passes (242 passed, 11 skipped), along with mypy, Ruff, formatting, and repository pre-commit hooks.