Debug parse error hook not emitted

[jxnl]

fix: emit parse:error hook for instructor.ValidationError

Describe your changes

Previously, the parse:error hook was not emitted when instructor.exceptions.ValidationError occurred during JSON parsing, as the retry_sync and retry_async functions only caught pydantic.ValidationError and JSONDecodeError. This PR updates the exception handling to also catch instructor.exceptions.ValidationError, ensuring the parse:error hook is consistently triggered for all validation-related parsing failures. Comprehensive tests have been added to verify the fix and prevent regressions.

Issue ticket number and link

#1817 #1817

Checklist before requesting a review

• I have performed a self-review of my code
• If it is a core feature, I have added thorough tests.
• If it is a core feature, I have added documentation.

---

Slack Thread

 

---

Important

Fix parse:error hook emission for instructor.exceptions.ValidationError in retry.py and add tests to verify behavior.

• Behavior:
  • Update retry_sync and retry_async in retry.py to catch instructor.exceptions.ValidationError and emit parse:error hook.
  • Ensures parse:error hook is triggered for all validation-related parsing failures.
• Tests:
  • Add test_parse_error_hook_fix.py to verify parse:error hook emission for InstructorValidationError in both sync and async contexts.
  • Include regression tests for pydantic.ValidationError and JSONDecodeError to ensure existing functionality is unaffected.

^{This description was created by for e8731bc. You can customize this summary. It will automatically update as commits are pushed.}

[cursor]

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
_{Learn more about Cursor Agents}

[claude]

Claude finished @jxnl's task —— View job

---

Code Review Complete ✅

I've thoroughly reviewed this pull request that fixes the parse:error hook not being emitted for instructor.ValidationError. Here's my comprehensive review:

✅ Code Quality and Best Practices

Excellent implementation! The changes are:

• ✅ Minimal and focused: Only adds the missing exception type to existing handlers
• ✅ Consistent: Applied to both sync (retry_sync:202) and async (retry_async:354-358) functions
• ✅ Follows existing patterns: Uses the same exception handling structure as other validation errors
• ✅ Well-documented: Clear commit message and PR description explaining the fix

✅ Bug Analysis - Correctly Fixed

The root cause was properly identified:

• ✅ Before: Only caught ValidationError (Pydantic) and JSONDecodeError
• ✅ After: Also catches InstructorValidationError from instructor.core.exceptions
• ✅ Impact: Ensures hooks.emit_parse_error(e) is called for all validation failures
• ✅ Backwards compatibility: Doesn't break existing error handling

✅ Performance Considerations

• ✅ Zero performance impact: Adding an exception type to a tuple has no runtime cost
• ✅ Exception handling path: Only affects error cases, not the happy path
• ✅ Identical processing: Same hook emission and retry logic as other validation errors

✅ Security Assessment

• ✅ No security concerns: Pure error handling improvement
• ✅ Error information: Doesn't expose sensitive data beyond existing patterns
• ✅ Exception propagation: Maintains existing security boundaries

✅ Test Coverage - Comprehensive

The test suite (tests/test_parse_error_hook_fix.py) is exceptionally thorough:

✅ Core Fix Tests

• test_parse_error_hook_emitted_for_instructor_validation_error_sync/async - Verifies the main fix
• Uses proper mocking of process_response functions
• Correctly verifies hook emission with InstructorValidationError

✅ Regression Tests

• Tests ensure existing functionality still works for:
  • pydantic.ValidationError
  • JSONDecodeError
• Both sync and async variants covered

✅ Integration Test

• test_all_parse_errors_caught_in_same_handler - Verifies all three error types work together
• Confirms error messages are preserved
• Tests the unified exception handling approach

✅ Test Quality

• Proper async test decorations (@pytest.mark.asyncio)
• Clean setup/teardown with mocks
• Clear assertions and helpful error tracking
• Good use of pytest fixtures and patterns

📝 Minor Observations

1. Import organization: Line 9 imports are correctly organized - no issues
2. Exception hierarchy: InstructorValidationError properly inherits from InstructorError:107
3. Hook consistency: Same hooks.emit_parse_error(e) call used for all validation errors

🎯 Recommendation: APPROVE

This is a high-quality fix that:

• ✅ Solves the reported issue completely
• ✅ Maintains backwards compatibility
• ✅ Includes comprehensive test coverage
• ✅ Has zero negative impact on performance or security
• ✅ Follows the existing codebase patterns perfectly

The implementation demonstrates excellent understanding of the codebase and problem domain. The tests are particularly impressive in their thoroughness and attention to edge cases.

---

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Updated (UTC)
✅ Deployment successful! View logs	instructor	225eb16	Oct 01 2025, 01:36 PM

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to e8731bc in 1 minute and 37 seconds. Click for details.

• Reviewed 361 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 4 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. instructor/core/retry.py:202

• Draft comment:
  Include InstructorValidationError in the sync exception tuple to ensure the parse:error hook is triggered for instructor-specific validation errors (#1817).
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 85% This comment is purely informative, explaining a change made in the code. It does not provide a suggestion, ask for confirmation, or request any action from the PR author. According to the rules, purely informative comments should be removed.

2. instructor/core/retry.py:358

• Draft comment:
  Add InstructorValidationError to the async exception tuple so that asynchronous retries also emit the parse:error hook for instructor-specific validation errors.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 85% This comment is purely informative, explaining a change made in the code. It does not provide a suggestion, ask for confirmation, or point out a potential issue. Therefore, it violates the rule against making purely informative comments.

3. tests/test_parse_error_hook_fix.py:52

• Draft comment:
  Consider asserting a more specific exception (e.g., InstructorRetryException) instead of a generic Exception to clarify test expectations.
• Reason this comment was not posted:
  Confidence changes required: 80% <= threshold 85% None

4. tests/test_parse_error_hook_fix.py:1

• Draft comment:
  The test suite is comprehensive, covering sync and async scenarios for InstructorValidationError, PydanticValidationError, and JSONDecodeError, ensuring consistent hook emission.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 85% None

Workflow ID: wflow_xGzaoqZB1uzGATXO

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[yuv-c]

Thanks for the quick feedback. Much appreciated 💯

[claude]

Claude finished @jxnl's task —— View job

---

Comprehensive PR Review ✅

I've conducted a thorough review of this pull request that fixes the parse:error hook not being emitted for instructor.ValidationError. Here's my comprehensive analysis:

Review Checklist:

• Read and analyze modified files
• Evaluate code quality and best practices
• Check for potential bugs or issues
• Assess performance implications
• Review security considerations
• Evaluate test coverage and quality
• Provide final review feedback

---

✅ Code Quality and Best Practices

Excellent implementation! The changes are:

• ✅ Minimal and focused: Only adds the missing exception type to existing handlers
• ✅ Consistent: Applied to both sync (retry_sync:202) and async (retry_async:354-358) functions
• ✅ Follows existing patterns: Uses the same exception handling structure as other validation errors
• ✅ Well-documented: Clear commit message and PR description explaining the fix

✅ Bug Analysis - Correctly Fixed

The root cause was properly identified:

• ✅ Before: Only caught ValidationError (Pydantic) and JSONDecodeError
• ✅ After: Also catches InstructorValidationError from instructor.core.exceptions
• ✅ Impact: Ensures hooks.emit_parse_error(e) is called for all validation failures
• ✅ Backwards compatibility: Doesn't break existing error handling

✅ Performance Considerations

• ✅ Zero performance impact: Adding an exception type to a tuple has no runtime cost
• ✅ Exception handling path: Only affects error cases, not the happy path
• ✅ Identical processing: Same hook emission and retry logic as other validation errors

✅ Security Assessment

• ✅ No security concerns: Pure error handling improvement
• ✅ Error information: Doesn't expose sensitive data beyond existing patterns
• ✅ Exception propagation: Maintains existing security boundaries

✅ Test Coverage - Comprehensive

The test suite (tests/test_parse_error_hook_fix.py) is exceptionally thorough:

✅ Core Fix Tests

• test_parse_error_hook_emitted_for_instructor_validation_error_sync/async - Verifies the main fix
• Uses proper mocking of process_response functions
• Correctly verifies hook emission with InstructorValidationError

✅ Regression Tests

• Tests ensure existing functionality still works for:
  • pydantic.ValidationError
  • JSONDecodeError
• Both sync and async variants covered

✅ Integration Test

• test_all_parse_errors_caught_in_same_handler - Verifies all three error types work together
• Confirms error messages are preserved
• Tests the unified exception handling approach

✅ Test Quality

• Proper async test decorations (@pytest.mark.asyncio)
• Clean setup/teardown with mocks
• Clear assertions and helpful error tracking
• Good use of pytest fixtures and patterns

📝 Minor Observations

1. Import organization: Line 9 imports are correctly organized - no issues
2. Exception hierarchy: InstructorValidationError properly inherits from InstructorError:107
3. Hook consistency: Same hooks.emit_parse_error(e) call used for all validation errors

🎯 Recommendation: APPROVE

This is a high-quality fix that:

• ✅ Solves the reported issue completely
• ✅ Maintains backwards compatibility
• ✅ Includes comprehensive test coverage
• ✅ Has zero negative impact on performance or security
• ✅ Follows the existing codebase patterns perfectly

The implementation demonstrates excellent understanding of the codebase and problem domain. The tests are particularly impressive in their thoroughness and attention to edge cases.

[claude]

Claude finished @jxnl's task —— View job

---

Code Review Complete ✅

I've thoroughly reviewed this pull request that fixes the parse:error hook not being emitted for instructor.ValidationError. Here's my comprehensive review:

✅ Code Quality and Best Practices

Excellent implementation! The changes are:

• ✅ Minimal and focused: Only adds the missing exception type to existing handlers
• ✅ Consistent: Applied to both sync (retry_sync:202) and async (retry_async:354-358) functions
• ✅ Follows existing patterns: Uses the same exception handling structure as other validation errors
• ✅ Well-documented: Clear commit message and PR description explaining the fix

✅ Bug Analysis - Correctly Fixed

The root cause was properly identified:

• ✅ Before: Only caught ValidationError (Pydantic) and JSONDecodeError
• ✅ After: Also catches InstructorValidationError from instructor.core.exceptions
• ✅ Impact: Ensures hooks.emit_parse_error(e) is called for all validation failures
• ✅ Backwards compatibility: Doesn't break existing error handling

✅ Performance Considerations

• ✅ Zero performance impact: Adding an exception type to a tuple has no runtime cost
• ✅ Exception handling path: Only affects error cases, not the happy path
• ✅ Identical processing: Same hook emission and retry logic as other validation errors

✅ Security Assessment

• ✅ No security concerns: Pure error handling improvement
• ✅ Error information: Doesn't expose sensitive data beyond existing patterns
• ✅ Exception propagation: Maintains existing security boundaries

✅ Test Coverage - Comprehensive

The test suite (tests/test_parse_error_hook_fix.py) is exceptionally thorough:

✅ Core Fix Tests

• test_parse_error_hook_emitted_for_instructor_validation_error_sync/async - Verifies the main fix
• Uses proper mocking of process_response functions
• Correctly verifies hook emission with InstructorValidationError

✅ Regression Tests

• Tests ensure existing functionality still works for:
  • pydantic.ValidationError
  • JSONDecodeError
• Both sync and async variants covered

✅ Integration Test

• test_all_parse_errors_caught_in_same_handler - Verifies all three error types work together
• Confirms error messages are preserved
• Tests the unified exception handling approach

✅ Test Quality

• Proper async test decorations (@pytest.mark.asyncio)
• Clean setup/teardown with mocks
• Clear assertions and helpful error tracking
• Good use of pytest fixtures and patterns

📝 Minor Observations

1. Import organization: Line 9 imports are correctly organized - no issues
2. Exception hierarchy: InstructorValidationError properly inherits from InstructorError:107
3. Hook consistency: Same hooks.emit_parse_error(e) call used for all validation errors

🎯 Recommendation: APPROVE

This is a high-quality fix that:

• ✅ Solves the reported issue completely
• ✅ Maintains backwards compatibility
• ✅ Includes comprehensive test coverage
• ✅ Has zero negative impact on performance or security
• ✅ Follows the existing codebase patterns perfectly

The implementation demonstrates excellent understanding of the codebase and problem domain. The tests are particularly impressive in their thoroughness and attention to edge cases.

---