fix(gateway/hooks): normalize scalar events to single-item list - #37486
fix(gateway/hooks): normalize scalar events to single-item list#37486developers-universe-1 wants to merge 1 commit into
Conversation
Issue NousResearch#11902: When a HOOK.yaml declared events as a scalar string (e.g. events: agent:start), the loader iterated over the string character-by-character, registering handlers for individual chars instead of the intended event name. Fix: After reading manifest.get('events', []), check isinstance(events, str) and wrap in a list before iterating. Also adds regression tests covering scalar string, list, and empty/missing events cases. Fixes NousResearch#11902
combatsheep
left a comment
combatsheep
left a comment
There was a problem hiding this comment.
Scalar values are normalized only when needed, and the regression tests cover scalar/list/missing manifest cases cleanly. Approving.
alt-glitch
added
type/bug
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Clean fix: normalizes scalar string events values in HOOK.yaml manifests to single-item lists, preventing the character-by-character iteration bug. Includes a comprehensive new test file (test_gateway_hooks.py) with 3 well-written tests covering scalar, list, and empty events.
✅ Looks Good
- One-line fix in
gateway/hooks.py— elegant - 100-line test file covering the regression
- Test verifies both the fix and that existing list behavior is preserved
- Well-documented test with regression reference (Issue #11902)
Reviewed by Hermes Agent
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused regression fix. The premise still holds on current main: gateway/hooks.py:113 reads events without normalization and gateway/hooks.py:147 iterates it directly, so a scalar YAML string is registered character-by-character.
Problems
- The new
tests/test_gateway_hooks.pyduplicates the existing HookRegistry discovery suite intests/gateway/test_hooks.py:35-112, which already owns list-event and missing-event coverage.
Suggested changes
- Fold the scalar-string regression into
tests/gateway/test_hooks.py, reusing_create_hook()and the existingHOOKS_DIRpatching pattern. Preserve the assertion thatagent:startis registered without character keys.
Automated hermes-sweeper review.
| assert "agent:start" in registry._handlers | ||
| # Ensure no character-level keys were registered | ||
| for char in "agent:start": | ||
| assert char not in registry._handlers, f"unexpected char key: {char!r}" |
There was a problem hiding this comment.
Please place this regression in the existing HookRegistry suite at tests/gateway/test_hooks.py (which already covers list and missing events) so hook-loader fixtures and behavior coverage remain consolidated.
teknium1
added
sweeper:risk-compatibility
5 participants
Summary
Fixes #11902
When a HOOK.yaml declared
eventsas a scalar string (e.g.events: agent:start), the loader iterated over the string character-by-character, registering handlers for individual characters instead of the intended event name.Changes
manifest.get('events', []), checkisinstance(events, str)and wrap in a list before iterating.Verification
python -m pytest tests/test_gateway_hooks.py -v # 3 passed