Skip to content

test: snapshot Claude Code install hooks#239

Open
Gradata wants to merge 2 commits into
mainfrom
gra-1211-claude-code-snapshot
Open

test: snapshot Claude Code install hooks#239
Gradata wants to merge 2 commits into
mainfrom
gra-1211-claude-code-snapshot

Conversation

@Gradata

@Gradata Gradata commented Jun 1, 2026

Copy link
Copy Markdown
Owner

Closes Paperclip GRA-1211.\n\nAdds a snapshot test for gradata install --agent claude-code hook coverage and updates the Claude Code adapter install/uninstall wiring so SDK install covers the lifecycles previously split across SDK/plugin behavior.\n\nVerification:\n- python3 -m pytest tests/test_install_claude_code_snapshot.py tests/test_hook_adapters.py -v → 12 passed\n\nNotes:\n- Claude Code uses the Stop hook lifecycle for session-end behavior; the snapshot pins Stopgradata.hooks.session_close.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough
  • Snapshot test suite added for gradata install --agent claude-code to pin expected Claude Code lifecycle hook configuration (GRA-1211); includes idempotency, existing-settings preservation, and documentation-compliance checks
  • Claude Code adapter expanded to wire multiple lifecycles: PreToolUse, PostToolUse, Stop (mapped to gradata.hooks.session_close), PreCompact, and UserPromptSubmit
  • New command helper functions in gradata.hooks.adapters._base: auto_correct_command(), session_close_command(), pre_compact_command(), context_inject_command(), plus existing hook_command() used for inject_brain_rules
  • Install logic refactored so SDK install covers all required lifecycles and returns "already_present" only when every phase is configured
  • Uninstall logic updated to remove signature-matching entries across all wired lifecycles and prune empty lifecycle blocks and the hooks container
  • Tests verify idempotency and that user-provided hooks are preserved when installing Gradata hooks
  • Snapshot file added: tests/snapshots/install_claude_code_settings.json (normalized for cross-platform runs)
  • Test result: 12 tests passing locally with the new snapshot and adapter changes
  • Breaking changes: none
  • Security fixes: none
  • New public API surface: the added command helper functions are exposed module-level helpers in gradata.hooks.adapters._base

Walkthrough

This PR extends Claude Code hook installation to manage five distinct lifecycle phases (PreToolUse, PostToolUse, Stop, PreCompact, UserPromptSubmit) by introducing command-builder helpers, rewriting the adapter's install/uninstall logic to check all lifecycles and conditionally append/remove entries while preserving user hooks, and validating the behavior with snapshot equality, idempotency, and coverage tests.

Changes

Claude Code Multi-Lifecycle Hook Wiring

Layer / File(s) Summary
Command Helper Functions
Gradata/src/gradata/hooks/adapters/_base.py
Four new functions (auto_correct_command, session_close_command, pre_compact_command, context_inject_command) build parameterized BRAIN_DIR=... <sys.executable> -m gradata.hooks.* command strings for invoking Gradata hook modules.
Claude Code Multi-Lifecycle Adapter
Gradata/src/gradata/hooks/adapters/claude_code.py
Import of new command helpers; install() rewritten to manage PreToolUse, PostToolUse, Stop, PreCompact, UserPromptSubmit with cross-lifecycle signature detection and conditional appending; uninstall() generalized to remove matching entries across all lifecycles while pruning empty keys.
Test Snapshot and Comprehensive Validation
Gradata/tests/snapshots/install_claude_code_settings.json, Gradata/tests/test_install_claude_code_snapshot.py
Snapshot defines expected multi-lifecycle hooks configuration; test suite includes snapshot equality assertion, idempotency validation, user-hook preservation check, and lifecycle coverage verification via helper assertions and deterministic normalization.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • Gradata/gradata#248: Modifies claude_code adapter hook install/update behavior and overlaps with this PR's multi-phase hook wiring changes.
  • Gradata/gradata#53: Adds mappings for Claude Code hook modules similar to the multi-lifecycle hook commands introduced here.
  • Gradata/gradata#215: Previously added uninstall helpers and signature handling for Claude Code hooks that intersect this PR's install/uninstall logic.

Suggested labels

feature

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 52.63% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'test: snapshot Claude Code install hooks' directly and accurately summarizes the primary change: adding a snapshot test for Claude Code hook installation verification.
Description check ✅ Passed The PR description comprehensively relates to the changeset, explaining the snapshot test addition, hook lifecycle updates, verification steps, and relevant implementation notes about the Stop lifecycle.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch gra-1211-claude-code-snapshot
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch gra-1211-claude-code-snapshot

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 OpenGrep (1.22.0)

OpenGrep fatal error (exit code 2):
┌──────────────┐
│ Opengrep CLI │
└──────────────┘

�[32m✔�[39m �[1mOpengrep OSS�[0m
�[32m✔�[39m Basic security coverage for first-party code vulnerabilities.

�[1m Loading rules from local config...�[0m
[00.16][ERROR]: Error: exception Glob.Lexer.Syntax_error("malformed glob pattern: missing ']'")
Raised at Glob__Lexer.syntax_error in file "libs/glob/Lexer.mll", line 8, characters 2-26
Called from Glob__Lexer.__ocaml_lex_token_rec in file "libs/glob/Lexer.mll", line 29, characters 26-53
Cal


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot added the feature label Jun 1, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (2)
Gradata/src/gradata/hooks/adapters/claude_code.py (1)

148-153: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Stale uninstall() docstring. It still describes single-lifecycle behavior ("drop the signature-matching PreToolUse entry", "User-owned PreToolUse entries"), but the function now sweeps all five lifecycles.

📝 Suggested wording
-    """Reverse ``install()``: drop the signature-matching PreToolUse entry.
+    """Reverse ``install()``: drop signature-matching entries across all
+    managed lifecycles (PreToolUse, PostToolUse, Stop, PreCompact, UserPromptSubmit).
 
     Idempotent — calling on an already-clean config returns ``already_present``
     (semantically: 'already in the desired absent state'). Empty containers
-    are pruned. User-owned PreToolUse entries (without our signature) are
-    preserved verbatim.
+    are pruned. User-owned entries (without our signature) are preserved verbatim.
     """
🤖 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 148 - 153,
Update the stale uninstall() docstring to reflect current behavior: state that
uninstall() now sweeps all five lifecycles (not just a single lifecycle) and
removes signature-matching PreToolUse entries across those lifecycle containers;
keep that it is idempotent (returns the "already_present"/absent-state when
nothing to remove), note that empty lifecycle containers are pruned, and clarify
that PreToolUse entries without our signature (user-owned) are preserved
unchanged; reference the function name uninstall() and the PreToolUse concept in
the text.
Gradata/src/gradata/hooks/adapters/_base.py (1)

132-164: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Collapse the five near-identical command builders into one parameterized helper.

hook_command, auto_correct_command, session_close_command, pre_compact_command, and context_inject_command differ only in the trailing module name. A single private builder removes the copy/paste and keeps the BRAIN_DIR=.../shlex.quote wiring in one place.

♻️ Proposed refactor
+def _module_command(brain_dir: Path, module: str) -> str:
+    return (
+        f"BRAIN_DIR={shlex.quote(str(brain_dir))} "
+        f"{shlex.quote(sys.executable)} -m gradata.hooks.{module}"
+    )
+
+
 def hook_command(brain_dir: Path) -> str:
-    return (
-        f"BRAIN_DIR={shlex.quote(str(brain_dir))} "
-        f"{shlex.quote(sys.executable)} -m gradata.hooks.inject_brain_rules"
-    )
+    return _module_command(brain_dir, "inject_brain_rules")


 def auto_correct_command(brain_dir: Path) -> str:
-    return (
-        f"BRAIN_DIR={shlex.quote(str(brain_dir))} "
-        f"{shlex.quote(sys.executable)} -m gradata.hooks.auto_correct"
-    )
+    return _module_command(brain_dir, "auto_correct")


 def session_close_command(brain_dir: Path) -> str:
-    return (
-        f"BRAIN_DIR={shlex.quote(str(brain_dir))} "
-        f"{shlex.quote(sys.executable)} -m gradata.hooks.session_close"
-    )
+    return _module_command(brain_dir, "session_close")


 def pre_compact_command(brain_dir: Path) -> str:
-    return (
-        f"BRAIN_DIR={shlex.quote(str(brain_dir))} "
-        f"{shlex.quote(sys.executable)} -m gradata.hooks.pre_compact"
-    )
+    return _module_command(brain_dir, "pre_compact")


 def context_inject_command(brain_dir: Path) -> str:
-    return (
-        f"BRAIN_DIR={shlex.quote(str(brain_dir))} "
-        f"{shlex.quote(sys.executable)} -m gradata.hooks.context_inject"
-    )
+    return _module_command(brain_dir, "context_inject")
🤖 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/_base.py` around lines 132 - 164, The five
near-identical builders (hook_command, auto_correct_command,
session_close_command, pre_compact_command, context_inject_command) should be
collapsed into a single private helper (e.g. _make_command(module_name: str,
brain_dir: Path) or _command_for(module: str, brain_dir: Path)) that constructs
the string using shlex.quote(str(brain_dir)) and shlex.quote(sys.executable) -m
gradata.hooks.{module}; then have each public function simply call that helper
with their respective module name to preserve existing function names/API.
Ensure the helper lives in the same module and that all existing function names
are retained as thin wrappers calling the helper.
🤖 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 91-103: The PostToolUse matcher currently appended to post_tool
uses "Edit|Write" which misses MultiEdit; update the matcher string in the block
that appends to post_tool (the branch guarded by has_post_tool) to include
"MultiEdit" (e.g., "Edit|Write|MultiEdit") so Claude Code auto-correction
triggers for multi-edit operations, and regenerate/update any Claude Code
hook/settings snapshot that pins this matcher string; the change affects the
block that calls auto_correct_command(brain_dir) and uses the id sig.

In `@Gradata/tests/test_install_claude_code_snapshot.py`:
- Around line 96-104: The normalization currently hardcodes "/tmp" and fails on
non-Linux temp dirs; update the test to normalize using the actual brain/temp
directory (thread the test's tmp_path/brain value through
_assert_matches_snapshot or compute brain_dir = str(tmp_path / "brain")) and use
that value in the two re.sub calls (escape it with re.escape) instead of the
literal "/tmp"; modify the patterns that touch serialized (the two re.sub calls
operating on serialized and the hook signature replacement) to replace
occurrences of the resolved brain_dir with "__BRAIN_DIR__" so snapshots are
portable across platforms.

---

Outside diff comments:
In `@Gradata/src/gradata/hooks/adapters/_base.py`:
- Around line 132-164: The five near-identical builders (hook_command,
auto_correct_command, session_close_command, pre_compact_command,
context_inject_command) should be collapsed into a single private helper (e.g.
_make_command(module_name: str, brain_dir: Path) or _command_for(module: str,
brain_dir: Path)) that constructs the string using shlex.quote(str(brain_dir))
and shlex.quote(sys.executable) -m gradata.hooks.{module}; then have each public
function simply call that helper with their respective module name to preserve
existing function names/API. Ensure the helper lives in the same module and that
all existing function names are retained as thin wrappers calling the helper.

In `@Gradata/src/gradata/hooks/adapters/claude_code.py`:
- Around line 148-153: Update the stale uninstall() docstring to reflect current
behavior: state that uninstall() now sweeps all five lifecycles (not just a
single lifecycle) and removes signature-matching PreToolUse entries across those
lifecycle containers; keep that it is idempotent (returns the
"already_present"/absent-state when nothing to remove), note that empty
lifecycle containers are pruned, and clarify that PreToolUse entries without our
signature (user-owned) are preserved unchanged; reference the function name
uninstall() and the PreToolUse concept in the 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: b5729f21-7514-4bf3-9555-641150e96a57

📥 Commits

Reviewing files that changed from the base of the PR and between a197bff and 06b2a4e.

📒 Files selected for processing (4)
  • Gradata/src/gradata/hooks/adapters/_base.py
  • Gradata/src/gradata/hooks/adapters/claude_code.py
  • Gradata/tests/snapshots/install_claude_code_settings.json
  • Gradata/tests/test_install_claude_code_snapshot.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 (py3.12)
  • GitHub Check: pytest (py3.11)
  • GitHub Check: pytest windows-latest / py3.12
  • GitHub Check: pytest ubuntu-latest / py3.11
  • GitHub Check: pytest ubuntu-latest / py3.12
  • GitHub Check: pytest macos-latest / py3.11
  • GitHub Check: pytest macos-latest / py3.12
  • GitHub Check: pytest windows-latest / py3.11
🧰 Additional context used
📓 Path-based instructions (2)
Gradata/src/**/*.py

📄 CodeRabbit inference engine (Gradata/AGENTS.md)

Gradata/src/**/*.py: Prefer sentence-transformers for local embeddings, google-genai for Gemini embeddings, cryptography for AES-GCM encrypted system.db, bm25s for BM25 rule ranking, and mem0ai for external memory adapters — guard all optional dependency imports with try / except ImportError at 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 bare except: pass — use typed exceptions or at minimum logger.warning(...) with exc_info=True to avoid silent failure in a memory product
Never import from out-of-scope sibling directories ../Sprites/ or ../Hausgem/ within gradata/* 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 inside gradata/*
Use atomic-write helper when writing JSON files to prevent corruption from mid-write crashes

Files:

  • Gradata/src/gradata/hooks/adapters/claude_code.py
  • Gradata/src/gradata/hooks/adapters/_base.py
Gradata/tests/**/*.py

📄 CodeRabbit inference engine (Gradata/AGENTS.md)

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
Add unit tests in tests/test_*.py for every CI push without LLM calls (deterministic); mark integration tests with @pytest.mark.integration and skip them by default (they hit real LLM APIs)

Files:

  • Gradata/tests/test_install_claude_code_snapshot.py
🔇 Additional comments (3)
Gradata/tests/snapshots/install_claude_code_settings.json (1)

3-14: The "Edit|Write" matcher pinned here is downstream of the PostToolUse matcher issue flagged in claude_code.py; update this snapshot together with that fix. Otherwise the snapshot faithfully mirrors adapter output.

Gradata/tests/test_install_claude_code_snapshot.py (1)

123-147: LGTM!

Gradata/src/gradata/hooks/adapters/claude_code.py (1)

78-140: ⚡ Quick win

Avoid relying on an undocumented per-hook id field for signature matching/uninstall in Claude Code.

In Gradata/src/gradata/hooks/adapters/claude_code.py (lines 78-140), the signature gradata:claude-code:<brain_dir> is stored only in an id key on each hook entry. Claude Code’s published hooks/settings schema does not define a user-configurable per-hook id, so validation/normalization could ignore/drop it; that would break your has_* checks and uninstall() matching and can leave duplicate/orphaned hooks. Embed the signature in a field Claude Code preserves (e.g., in the command string, or via a documented matcher like if) and treat id as best-effort.

Comment on lines +91 to +103
if not has_post_tool:
post_tool.append(
{
"matcher": "Edit|Write",
"hooks": [
{
"type": "command",
"command": auto_correct_command(brain_dir),
"id": sig,
}
],
}
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Claude Code PostToolUse matcher regex - does "Edit|Write" match the MultiEdit tool or must MultiEdit be listed explicitly

💡 Result:

In Claude Code, the "Edit|Write" matcher does not automatically include "MultiEdit" [1][2]. If you want your hook to trigger for MultiEdit operations, you must list it explicitly in the matcher, such as "Edit|Write|MultiEdit" [3][4][2]. While the matcher field supports regular expressions [5][6], users have frequently reported that relying on simplified patterns like "Edit|Write" can lead to unexpected behavior where only some tools are matched [1][7]. To ensure consistent execution across all relevant file-editing tools, explicitly including each desired tool name—specifically "Edit", "Write", and "MultiEdit"—is the recommended best practice [4][2].

Citations:


Fix Claude Code PostToolUse matcher to trigger on MultiEdit
PostToolUse uses matcher "Edit|Write", and Claude Code does not treat that pattern as implicitly including MultiEdit—so auto-correction won’t run for multi-edit operations unless MultiEdit is listed explicitly.

🐛 Proposed fix
-                    "matcher": "Edit|Write",
+                    "matcher": "Edit|MultiEdit|Write",

Update any corresponding Claude Code hook/settings snapshot that pins this matcher string as well.

🤖 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 91 - 103, The
PostToolUse matcher currently appended to post_tool uses "Edit|Write" which
misses MultiEdit; update the matcher string in the block that appends to
post_tool (the branch guarded by has_post_tool) to include "MultiEdit" (e.g.,
"Edit|Write|MultiEdit") so Claude Code auto-correction triggers for multi-edit
operations, and regenerate/update any Claude Code hook/settings snapshot that
pins this matcher string; the change affects the block that calls
auto_correct_command(brain_dir) and uses the id sig.

Comment on lines +96 to +104
serialized = json.dumps(settings, indent=2, sort_keys=True)
# Normalize: BRAIN_DIR=/tmp/pytest-N/.../brain → BRAIN_DIR=__BRAIN_DIR__
serialized = re.sub(r"BRAIN_DIR=/tmp/[^ ]+/brain", "BRAIN_DIR=__BRAIN_DIR__", serialized)
# Normalize: hook signature ID
serialized = re.sub(
r'"gradata:claude-code:/tmp/[^"]+brain"',
'"gradata:claude-code:__BRAIN_DIR__"',
serialized,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Normalization hardcodes /tmp, so the snapshot test fails off Linux.

tmp_path lives under tempfile.gettempdir(), which is not /tmp on macOS (typically /var/folders/..., and Path.resolve() yields /private/var/...). The two re.sub patterns anchored on /tmp/ won't match there, leaving raw temp paths in the serialized output and breaking the snapshot equality assertion on developer machines. Normalize on the actual brain/temp dir instead of a literal prefix.

🛠️ More portable normalization
-def _normalized_snapshot(settings: dict) -> str:
+def _normalized_snapshot(settings: dict, brain_dir: Path | None = None) -> str:
     """Return normalized settings.json snapshot text.

     Brain-directory paths are normalized to a stable ``__BRAIN_DIR__``
     placeholder so the snapshot file doesn't change on every test run
     (tmp_paths are random per pytest invocation).
     """
     import re

     serialized = json.dumps(settings, indent=2, sort_keys=True)
-    # Normalize: BRAIN_DIR=/tmp/pytest-N/.../brain → BRAIN_DIR=__BRAIN_DIR__
-    serialized = re.sub(r"BRAIN_DIR=/tmp/[^ ]+/brain", "BRAIN_DIR=__BRAIN_DIR__", serialized)
-    # Normalize: hook signature ID
-    serialized = re.sub(
-        r'"gradata:claude-code:/tmp/[^"]+brain"',
-        '"gradata:claude-code:__BRAIN_DIR__"',
-        serialized,
-    )
+    if brain_dir is not None:
+        for p in {str(brain_dir), brain_dir.resolve().as_posix()}:
+            serialized = serialized.replace(p, "__BRAIN_DIR__")
+    else:
+        # Fallback: strip any absolute temp path ending in /brain.
+        serialized = re.sub(r"(/[^\s\"]+)?/brain\b", "__BRAIN_DIR__", serialized)
     return serialized + "\n"

Thread brain through _assert_matches_snapshot accordingly.

🤖 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_install_claude_code_snapshot.py` around lines 96 - 104,
The normalization currently hardcodes "/tmp" and fails on non-Linux temp dirs;
update the test to normalize using the actual brain/temp directory (thread the
test's tmp_path/brain value through _assert_matches_snapshot or compute
brain_dir = str(tmp_path / "brain")) and use that value in the two re.sub calls
(escape it with re.escape) instead of the literal "/tmp"; modify the patterns
that touch serialized (the two re.sub calls operating on serialized and the hook
signature replacement) to replace occurrences of the resolved brain_dir with
"__BRAIN_DIR__" so snapshots are portable across platforms.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
Gradata/tests/test_install_claude_code_snapshot.py (1)

97-111: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

macOS temp paths still break snapshot normalization.

The updated regex patterns add Windows support but still hardcode /tmp/ for Unix-like systems. On macOS, tmp_path is typically under /var/folders/... (which resolves to /private/var/folders/...), so neither pattern will match:

  • Line 101 matches /tmp/... but not /var/... or /private/var/...
  • Line 108 matches /tmp/... but not macOS-resolved paths

The test will fail with a snapshot mismatch on macOS developer machines.

🔧 Robust cross-platform normalization

Pass the actual brain_dir and replace concrete paths instead of pattern-matching:

-def _normalized_snapshot(settings: dict) -> str:
+def _normalized_snapshot(settings: dict, brain_dir: Path) -> str:
     """Return normalized settings.json snapshot text.

     Brain-directory paths are normalized to a stable ``__BRAIN_DIR__``
     placeholder so the snapshot file doesn't change on every test run
     (tmp_paths are random per pytest invocation).
     """
-    import re

     serialized = json.dumps(settings, indent=2, sort_keys=True)
-    # Normalize hook commands across OSes:
-    #   Linux:   BRAIN_DIR=/tmp/.../brain /usr/bin/python3 -m ...
-    #   Windows: BRAIN_DIR='C:\\...\\brain' 'D:\\...\\python.exe' -m ...
-    serialized = re.sub(
-        r"BRAIN_DIR=(?:/tmp/[^ ]+/brain|'[A-Z]:\\\\[^']+\\\\brain') "
-        r"(?:/usr/bin/python3|'[A-Z]:\\\\[^']+\\\\python\.exe') -m",
-        "BRAIN_DIR=__BRAIN_DIR__ /usr/bin/python3 -m",
-        serialized,
-    )
-    # Normalize: hook signature ID
-    serialized = re.sub(
-        r'"gradata:claude-code:(?:/tmp/[^"]+brain|[A-Z]:/[^"]+/brain)"',
-        '"gradata:claude-code:__BRAIN_DIR__"',
-        serialized,
-    )
+    # Normalize: replace actual brain_dir with placeholder
+    #   - as_posix() for signature IDs (gradata:claude-code:/actual/path)
+    #   - str() for BRAIN_DIR env var in commands
+    for actual_path in {str(brain_dir), brain_dir.resolve().as_posix()}:
+        serialized = serialized.replace(actual_path, "__BRAIN_DIR__")
+    # Normalize: replace sys.executable (varies by environment)
+    import shlex, sys
+    serialized = serialized.replace(shlex.quote(sys.executable), "/usr/bin/python3")
     return serialized + "\n"

Then thread brain through _assert_matches_snapshot:

 def _assert_matches_snapshot(settings: dict, test_file: str) -> None:
+def _assert_matches_snapshot(settings: dict, test_file: str, brain_dir: Path) -> None:
     snapshot_file = _snapshot_path(test_file)
     expected = snapshot_file.read_text(encoding="utf-8")
-    actual = _normalized_snapshot(settings)
+    actual = _normalized_snapshot(settings, brain_dir)
     assert actual == expected, (

And update all call sites:

-    _assert_matches_snapshot(settings, __file__)
+    _assert_matches_snapshot(settings, __file__, brain)
🤖 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_install_claude_code_snapshot.py` around lines 97 - 111,
The regexes normalizing webhook command and signature in the serialized string
only match /tmp/... and Windows paths, missing macOS temp dirs; instead compute
the actual brain_dir used in the test (brain_dir) and replace concrete
occurrences in serialized (the variable named serialized) using a re-escaped
brain_dir value so it matches regardless of /tmp vs /var/private paths, then
normalize the python executable portion as before; thread the brain/brain_dir
through _assert_matches_snapshot and update its call sites so the test uses the
actual brain_dir when performing snapshot replacement.
🤖 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.

Duplicate comments:
In `@Gradata/tests/test_install_claude_code_snapshot.py`:
- Around line 97-111: The regexes normalizing webhook command and signature in
the serialized string only match /tmp/... and Windows paths, missing macOS temp
dirs; instead compute the actual brain_dir used in the test (brain_dir) and
replace concrete occurrences in serialized (the variable named serialized) using
a re-escaped brain_dir value so it matches regardless of /tmp vs /var/private
paths, then normalize the python executable portion as before; thread the
brain/brain_dir through _assert_matches_snapshot and update its call sites so
the test uses the actual brain_dir when performing snapshot replacement.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c85cfd11-585e-4fca-9a5f-2a84f7061fe4

📥 Commits

Reviewing files that changed from the base of the PR and between 06b2a4e and 0c28723.

📒 Files selected for processing (1)
  • Gradata/tests/test_install_claude_code_snapshot.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
Gradata/tests/**/*.py

📄 CodeRabbit inference engine (Gradata/AGENTS.md)

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
Add unit tests in tests/test_*.py for every CI push without LLM calls (deterministic); mark integration tests with @pytest.mark.integration and skip them by default (they hit real LLM APIs)

Files:

  • Gradata/tests/test_install_claude_code_snapshot.py
🧠 Learnings (4)
📓 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 : Add unit tests in `tests/test_*.py` for every CI push without LLM calls (deterministic); mark integration tests with `pytest.mark.integration` and skip them by default (they hit real LLM APIs)

Applied to files:

  • Gradata/tests/test_install_claude_code_snapshot.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_install_claude_code_snapshot.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: 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_install_claude_code_snapshot.py
🔇 Additional comments (4)
Gradata/tests/test_install_claude_code_snapshot.py (4)

130-154: LGTM!


157-182: LGTM!


185-250: LGTM!


253-283: LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant