Safer agent hook parsing, make sure we preserve unknown hook types#314
Safer agent hook parsing, make sure we preserve unknown hook types#314
Conversation
Entire-Checkpoint: 48463c89a2b7
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #308 where entire enable was silently dropping unknown Claude Code hook types (e.g., PreCompact, Notification, SubagentStop) from agent settings files. The root cause was that the CLI unmarshaled hooks into a struct with only known fields, causing Go's JSON unmarshaler to silently discard unknown fields. The fix uses map[string]json.RawMessage to preserve all hook types while only parsing and modifying the specific hooks that Entire needs.
Changes:
- Refactored hook parsing to use
map[string]json.RawMessagefor forward compatibility - Added shared test utilities for hook verification
- Enhanced test coverage for unknown hook type preservation
- Applied fix consistently to both Claude Code and Gemini CLI agents
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/agent/testutil/hooks.go | New shared test utility package for reading raw hooks and extracting keys from JSON maps |
| cmd/entire/cli/agent/claudecode/hooks.go | Refactored InstallHooks and UninstallHooks to use map[string]json.RawMessage for preserving unknown hook types; added parseHookType and marshalHookType helpers |
| cmd/entire/cli/agent/claudecode/hooks_test.go | Added comprehensive tests for unknown hook type preservation during install/uninstall; refactored to use shared testutil functions; renamed containsDenyRule to containsRule for clarity |
| cmd/entire/cli/agent/geminicli/hooks.go | Refactored InstallHooks and UninstallHooks to use map[string]json.RawMessage for preserving unknown hook types; added parseGeminiHookType and marshalGeminiHookType helpers |
| cmd/entire/cli/agent/geminicli/hooks_test.go | Added comprehensive tests for unknown hook type preservation during install/uninstall; added tests for preserving user hooks on same hook types |
| cmd/entire/cli/integration_test/hooks_test.go | Fixed formatting (indentation) of import statement |
toothbrush
left a comment
There was a problem hiding this comment.
Looks reasonable to me. I wonder if this kind of logic can/should be extracted out of the individual Gemini & Claude hook definitions, so that it'll be easier to reuse for the N+1st agent we add? Something we might want to bear in mind.
|
@toothbrush yes, it should. I also thought about this, but given we are having an agent refactor coming up I decided to leave it like that for now. |
Fixes #308
The agent settings parsing was too strict and removed any hook types that weren't defined / known in the CLI repo. This handles this safer and prevents removing hook types the CLI doesn't know about.