chore: improve error handling and logging for webhook events

[CarlJi]

• Replace generic error messages with structured error types in parser
• Add proper error classification for unsupported events and validation errors
• Reduce log noise by changing info logs to debug for routine events
• Skip error logging for expected unsupported event types in webhook handler

[CarlJi]

/review

[codecov]

Codecov Report

❌ Patch coverage is 0% with 40 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@068a445). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
internal/events/errors.go	0.00%	26 Missing ⚠️
internal/events/parser.go	0.00%	8 Missing ⚠️
internal/webhook/handler.go	0.00%	5 Missing ⚠️
internal/agent/agent.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #336   +/-   ##
=======================================
  Coverage        ?   12.83%           
=======================================
  Files           ?       53           
  Lines           ?    11081           
  Branches        ?        0           
=======================================
  Hits            ?     1422           
  Misses          ?     9511           
  Partials        ?      148           

Flag	Coverage Δ
unittests	12.83% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[CarlJi]

/review

[CarlJi]

🤖 代码审查结果

Code Review for PR #336: Structured Error Handling Improvements

✅ Overall Assessment

This PR makes excellent improvements to error handling by introducing structured error types and reducing log noise. The changes are well-architected and follow Go best practices.

🔍 Detailed Analysis

1. New Error Types (internal/events/errors.go) ⭐

Strengths:

• Excellent error classification: Separates parsing errors from validation errors
• Proper error wrapping: Implements Unwrap() method for errors.Is() and errors.As() compatibility
• Rich context: EventError struct provides operation, event type, underlying error, and context
• Clean helper functions: UnsupportedEventTypeError(), ParsingError(), ValidationError() make error creation consistent
• Good documentation: Clear comments explaining each error type

Code Quality: 🟢 Excellent

2. Parser Changes (internal/events/parser.go) ⭐

Improvements Made:

• Line 38: Replaced generic error with UnsupportedEventTypeError(eventType)
• Line 74: Replaced generic error with UnsupportedEventTypeError(eventType)
• Line 92: Replaced "not implemented yet" with proper UnsupportedEventTypeError(eventType)
• Lines 104, 109, 112, 115, 118: Replaced generic validation errors with structured ParsingError() and ValidationError() calls

Impact: These changes make error handling consistent and enable proper error classification in calling code.

Code Quality: 🟢 Excellent

3. Agent Logging Improvements (internal/agent/agent.go) ⭐

Changes:

• Line 119: Changed Infof to Debugf for routine event processing - reduces log noise
• Line 124: Removed redundant warning log since error is already being returned to caller

Impact: Reduces log verbosity for normal operations while preserving error information.

Code Quality: 🟢 Good

4. Webhook Handler Enhancements (internal/webhook/handler.go) ⭐

Improvement:

• Lines 87-91: Added intelligent error handling that uses errors.Is() to check for ErrUnsupportedEventType
• Unsupported events now log at DEBUG level instead of WARN level
• Other errors still log at WARN level appropriately

Impact: Expected unsupported events won't clutter logs with warnings.

Code Quality: 🟢 Excellent

🚀 Architecture & Design

Strengths:

1. Separation of Concerns: Error types are properly separated into their own package
2. Consistent Error Handling: All event parsing now uses the same error creation patterns
3. Proper Go Error Handling: Implements Unwrap() and uses errors.Is() correctly
4. Backward Compatible: Changes don't break existing interfaces

Design Patterns:

• ✅ Error wrapping pattern implemented correctly
• ✅ Factory pattern for error creation
• ✅ Structured error types with context

🐛 Potential Issues

Minor Observations:

1. Line 148+ in parser.go: Some parsing functions still use generic fmt.Errorf() instead of the new structured errors. Consider consistency:

   // Current (lines 148, 153, etc.)
   return nil, fmt.Errorf("failed to unmarshal pull request review event: %w", err)
   
   // Consider
   return nil, ParsingError("pull_request_review", err)

2. Error message consistency: Some validation errors use generic fmt.Errorf() while others use ValidationError(). Consider updating remaining instances for consistency.

📋 Documentation Consistency

Status: ✅ Good

• No documentation updates needed as this is an internal refactoring
• Error messages are self-documenting
• Code comments are appropriate and clear

🔧 Recommendations

1. Complete the migration: Consider updating remaining fmt.Errorf() instances in other parsing functions (lines 148-163, 188-203, etc.) to use the new structured error helpers for complete consistency.

2. Consider error codes: For future enhancement, you might consider adding error codes to EventError for programmatic error handling.

3. Testing: Ensure error handling tests cover the new structured error types and errors.Is() behavior.

✅ Summary

This is a high-quality PR that significantly improves the codebase's error handling. The structured approach to errors, reduced log noise, and proper Go error handling patterns make this an excellent contribution.

Recommendation: APPROVE ✅

The code is ready to merge with the minor suggestions for future improvements being optional enhancements rather than blocking issues.