fix: handle string responses in agent model_dump calls

[jeremyeder]

Summary

Fixes the critical error where agent analyses fail with "'str' object has no attribute 'model_dump'" that was occurring for STAFF_ENGINEER and ENGINEERING_MANAGER personas.

Root Cause

The issue occurs when Settings.llm.astructured_predict() sometimes returns a string instead of the expected Pydantic model object. The code was blindly calling .model_dump() on all responses, causing runtime errors when strings were returned.

Changes Made

• Type checking in stream_structured_predict(): Added safety check to detect when LLM returns unexpected types and wrap them properly
• Safe model_dump in event streaming: Added hasattr() check before calling .model_dump()
• Consistent error handling: Made both functions use the same safe approach
• Better function reuse: Made event streaming use the safer stream_structured_predict() function

Technical Details

Before (Broken)

return final_response.model_dump()  # ❌ Crashes if final_response is a string

After (Fixed)

if hasattr(final_response, 'model_dump'):
    result = final_response.model_dump()
else:
    result = {"analysis": str(final_response), "persona": persona}

Test Results

✅ Before Fix: STAFF_ENGINEER and ENGINEERING_MANAGER analyses failed with model_dump errors
✅ After Fix: All agents handle both Pydantic models and string responses gracefully

Impact

• Agent Analysis: Prevents runtime crashes for all agent personas
• RFE Generation: Ensures workflow completes even with unexpected response types
• Error Handling: Graceful degradation when LLM returns unexpected formats
• Robustness: Makes the system more resilient to LLM API variations

🤖 Generated with Claude Code

[claude]

Code Review Feedback

✅ Positive Aspects

Defensive Programming: Excellent use of hasattr() checks to handle unexpected response types gracefully. This is a robust approach to dealing with LLM API variability.

DRY Principle: Smart refactoring to make stream_structured_predict_with_events() use the safer stream_structured_predict() function instead of duplicating the LLM call.

Error Handling: Good fallback strategy when the LLM returns unexpected types - wrapping strings in the expected Pydantic model structure.

Clear Documentation: The PR description clearly explains the root cause and fix with before/after examples.

🔍 Code Quality Observations

Type Safety: The fix addresses a runtime type issue, but consider adding type hints for better IDE support.

Logging Consistency: Using print() statements for logging (lines 30, 35, 109). Consider using a proper logging framework instead.

Magic Values: The fallback structure is hardcoded. Consider extracting to a helper method.

⚠️ Potential Issues

Constructor Assumptions (line 31): The code assumes output_cls(analysis=str(response), persona=persona) will work, but this might fail if the Pydantic model constructor requires different fields.

Import Duplication: import asyncio appears both at the top (line 3) and inside the function (line 79). Remove the inline import.

Performance: The simulated streaming with await asyncio.sleep(0.01) adds 10ms per chunk. For long responses, this could add significant latency.

🧪 Test Coverage Concerns

The changes lack test coverage for:

• LLM returning string responses instead of Pydantic models
• Error handling paths in both functions
• Edge cases where output_cls constructor fails

📊 Overall Assessment

This is a solid defensive fix that addresses a critical runtime error. The approach is pragmatic and maintains backward compatibility while improving system reliability.

Status: ✅ Approve with minor suggestions