-
Notifications
You must be signed in to change notification settings - Fork 0
fix: isolate claude hook installs + dedupe on reinstall (GRA-1233) #269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,16 +163,15 @@ def _extract_brain_dir_from_command(command: str) -> Path | None: | |
|
|
||
| def _missing_hook_brain_dirs(settings_path: Path | None = None) -> list[Path]: | ||
| """Find Gradata hook BRAIN_DIR targets in Claude settings that no longer exist.""" | ||
| if settings_path is None and (env_str("GRADATA_HOOK_ROOT") or env_str("GRADATA_HOOK_ROOT_POST")): | ||
| if settings_path is None and ( | ||
| env_str("GRADATA_HOOK_ROOT") or env_str("GRADATA_HOOK_ROOT_POST") | ||
| ): | ||
| # Test/dev hook roots are intentionally isolated; do not let a developer's | ||
| # real ~/.claude/settings.json leak warnings into those runs. | ||
| return [] | ||
| path = settings_path or _settings_path() | ||
| if not path.is_file(): | ||
| return [] | ||
| try: | ||
| settings = json.loads(path.read_text(encoding="utf-8")) | ||
| except Exception: | ||
| settings = _read_settings(path) | ||
| if not settings: | ||
| return [] | ||
| hooks = settings.get("hooks", {}) | ||
| if not isinstance(hooks, dict): | ||
|
|
@@ -198,6 +197,71 @@ def _missing_hook_brain_dirs(settings_path: Path | None = None) -> list[Path]: | |
| return missing | ||
|
|
||
|
|
||
| def _read_settings(path: Path) -> dict | None: | ||
| if not path.is_file(): | ||
| return None | ||
| try: | ||
| settings = json.loads(path.read_text(encoding="utf-8")) | ||
| except Exception: | ||
| return None | ||
| return settings if isinstance(settings, dict) else None | ||
|
|
||
|
|
||
| def _remove_missing_hook_brain_dirs(settings_path: Path | None = None) -> list[Path]: | ||
| """Remove Gradata hook commands whose BRAIN_DIR targets no longer exist.""" | ||
| path = settings_path or _settings_path() | ||
| settings = _read_settings(path) | ||
| if not settings: | ||
| return [] | ||
| hooks = settings.get("hooks") | ||
|
Comment on lines
+210
to
+216
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preserve hook-root isolation in the removal path. Line 210 starts a destructive cleanup path, but unlike Line 166 it does not short-circuit when Suggested fix def _remove_missing_hook_brain_dirs(settings_path: Path | None = None) -> list[Path]:
"""Remove Gradata hook commands whose BRAIN_DIR targets no longer exist."""
+ if settings_path is None and (
+ env_str("GRADATA_HOOK_ROOT") or env_str("GRADATA_HOOK_ROOT_POST")
+ ):
+ return []
path = settings_path or _settings_path()🤖 Prompt for AI Agents |
||
| if not isinstance(hooks, dict): | ||
| return [] | ||
| removed: list[Path] = [] | ||
| seen: set[str] = set() | ||
| for event in list(hooks): | ||
| groups = hooks.get(event) | ||
| if not isinstance(groups, list): | ||
| continue | ||
| kept_groups: list = [] | ||
| for group in groups: | ||
| if not isinstance(group, dict): | ||
| kept_groups.append(group) | ||
| continue | ||
| group_hooks = group.get("hooks") | ||
| if not isinstance(group_hooks, list): | ||
| kept_groups.append(group) | ||
| continue | ||
| kept_hook_entries: list = [] | ||
| for hook in group_hooks: | ||
| if not isinstance(hook, dict): | ||
| kept_hook_entries.append(hook) | ||
| continue | ||
| brain_dir = _extract_brain_dir_from_command(str(hook.get("command", ""))) | ||
| if brain_dir is None or brain_dir.exists(): | ||
| kept_hook_entries.append(hook) | ||
| continue | ||
| key = brain_dir.as_posix() | ||
| if key not in seen: | ||
| removed.append(brain_dir) | ||
| seen.add(key) | ||
| if kept_hook_entries: | ||
| new_group = dict(group) | ||
| new_group["hooks"] = kept_hook_entries | ||
| kept_groups.append(new_group) | ||
| if kept_groups: | ||
| hooks[event] = kept_groups | ||
| else: | ||
| hooks.pop(event, None) | ||
| if removed: | ||
| if hooks: | ||
| settings["hooks"] = hooks | ||
| else: | ||
| settings.pop("hooks", None) | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| path.write_text(json.dumps(settings, indent=2, sort_keys=True) + "\n", encoding="utf-8") | ||
| return removed | ||
|
Comment on lines
+260
to
+262
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the atomic JSON write helper for settings persistence. Line 261 writes As per coding guidelines, “Use atomic-write helper when writing JSON files to prevent corruption from mid-write crashes.” 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
|
|
||
| def _hook_dirs() -> list[Path]: | ||
| pre = os.environ.get("GRADATA_HOOK_ROOT") or ".claude/hooks/pre-tool/generated" | ||
| post = os.environ.get("GRADATA_HOOK_ROOT_POST") or ".claude/hooks/post-tool/generated" | ||
|
|
@@ -280,7 +344,9 @@ def main() -> int: | |
| for path in missing_brain_dirs: | ||
| print(f" - missing BRAIN_DIR: {path}") | ||
| target_brain = env_str("GRADATA_BRAIN") or env_str("BRAIN_DIR") or "~/.gradata/brain" | ||
| print(f" fix: gradata install --agent claude-code --brain {shlex.quote(target_brain)}") | ||
| print( | ||
| f" fix: gradata install --agent claude-code --brain {shlex.quote(target_brain)}" | ||
| ) | ||
| print() | ||
|
|
||
| return 0 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| from pathlib import Path | ||
|
|
||
| from gradata import _doctor | ||
|
|
||
|
|
||
| def test_doctor_warns_and_removes_missing_gradata_hook_brain_dirs( | ||
| tmp_path: Path, monkeypatch | ||
| ) -> None: | ||
| home = tmp_path / "home" | ||
| monkeypatch.setenv("HOME", str(home)) | ||
| monkeypatch.setenv("USERPROFILE", str(home)) | ||
| missing_brain = tmp_path / "pytest-55" / "test_hook_adapter_install_is_i0" / "brain" | ||
| live_brain = tmp_path / "live-brain" | ||
| live_brain.mkdir() | ||
| settings_path = home / ".claude" / "settings.json" | ||
| settings_path.parent.mkdir(parents=True) | ||
| settings_path.write_text( | ||
| json.dumps( | ||
| { | ||
| "hooks": { | ||
| "PreToolUse": [ | ||
| { | ||
| "matcher": "*", | ||
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": f"BRAIN_DIR={missing_brain} python -m gradata.hooks.inject_brain_rules", | ||
| } | ||
| ], | ||
| }, | ||
| { | ||
| "matcher": "*", | ||
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": f"BRAIN_DIR={live_brain} python -m gradata.hooks.inject_brain_rules", | ||
| } | ||
| ], | ||
| }, | ||
| ] | ||
| } | ||
| } | ||
| ), | ||
| encoding="utf-8", | ||
| ) | ||
|
|
||
| check = _doctor._check_missing_hook_brain_dirs(auto_remove=True) | ||
|
|
||
| assert check["status"] == "warn" | ||
| assert str(missing_brain) in check["detail"] | ||
| settings = json.loads(settings_path.read_text(encoding="utf-8")) | ||
| commands = [ | ||
| hook["command"] for group in settings["hooks"]["PreToolUse"] for hook in group["hooks"] | ||
| ] | ||
| assert all(str(missing_brain) not in command for command in commands) | ||
| assert any(str(live_brain) in command for command in commands) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_has_exact_installed_hook_set()can miss duplicates inside a single group.Line 107 builds
module_entriesfrom top-level event entries. If one group contains twogradata.hooks.<module>commands, it still counts as one entry and Line 110 passes, soinstall()returnsalready_presentand skips dedupe.Suggested fix
def _has_exact_installed_hook_set(hooks: dict, sig: str) -> bool: """Return true only when every Gradata module hook is the desired one.""" for event, _matcher, module in HOOKS: entries = hooks.get(event) if not isinstance(entries, list): return False - module_entries = [ - entry for entry in entries if _is_gradata_module_hook(entry, event, module) - ] - if len(module_entries) != 1: + module_hooks: list[object] = [] + for entry in entries: + if isinstance(entry, dict) and isinstance(entry.get("hooks"), list): + module_hooks.extend( + hook + for hook in entry["hooks"] + if _is_gradata_module_hook(hook, event, module) + ) + elif _is_gradata_module_hook(entry, event, module): + module_hooks.append(entry) + if len(module_hooks) != 1: return False - if f"{sig}:{event}:{module}" not in str(module_entries[0]): + if f"{sig}:{event}:{module}" not in str(module_hooks[0]): return False return True🤖 Prompt for AI Agents