refactor(core): StructuredOutput types, timeout increase, provider loading

[Kamilbenkirane]

Summary

• Update ParameterMapper.parse_output to use StructuredOutput return type for type safety
• Update Client._parse_content to use StructuredOutput return type
• Increase HTTP default timeout from 60s to 180s for longer API calls
• Add provider entry point loading in registry module
• Update test assertions to match new timeout value

Changes

• src/celeste/parameters.py: StructuredOutput return type
• src/celeste/client.py: StructuredOutput return type
• src/celeste/http.py: DEFAULT_TIMEOUT = 180.0
• src/celeste/registry.py: Provider entry point loading
• src/celeste/auth.py: Use entry point loading function
• tests/unit_tests/test_client.py: Update test helpers for StructuredOutput
• tests/unit_tests/test_http.py: Update timeout assertions

Test plan

• All unit tests pass
• Type checking passes (mypy)
• Linting passes (ruff)
• Security scan passes (bandit)

[claude]

Code Review: refactor(core): StructuredOutput types, timeout increase, provider loading

Summary

This PR introduces several improvements to the core codebase: stronger type safety with StructuredOutput, increased timeout for long-running operations, and a new provider entry point loading mechanism. Overall, the changes are well-structured and improve code quality.

---

✅ Strengths

1. Type Safety Improvements

The introduction of StructuredOutput return types in ParameterMapper.parse_output() and Client._parse_content() is excellent:

• src/celeste/parameters.py:36-38: Clear type signature replacing object with StructuredOutput
• src/celeste/client.py:127, 209, 211: Consistent usage throughout the client pipeline
• This provides better IDE autocomplete and catches type errors at development time

2. Well-Tested Changes

All test files have been updated to match the new signatures:

• tests/unit_tests/test_client.py:96-99, 126-129: Test helpers properly updated
• tests/unit_tests/test_http.py:299, 319: Timeout assertions correctly updated
• The PR description confirms all tests, type checking, linting, and security scans pass

3. Provider Loading Separation

The new _load_providers_from_entry_points() function in src/celeste/registry.py:28-44 properly separates concerns:

• Separate tracking with _loaded_providers set
• Entry point group celeste.providers is distinct from celeste.packages
• Early return optimization prevents redundant loading

---

🔍 Issues & Concerns

1. Timeout Increase Lacks Justification ⚠️ MEDIUM PRIORITY

src/celeste/http.py:17: DEFAULT_TIMEOUT increased from 60s to 180s (3x)

Concerns:

• No explanation in the PR description for why 180s was chosen
• This is a global default affecting all HTTP operations across the entire library
• Users with fast APIs (text generation, embeddings) will now wait 3 minutes on timeout instead of 1 minute
• The Google video generation provider already has its own DEFAULT_TIMEOUT = 300.0 (5 minutes) in packages/capabilities/video-generation/src/celeste_video_generation/providers/google/config.py:7, showing capability-specific timeouts are supported

Recommendation:
Instead of changing the global default, consider:

1. Allow per-client or per-capability timeout configuration
2. Document which specific operations require longer timeouts
3. Consider reverting to 60s and having video/long-running capabilities override as needed
4. If keeping 180s, add a comment explaining the rationale (e.g., "Increased for long-running operations like video generation")

2. Type Definition Could Be More Specific 🔷 LOW PRIORITY

src/celeste/types.py:9: StructuredOutput = str | JsonValue | BaseModel | list[BaseModel]

Observation:

• JsonValue already includes str, so the union starts with str redundantly
• The type allows list[BaseModel] but not list[str] or list[JsonValue], which seems intentionally restrictive

Recommendation:

• If this is intentional (likely for API response parsing), consider adding a docstring explaining why
• Example: """Structured output types for AI provider responses. Str is explicit for text content."""

3. Potential Circular Import Risk 🔷 LOW PRIORITY

src/celeste/auth.py:69: Import inside function to avoid circular dependency

from celeste.registry import _load_providers_from_entry_points

Observation:

• This is a valid pattern to avoid circular imports
• However, it happens on every call to get_auth_class(), though the function itself has early returns

Recommendation:

• Current implementation is acceptable
• Consider documenting why the import is inside the function (e.g., # Import here to avoid circular dependency)

---

🎯 Positive Patterns

1. Consistent naming: The rename from _load_from_entry_points to _load_providers_from_entry_points is clear and descriptive
2. Test coverage: All affected test assertions were updated
3. Type narrowing: Moving from object to StructuredOutput improves type safety without breaking changes
4. Performance optimization: Early returns in entry point loading prevent redundant work

---

📋 Recommendations

Must Address:

1. Document or reconsider the timeout increase - This affects all users and needs clear justification

Nice to Have:

2. Add docstring to StructuredOutput type explaining its purpose
3. Add inline comment in auth.py:69 about circular import avoidance
4. Consider adding a migration guide if any downstream providers need updates

---

🔒 Security & Performance

• ✅ No security concerns identified
• ✅ No obvious performance regressions (entry point loading is cached)
• ⚠️ Timeout increase may improve stability for long operations but could worsen UX for failed fast operations

---

Final Assessment

Status: Approve with minor concerns

The type safety improvements and provider loading separation are solid contributions. The main concern is the global timeout increase, which needs better justification or a more targeted approach. If the timeout change is essential for specific use cases, consider making it configurable or documenting the rationale clearly.

Suggested Action: Address the timeout justification before merging, or consider making it configurable per-capability rather than a global change.