test: cover stale Claude hook pruning#271
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.
|
Warning Review limit reached
More reviews will be available in 23 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA regression test is added to ChangesClaude Code Hook Pruning Test
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@Gradata/tests/test_hook_adapters.py`:
- Around line 319-326: The test builds the expected hook command string manually
which doesn't account for quoting used by hook_command(), so replace the manual
expected value with a call to hook_command(...) to match implementation;
specifically, in the loop over stale_by_event (and where you collect
hook["command"] from settings["hooks"][event]), assert commands ==
[hook_command(brain_dir, module)] (import or reference the existing hook_command
function) so both brain_dir and sys.executable are quoted the same way as
production.
🪄 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: 3cb9b738-778f-4d64-8e68-24d56c5e2f38
📒 Files selected for processing (1)
Gradata/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 macos-latest / py3.11
- GitHub Check: pytest windows-latest / py3.11
- GitHub Check: pytest (py3.11)
- GitHub Check: pytest ubuntu-latest / py3.12
- GitHub Check: pytest ubuntu-latest / py3.11
- GitHub Check: pytest macos-latest / py3.12
- GitHub Check: pytest windows-latest / py3.12
- GitHub Check: pytest (py3.12)
🧰 Additional context used
📓 Path-based instructions (1)
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.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.py
📚 Learning: 2026-04-17T17:18:07.439Z
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.
Applied to files:
Gradata/tests/test_hook_adapters.py
🔇 Additional comments (2)
Gradata/tests/test_hook_adapters.py (2)
5-5: LGTM!
265-318: LGTM!
| for event, (_matcher, module) in stale_by_event.items(): | ||
| commands = [ | ||
| hook["command"] | ||
| for group in settings["hooks"][event] | ||
| for hook in group["hooks"] | ||
| if f"gradata.hooks.{module}" in hook["command"] | ||
| ] | ||
| assert commands == [f"BRAIN_DIR={brain_dir} {sys.executable} -m gradata.hooks.{module}"] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use hook_command() for assertion to match implementation quoting.
The assertion constructs the expected command manually, but hook_command() (from context snippet 1) uses shlex.quote() for both brain_dir and sys.executable. If paths contain special characters (spaces, quotes), this assertion will fail even when the code is correct.
♻️ Refactor to use hook_command
+from gradata.hooks.adapters._base import AGENTS, adapter_config_path, get_adapter, hook_command
-from gradata.hooks.adapters._base import AGENTS, adapter_config_path, get_adapter for event, (_matcher, module) in stale_by_event.items():
commands = [
hook["command"]
for group in settings["hooks"][event]
for hook in group["hooks"]
if f"gradata.hooks.{module}" in hook["command"]
]
- assert commands == [f"BRAIN_DIR={brain_dir} {sys.executable} -m gradata.hooks.{module}"]
+ assert commands == [hook_command(brain_dir, module)]🤖 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/tests/test_hook_adapters.py` around lines 319 - 326, The test builds
the expected hook command string manually which doesn't account for quoting used
by hook_command(), so replace the manual expected value with a call to
hook_command(...) to match implementation; specifically, in the loop over
stale_by_event (and where you collect hook["command"] from
settings["hooks"][event]), assert commands == [hook_command(brain_dir, module)]
(import or reference the existing hook_command function) so both brain_dir and
sys.executable are quoted the same way as production.
Gradata
force-pushed
the
gra-2052-prune-stale-hooks
branch
from
7ee0f2a to
a816dc3
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.
1 participant
Summary
Verification
env -u BRAIN_DIR -u GRADATA_BRAIN python3 -m pytest tests/test_hook_adapters.py -q→ 22 passedenv -u BRAIN_DIR -u GRADATA_BRAIN python3 -m pytest tests/test_cli_install_agent.py tests/test_hook_adapters.py -q→ 31 passed, 1 skippedgit diff --check→ passed/home/olive/.local/bin/uvx ruff check tests/test_hook_adapters.py→ All checks passed