fix: CI type fixes and Google provider package#77
Conversation
- Add StructuredOutput return type to Client and ParameterMapper - Update HTTP default timeout from 60s to 180s - Add provider registry loading for auth entry points - Fix test assertions for new timeout value - Add Google provider package (generate_content API)
Pull Request Review: CI Type Fixes and Google Provider PackageSummaryThis PR introduces important type safety improvements and adds a new Google provider package. Overall, the implementation is solid and follows established patterns, but there are several areas that need attention before merging. ✅ Code Quality & Best PracticesStrengths
Areas for Improvement1. Missing Entry Point ImplementationLocation: The [project.entry-points."celeste.providers"]
google = "celeste_google:register_provider"But """Google provider package for Celeste AI."""
def register_provider() -> None:
"""Register Google provider authentication and capabilities."""
# Import and register auth classes
# Import and register capability clients
pass2. Incomplete Package MetadataLocation: Missing dependency declaration. Compare with Anthropic provider - you should specify dependencies: dependencies = ["celeste-ai", "httpx", "google-auth"]The 🐛 Potential Bugs & IssuesCritical Issues1. File Path Security VulnerabilityLocation: elif image.path:
with open(image.path, "rb") as f:
image_bytes = f.read()Issues:
Recommendation: elif image.path:
try:
# Validate path is safe (implement or use constraint)
if not self._is_safe_path(image.path):
raise ValueError(f"Invalid file path: {image.path}")
# Check file size before reading
file_size = os.path.getsize(image.path)
if file_size > MAX_IMAGE_SIZE: # e.g., 10MB
raise ValueError(f"Image file too large: {file_size} bytes")
with open(image.path, "rb") as f:
image_bytes = f.read()
except (OSError, IOError) as e:
raise ValueError(f"Failed to read image file: {e}") from e2. Missing Error Handling in parse_outputLocation: parsed = json.loads(content) if isinstance(content, str) else content
# For list[T], handle various formats Google might return
origin = get_origin(value)
if origin is list and isinstance(parsed, dict):
if "items" in parsed:
parsed = parsed["items"]
else:
# If it's a dict but not wrapped, try to extract array values
parsed = list(parsed.values()) if parsed else []
return TypeAdapter(value).validate_python(parsed)Issues:
Recommendation: Add explicit error handling or document that exceptions should bubble up (current pattern in other providers). 3. Inconsistent None HandlingLocation: candidates = event.get("candidates", [])
if not candidates:
return NoneLater: text_delta = parts[0].get("text") if parts else NoneThe code assumes text_delta = parts[0].get("text") if parts and len(parts) > 0 else NoneOr simply: text_delta = parts[0].get("text", "") if parts else ""Medium Issues4. Missing DocstringsSeveral methods lack proper docstrings:
5. Hardcoded API VersionLocation: All endpoints use
⚡ Performance ConsiderationsPositive
Concerns1. In-Memory Base64 EncodingLocation: Loading entire image files into memory and base64 encoding them can be memory-intensive for large images. Impact: For a 10MB image, base64 encoding creates ~13.3MB string in memory, plus the original bytes. Recommendation:
2. Recursive Schema ProcessingLocation: The Recommendation: Add depth limit or document schema complexity limitations. 3. Multiple Dictionary IterationsLocation: # Find text part and insert images before it
text_index = next(
(i for i, part in enumerate(parts) if "text" in part), len(parts)
)
# Insert image parts before text
parts[text_index:text_index] = image_partsMinor optimization: You're already iterating through parts, could cache the index during the first pass. 🔒 Security ConcernsHigh Priority1. Path Traversal VulnerabilityAlready discussed above in bugs section - MUST FIX before merge. 2. Missing Input ValidationLocation: Multiple parameter mappers While base class validation exists, there's no validation for:
Recommendation: Add constraint-based validation for these edge cases. Medium Priority3. Unresolved Schema ReferencesLocation: json_schema = self._remove_unsupported_fields(json_schema)Unlike Cohere/Mistral providers, you're not resolving Recommendation: Check if Google's API requires resolved references. If so, implement 4. Timeout Change ImpactLocation: -DEFAULT_TIMEOUT = 60.0
+DEFAULT_TIMEOUT = 180.0Concern: Tripling the timeout could hide performance issues and delay error detection. Was this increased because Google's API is slower? Recommendation:
🧪 Test CoverageCritical Gaps1. No Unit Tests for Google ProviderThe PR adds 574 lines of new code but zero test files for the Google provider package. Missing Tests:
Recommendation: Add comprehensive unit tests covering:
2. Updated Tests Not ComprehensiveLocation: Only timeout assertions were updated. Need tests for:
3. Integration Tests MissingNo integration tests verify:
📋 Additional RecommendationsDocumentation
Code Organization
Registry ChangesLocation: Good addition, but consider:
ConsistencyLocation: Anthropic provider uses 🎯 Required Changes Before Merge
📊 Summary Score
Overall: Good foundation, but needs critical fixes and tests before merge. ✨ Positive Notes
Keep up the great work! Once the critical issues are addressed and tests are added, this will be a valuable addition to the project. Review generated by Claude Code • Let me know if you'd like me to elaborate on any specific issue or need help implementing fixes. |
1 participant
Summary
Changes
src/celeste/client.py- StructuredOutput return typesrc/celeste/parameters.py- StructuredOutput return typesrc/celeste/http.py- Timeout 60s → 180ssrc/celeste/auth.py- Use provider entry pointssrc/celeste/registry.py- Add provider loading functionpackages/providers/google/- New Google provider packagetests/unit_tests/- Update test assertionsTest plan
make cipasses