Replace generic errors with custom exceptions in text-generation

[Kamilbenkirane]

Summary

This PR replaces generic Python exceptions (ValueError, TypeError) with appropriate custom exceptions from celeste.exceptions in the text-generation package.

Changes

• client.py: Replaced TypeError with ValidationError for missing prompt parameter
• anthropic/parameters.py:
  • Replaced ValueError with ConstraintViolationError for invalid thinking_budget
  • Replaced ValueError with ValidationError for empty tool_use input dict
• anthropic/streaming.py: Replaced ValueError with ValidationError for tool_use validation errors

Benefits

• Consistent error handling across the codebase
• Better error type hierarchy for catching specific exceptions
• Improved error messages and context

Note

Response parsing errors (missing content in API responses) remain as ValueError as they are internal parsing errors, consistent with the core package approach.

[claude]

PR Review: Replace generic errors with custom exceptions in text-generation

Summary

This PR successfully replaces generic Python exceptions with custom exceptions from celeste.exceptions, improving error handling consistency across the text-generation package. The changes are well-targeted and follow good error handling practices.

Code Quality ✅

Strengths:

• Appropriate exception mapping: The choice of exceptions is semantically correct:
  • ValidationError for invalid input/parameter validation (missing prompt, empty tool_use dict)
  • ConstraintViolationError for constraint violations (thinking_budget < 1024)
• Imports are correct: All necessary exceptions are properly imported from celeste.exceptions
• Docstring updates: Updated the docstring in parameters.py:43 to reflect the new exception type
• Consistent with core package: The PR notes correctly preserve ValueError for internal parsing errors, maintaining consistency

Potential Issues ⚠️

1. Missing ValidationError from public API exports (High Priority)

The ValidationError exception is used in the code but is not exported from celeste/__init__.py. This means:

• External users cannot catch this exception by importing from celeste
• They would need to know to import from celeste.exceptions directly
• This breaks the principle of least surprise

Recommendation: Add ValidationError to:

• src/celeste/exceptions.py:162-173 (the __all__ list) ✅ Already there
• src/celeste/__init__.py:9-20 (import statement) ❌ Missing
• src/celeste/__init__.py:113-141 (the __all__ list) ❌ Missing

Files to update:

• src/celeste/__init__.py

2. Semantic error in client.py (Medium Priority)

In client.py:62, a TypeError was replaced with ValidationError:

raise ValidationError(msg)  # Previously TypeError

While ValidationError is more consistent with the custom exception hierarchy, the original TypeError was semantically more accurate. According to Python conventions:

• TypeError: Wrong type of argument (missing required positional/keyword argument)
• ValidationError: Value validation failed (wrong value, but correct type)

Recommendation: Consider using TypeError here OR create a more specific exception like MissingParameterError that inherits from ValidationError. The current choice is acceptable but not ideal.

3. Test coverage gap (High Priority)

The PR does not include tests for the new exception types. Current test search shows:

• No tests validate that ValidationError is raised for empty tool_use dicts
• No tests validate that ConstraintViolationError is raised for invalid thinking_budget
• No tests validate that ValidationError is raised for missing prompt

Recommendation: Add unit tests to verify:

# Test for client.py:62
def test_missing_prompt_raises_validation_error():
    # Should raise ValidationError when prompt is missing
    
# Test for parameters.py:57  
def test_invalid_thinking_budget_raises_constraint_violation():
    # Should raise ConstraintViolationError when thinking_budget < 1024
    
# Test for parameters.py:154
def test_empty_tool_use_dict_raises_validation_error():
    # Should raise ValidationError for empty dict with BaseModel
    
# Test for streaming.py:312, 326
def test_streaming_validation_errors():
    # Should raise ValidationError for missing content scenarios

Performance Considerations ✅

No performance impact. Custom exceptions have negligible overhead compared to built-in exceptions.

Security Concerns ✅

No security issues identified. The changes improve error handling which can help with security (better error messages for debugging without exposing internals).

Version Bump Analysis ⚠️

The PR bumps versions from 0.1.1 → 0.2.0 (minor version bump). According to semantic versioning:

• Patch (0.1.1 → 0.1.2): Backward-compatible bug fixes
• Minor (0.1.1 → 0.2.0): Backward-compatible new functionality
• Major (0.1.1 → 1.0.0): Breaking changes

Analysis:
This change is technically a breaking change if external code catches the old exception types (TypeError, ValueError). However, since:

1. The package is still in 0.x (unstable API)
2. Catching specific exception types is less common than catching general Exception
3. The new exceptions inherit from Error which inherits from Exception

A minor version bump is acceptable for a 0.x release, but consider documenting this as a breaking change in release notes.

Recommendations Summary

Must Fix (before merge):

1. ✅ Export ValidationError from celeste/__init__.py - Users need to be able to import and catch this exception

Should Fix (before or after merge):

2. 📝 Add test coverage for all new exception scenarios
3. 📝 Add migration guide in PR description or CHANGELOG for users catching old exceptions

Optional (consider for follow-up):

4. 💭 Reconsider TypeError → ValidationError in client.py:62 (or document the reasoning)
5. 💭 Add examples to docstrings showing how to catch the new exceptions

Overall Assessment

Verdict: Approve with minor changes required ✅⚠️

This is a solid refactoring that improves error handling consistency. The main blocker is the missing ValidationError export. Once that's addressed and tests are added, this will be ready to merge.

Great work on maintaining consistency and updating documentation! 👍

---

Review generated with assistance from Claude Code

[claude]

Pull Request Review: Replace generic errors with custom exceptions

Summary

This PR successfully replaces generic Python exceptions with custom exceptions from celeste.exceptions, improving error handling consistency across the codebase. The changes are well-structured and follow good practices.

✅ Strengths

1. Excellent Exception Hierarchy Design

• Proper inheritance: ConstraintViolationError extends ValidationError which extends Error
• Semantically correct exception choices:
  • ValidationError for input validation (missing prompt, empty tool_use dict)
  • ConstraintViolationError for constraint violations (thinking_budget < 1024)
• Exports added to public API in src/celeste/__init__.py:20,135

2. Consistent Error Handling

The changes improve error granularity across multiple files:

• packages/text-generation/src/celeste_text_generation/client.py:62 - Missing prompt validation
• packages/text-generation/src/celeste_text_generation/providers/anthropic/parameters.py:57 - thinking_budget constraint
• packages/text-generation/src/celeste_text_generation/providers/anthropic/streaming.py:312,326 - tool_use validation

3. Good Architectural Improvements

• Removed @abstractmethod from _stream_class() and _make_stream_request() in src/celeste/client.py:150,154
• Both now raise StreamingNotSupportedError by default, allowing non-streaming clients to inherit without implementing unused methods
• Clean separation: exceptions remain low-level (no circular dependencies)

4. Version Bump

Appropriate minor version bump from 0.1.1 → 0.2.0 for API changes

🔍 Areas for Improvement

1. Test Coverage Gap ⚠️

Priority: Medium

The PR lacks tests for the new exception behavior. The existing tests/unit_tests/test_exceptions.py only tests exception construction, not the actual code paths that raise them.

Recommendation: Add tests like:

# Test missing prompt raises ValidationError
def test_missing_prompt_raises_validation_error():
    with pytest.raises(ValidationError, match="prompt is required"):
        client._create_inputs()

# Test thinking_budget constraint
def test_thinking_budget_below_1024_raises_constraint_error():
    with pytest.raises(ConstraintViolationError, match="thinking_budget must be"):
        mapper.map(request, 500, model)

# Test empty tool_use dict
def test_empty_tool_use_dict_raises_validation_error():
    with pytest.raises(ValidationError, match="Empty tool_use input dict"):
        mapper.parse_output({}, BaseModel)

2. Minor: Exception Message Clarity

Priority: Low

In packages/text-generation/src/celeste_text_generation/providers/anthropic/parameters.py:153:

msg = "Empty tool_use input dict cannot be converted to BaseModel"

Suggestion: Make the error message more actionable by explaining what the user should do or what went wrong upstream:

msg = "Empty tool_use input received from API. Cannot convert empty dict to BaseModel. This may indicate the model returned no structured data."

3. Docstring Consistency

Priority: Low

The docstring at parameters.py:43 still references the old exception type:

Raises:
    ConstraintViolationError: If value is not -1 and is less than 1024 (minimum required).

This is actually correct now! Good job updating it. However, verify similar docstrings in streaming.py are also updated if they exist.

🛡️ Security & Performance

✅ No security concerns - Custom exceptions don't introduce vulnerabilities
✅ No performance impact - Exception instantiation is negligible
✅ No breaking changes - All custom exceptions inherit from Exception, so existing except Exception handlers still work

📊 Code Quality

• Style: Consistent with codebase conventions
• Type Safety: Proper imports and type annotations maintained
• Readability: Clear error messages that help developers debug issues
• Maintainability: Centralized exception definitions in exceptions.py

🎯 Recommendations

Must Fix

None - the PR is functionally correct

Should Fix

1. Add unit tests for the new exception paths (see examples above)
2. Consider integration tests that verify end-to-end exception behavior

Nice to Have

1. Enhance error messages with more context about what went wrong
2. Consider adding a migration guide if external users catch these exceptions
3. Document the exception hierarchy in README or docs

✅ Final Verdict

APPROVED with recommendations

This is a solid refactoring that improves error handling consistency. The only concern is the lack of test coverage for the actual exception-raising code paths. I recommend adding tests before merging to ensure the behavior is locked in and prevent regressions.

The architectural changes (removing @abstractmethod for streaming methods) are smart and reduce boilerplate for non-streaming clients.

Estimated Risk: Low
Estimated Effort to Address: 1-2 hours for comprehensive test coverage

---

Great work on maintaining consistency and improving the exception hierarchy! 🚀