refactor(cli, supervisor, provider): unify Hook input and simplify OutputDecision#68
Merged
refactor(cli, supervisor, provider): unify Hook input and simplify OutputDecision#68
Conversation
…PreToolUse hook ## Code Quality Improvements 1. **Unified code style** - Changed all Chinese comments to English for consistency 2. **Added constants** - Defined PreToolUseMatcherAskUserQuestion constant to avoid magic strings 3. **Improved type aliases** - Added detailed deprecation explanation for HookOutput 4. **Enhanced documentation** - Added comprehensive comments explaining PreToolUseInput fields ## Bug Fixes 1. **Fixed event type detection** - Unknown event types now correctly default to Stop event for backward compatibility - Previous: Only checked if eventType == "" - Fixed: Explicitly check for known types (Stop, PreToolUse), default to Stop for all others 2. **Fixed test expectations** - Updated TestDetectEventType to expect EventTypeStop for unknown event types 3. **Added session ID validation** - detectEventType now returns error when session_id is missing ## Test Improvements 1. **Added TestDetectEventType_EmptySessionID** - Validates error handling for missing session_id 2. **Added TestDetectEventType_InvalidJSON** - Validates error handling for malformed JSON 3. **Fixed test expectations** - Updated default feedback messages to use English ## Additional Changes - Updated fallback feedback messages from Chinese to English: - "请继续完成任务" → "Please continue completing the task" - "请继续完成任务后再提问" → "Please complete the task before asking questions" - "任务还没有完成，请继续" → "Please continue completing the task" All changes maintain backward compatibility with existing Stop hook functionality.
## Changes 1. **Unified Hook input** - Replaced separate HookInputHeader, StopHookInput, and PreToolUseInput with single HookInput struct - HookInput contains all fields for both event types - HookEventName determines which fields are used - Removed detectEventType function - parsing now happens directly in RunSupervisorHook 2. **Simplified OutputDecision** - Added eventType parameter instead of separate functions - OutputDecision now accepts EventType, allow, feedback - Internal methods: outputStopDecisionInternal and outputPreToolUseDecisionInternal - Deprecated OutputPreToolUseDecision now calls OutputDecision with EventTypePreToolUse 3. **Removed unnecessary constant** - Deleted PreToolUseMatcherAskUserQuestion constant, using literal string ## Benefits - **Clearer API**: Single input type for all hook events - **Better extensibility**: Easy to add new event types by adding new case in OutputDecision - **Backward compatible**: Existing Stop event behavior unchanged - **No magic strings**: Using literal "AskUserQuestion" directly ## Files Changed - internal/cli/hook.go - Unified input parsing and simplified logic - internal/cli/hook_test.go - Updated tests for unified HookInput - internal/provider/provider.go - Removed constant, use string literal - internal/supervisor/output.go - Refactored OutputDecision with eventType parameter - internal/supervisor/output_test.go - Updated tests for new signature - internal/supervisor/logger_integration_test.go - Updated OutputDecision calls All tests pass and all checks succeed.
guyskk
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors the PreToolUse hook implementation from PR #66 to improve code quality and API design.
Changes
1. Unified Hook Input Structure
Problem: PR #66 had three separate input types (HookInputHeader, StopHookInput, PreToolUseInput)
Solution: Created single unified
HookInputstruct that contains all fields for both event typesBenefits:
2. Simplified OutputDecision API
Problem: PR #66 had two separate methods (OutputDecision and OutputPreToolUseDecision)
Solution:
eventTypeparameter toOutputDecisionoutputStopDecisionInternalandoutputPreToolUseDecisionInternalBenefits:
3. Removed DetectEventType Function
Problem:
detectEventTypefunction only read and parsed, then returned parts separatelySolution: Parse input directly in
RunSupervisorHookusingjson.DecoderBenefits:
4. Code Quality Improvements
5. Test Improvements
HookInputstruct parsingOutputPreToolUseDecisiontestsOutputDecisionAPITesting
Files Changed
internal/cli/hook.go- Unified input parsing, simplified logicinternal/cli/hook_test.go- Updated tests for unified input structureinternal/provider/provider.go- No changes (using literal "AskUserQuestion")internal/supervisor/output.go- Simplified OutputDecision with eventType parameterinternal/supervisor/output_test.go- Updated tests for new APIinternal/supervisor/logger_integration_test.go- Updated OutputDecision callsBackward Compatibility
All changes maintain 100% backward compatibility with existing Stop hook functionality. Unknown event types default to Stop format, ensuring robust behavior for future Claude Code updates.