fix: isolate Claude hook installs#248
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThe PR improves Gradata's Claude hook lifecycle by centralizing settings file parsing, detecting and removing hook entries with missing BRAIN_DIR targets, integrating those checks into the doctor diagnostics system, and refining hook installation to avoid duplicate entries through exact-set matching rather than broad substring checks. ChangesHook lifecycle management and cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Gradata
force-pushed
the
gra-108-hook-test-isolation-dedupe
branch
from
6044f45 to
8597b76
Compare
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@Gradata/src/gradata/hooks/adapters/claude_code.py`:
- Around line 101-114: The _has_exact_installed_hook_set function currently only
verifies there's one Gradata module entry per (event,module) and that the
signature (sig) appears in it; update it to also validate that the configured
matcher equals the expected _matcher for that (event,module). After building
module_entries (the single matching entry), extract the matcher from
module_entries[0] (the same structure used by _is_gradata_module_hook) and
compare it to _matcher; if they differ, return False so install() won't treat a
wrong-matcher-but-same-id hook as already_present. Ensure you reference HOOKS,
_has_exact_installed_hook_set, _is_gradata_module_hook, module_entries, event,
module, sig and _matcher when making this check.
In `@Gradata/src/gradata/hooks/stale_hook_check.py`:
- Around line 200-207: The current _read_settings(path: Path) swallows all
read/parse errors and returns None, making callers like
_missing_hook_brain_dirs() and _remove_missing_hook_brain_dirs() treat
unreadable settings as "no hooks" — change _read_settings to only return None
when the file doesn't exist, but for read/parse failures catch specific
exceptions (json.JSONDecodeError, OSError) and either raise a small custom
exception (e.g., UnreadableSettingsError) or return a distinctive sentinel value
(e.g., {"__unreadable__": True}) so callers can differentiate this case; also
log a warning with logger.warning(...) including the path and exc_info=True to
avoid silent failures and reference the function name _read_settings as the
change location and update callers (_missing_hook_brain_dirs,
_remove_missing_hook_brain_dirs) to handle the new error/sentinel path
accordingly.
- Around line 210-215: The function _remove_missing_hook_brain_dirs should
respect the isolated-hook-root guard like _missing_hook_brain_dirs: detect if
either GRADATA_HOOK_ROOT or GRADATA_HOOK_ROOT_POST environment variables are set
and, if so, short-circuit and return an empty list instead of reading/mutating
the real Claude settings; update _remove_missing_hook_brain_dirs to perform this
check before calling _settings_path()/_read_settings() (and note that diagnose()
now calls _remove_missing_hook_brain_dirs by default, so the guard must run
early to prevent touching the real ~/.claude/settings.json).
- Around line 255-261: When removing hooks and persisting the updated settings
(the block that currently calls path.write_text(json.dumps(settings,...))),
replace the direct write with the repository's atomic JSON writer to avoid
truncation — e.g., call the project's atomic_write_json helper (or
write_json_atomic) to write the settings dict to path; keep the same logic
around settings, hooks, removed and path.parent.mkdir but route the final write
through the atomic helper instead of path.write_text.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e55104d-7e78-4181-912e-00f512d8f855
📒 Files selected for processing (5)
Gradata/src/gradata/_doctor.pyGradata/src/gradata/hooks/adapters/claude_code.pyGradata/src/gradata/hooks/stale_hook_check.pyGradata/tests/test_doctor_hooks.pyGradata/tests/test_hook_adapters.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: pytest windows-latest / py3.11
- GitHub Check: pytest ubuntu-latest / py3.11
- GitHub Check: pytest macos-latest / py3.12
- GitHub Check: pytest ubuntu-latest / py3.12
- GitHub Check: pytest macos-latest / py3.11
- GitHub Check: pytest windows-latest / py3.12
- GitHub Check: pytest (py3.11)
- GitHub Check: pytest (py3.12)
🧰 Additional context used
📓 Path-based instructions (2)
Gradata/tests/**/*.py
📄 CodeRabbit inference engine (Gradata/AGENTS.md)
Gradata/tests/**/*.py: SetBRAIN_DIRenvironment variable viatmp_pathin conftest.py for test isolation — ensure_paths.pymodule cache refreshes when callingBrain.init()directly inside tests
Add unit tests intests/test_*.pyfor every CI push without LLM calls (deterministic); mark integration tests with@pytest.mark.integrationand skip them by default (they hit real LLM APIs)
Files:
Gradata/tests/test_hook_adapters.pyGradata/tests/test_doctor_hooks.py
Gradata/src/**/*.py
📄 CodeRabbit inference engine (Gradata/AGENTS.md)
Gradata/src/**/*.py: Prefersentence-transformersfor local embeddings,google-genaifor Gemini embeddings,cryptographyfor AES-GCM encrypted system.db,bm25sfor BM25 rule ranking, andmem0aifor external memory adapters — guard all optional dependency imports withtry / except ImportErrorat the call site, never at module level
Maintain strict layering: Layer 0 (Primitives: _types.py, _db.py, _events.py, _paths.py, _file_lock.py; Patterns: contrib/patterns/) must never import from Layer 1 (Enhancements: enhancements/, rules/) or Layer 2 (Public API: brain.py, cli.py, daemon.py, mcp_server.py)
Never use bareexcept: pass— use typed exceptions or at minimumlogger.warning(...)withexc_info=Trueto avoid silent failure in a memory product
Never import from out-of-scope sibling directories../Sprites/or../Hausgem/withingradata/*code — that is a layering bug
Never leak private-sibling paths into public docs/code — no references to../Sprites/,../Hausgem/, email addresses, OneDrive paths, or Sprites-specific examples from insidegradata/*
Use atomic-write helper when writing JSON files to prevent corruption from mid-write crashes
Files:
Gradata/src/gradata/_doctor.pyGradata/src/gradata/hooks/adapters/claude_code.pyGradata/src/gradata/hooks/stale_hook_check.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: Gradata
Repo: Gradata/gradata PR: 0
File: :0-0
Timestamp: 2026-04-17T17:18:07.439Z
Learning: In PR `#102` (gradata/gradata), Round 2 addressed: cli.py env-first brain resolution (GRADATA_BRAIN > --brain-dir > cwd), _tenant.py corrupt .tenant_id overwrite, _env_int default clamping to minimum, and _events.py tenant-scoped fallback SELECT for dedup. All ruff and 99 tests green after these fixes.
📚 Learning: 2026-05-01T15:50:32.772Z
Learnt from: CR
Repo: Gradata/gradata PR: 0
File: Gradata/AGENTS.md:0-0
Timestamp: 2026-05-01T15:50:32.772Z
Learning: Applies to Gradata/tests/**/*.py : Set `BRAIN_DIR` environment variable via `tmp_path` in conftest.py for test isolation — ensure `_paths.py` module cache refreshes when calling `Brain.init()` directly inside tests
Applied to files:
Gradata/tests/test_hook_adapters.pyGradata/tests/test_doctor_hooks.pyGradata/src/gradata/hooks/stale_hook_check.py
📚 Learning: 2026-05-01T15:50:32.772Z
Learnt from: CR
Repo: Gradata/gradata PR: 0
File: Gradata/AGENTS.md:0-0
Timestamp: 2026-05-01T15:50:32.772Z
Learning: Use `from gradata import Brain` as the public entry point — `brain.correct()` is THE entry point for the headline product promise
Applied to files:
Gradata/src/gradata/hooks/stale_hook_check.py
🔇 Additional comments (2)
Gradata/tests/test_hook_adapters.py (1)
135-214: LGTM!Gradata/tests/test_doctor_hooks.py (1)
1-60: LGTM!
| 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: | ||
| return False | ||
| if f"{sig}:{event}:{module}" not in str(module_entries[0]): | ||
| return False | ||
| return True |
There was a problem hiding this comment.
Validate the configured matcher in the “exact set” check.
Right now this returns True as long as each (event, module) has one Gradata entry with the expected signature. If a group was edited to the wrong matcher but kept the same id, install() will incorrectly return already_present and never repair the hook behavior.
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:
+ 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:
return False
- if f"{sig}:{event}:{module}" not in str(module_entries[0]):
+ entry = module_entries[0]
+ if f"{sig}:{event}:{module}" not in str(entry):
return False
+ if not isinstance(entry, dict):
+ return False
+ if matcher is None:
+ if "matcher" in entry:
+ return False
+ elif entry.get("matcher") != matcher:
+ return False
return True🤖 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 `@Gradata/src/gradata/hooks/adapters/claude_code.py` around lines 101 - 114,
The _has_exact_installed_hook_set function currently only verifies there's one
Gradata module entry per (event,module) and that the signature (sig) appears in
it; update it to also validate that the configured matcher equals the expected
_matcher for that (event,module). After building module_entries (the single
matching entry), extract the matcher from module_entries[0] (the same structure
used by _is_gradata_module_hook) and compare it to _matcher; if they differ,
return False so install() won't treat a wrong-matcher-but-same-id hook as
already_present. Ensure you reference HOOKS, _has_exact_installed_hook_set,
_is_gradata_module_hook, module_entries, event, module, sig and _matcher when
making this check.
| 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 |
There was a problem hiding this comment.
Don't collapse unreadable settings.json into a clean state.
Returning None for every read/parse failure makes _missing_hook_brain_dirs() and _remove_missing_hook_brain_dirs() behave as if no hooks exist. A malformed or permission-denied Claude settings file will therefore suppress both the warning path and the doctor remediation path.
As per coding guidelines, "Never use bare except: pass — use typed exceptions or at minimum logger.warning(...) with exc_info=True to avoid silent failure in a memory product".
🤖 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 `@Gradata/src/gradata/hooks/stale_hook_check.py` around lines 200 - 207, The
current _read_settings(path: Path) swallows all read/parse errors and returns
None, making callers like _missing_hook_brain_dirs() and
_remove_missing_hook_brain_dirs() treat unreadable settings as "no hooks" —
change _read_settings to only return None when the file doesn't exist, but for
read/parse failures catch specific exceptions (json.JSONDecodeError, OSError)
and either raise a small custom exception (e.g., UnreadableSettingsError) or
return a distinctive sentinel value (e.g., {"__unreadable__": True}) so callers
can differentiate this case; also log a warning with logger.warning(...)
including the path and exc_info=True to avoid silent failures and reference the
function name _read_settings as the change location and update callers
(_missing_hook_brain_dirs, _remove_missing_hook_brain_dirs) to handle the new
error/sentinel path accordingly.
| 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 [] |
There was a problem hiding this comment.
Carry over the isolated-hook-root guard before mutating real Claude settings.
_missing_hook_brain_dirs() already short-circuits when GRADATA_HOOK_ROOT / GRADATA_HOOK_ROOT_POST are set so isolated test/dev runs do not touch the user's real ~/.claude/settings.json. _remove_missing_hook_brain_dirs() lacks that guard, and diagnose() now calls it by default, so the doctor path can still rewrite real user config in those isolated contexts.
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()
settings = _read_settings(path)
if not settings:
return []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 [] | |
| 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() | |
| settings = _read_settings(path) | |
| if not settings: | |
| return [] |
🤖 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 `@Gradata/src/gradata/hooks/stale_hook_check.py` around lines 210 - 215, The
function _remove_missing_hook_brain_dirs should respect the isolated-hook-root
guard like _missing_hook_brain_dirs: detect if either GRADATA_HOOK_ROOT or
GRADATA_HOOK_ROOT_POST environment variables are set and, if so, short-circuit
and return an empty list instead of reading/mutating the real Claude settings;
update _remove_missing_hook_brain_dirs to perform this check before calling
_settings_path()/_read_settings() (and note that diagnose() now calls
_remove_missing_hook_brain_dirs by default, so the guard must run early to
prevent touching the real ~/.claude/settings.json).
| 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") |
There was a problem hiding this comment.
Write settings.json atomically.
This direct write_text() can leave Claude's config truncated or partially rewritten on crash/interruption. Please route this through the repo's atomic JSON write helper instead of overwriting the file in place.
As per coding guidelines, "Use atomic-write helper when writing JSON files to prevent corruption from mid-write crashes".
🤖 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 `@Gradata/src/gradata/hooks/stale_hook_check.py` around lines 255 - 261, When
removing hooks and persisting the updated settings (the block that currently
calls path.write_text(json.dumps(settings,...))), replace the direct write with
the repository's atomic JSON writer to avoid truncation — e.g., call the
project's atomic_write_json helper (or write_json_atomic) to write the settings
dict to path; keep the same logic around settings, hooks, removed and
path.parent.mkdir but route the final write through the atomic helper instead of
path.write_text.
1 participant
Summary
Verification
python3 -m pytest Gradata/tests/test_hook_adapters.py Gradata/tests/test_doctor_hooks.py -q→ 19 passed/home/olive/.local/bin/uvx ruff check Gradata/src/gradata/_doctor.py Gradata/src/gradata/hooks/adapters/claude_code.py Gradata/src/gradata/hooks/stale_hook_check.py Gradata/tests/test_hook_adapters.py Gradata/tests/test_doctor_hooks.py→ All checks passed/home/olive/.local/bin/uvx ruff format --check Gradata/src/gradata/_doctor.py Gradata/src/gradata/hooks/adapters/claude_code.py Gradata/src/gradata/hooks/stale_hook_check.py Gradata/tests/test_hook_adapters.py Gradata/tests/test_doctor_hooks.py→ 5 files already formatted