fix(title-generation): handle raw SSE responses - #1
Conversation
Add explicit guards for malformed title-generation responses and surface a clearer transport-mismatch warning when LiteLLM returns streamed SSE text instead of a parsed response object.
There was a problem hiding this comment.
Pull request overview
This PR improves robustness around session title generation when the auxiliary LLM call returns an unexpected transport payload (notably raw SSE text), and adjusts auxiliary routing so title generation can follow the main runtime transport mode (e.g., Responses API).
Changes:
- Add parsing/guard logic for malformed title-generation payloads (including raw SSE strings) and refine warning/log messaging.
- Emit a clearer user-facing warning when a title-generation failure appears to be a transport mismatch.
- Inherit
api_modefrommain_runtimefor auxiliary calls when the task config doesn’t specify anapi_mode, with added test coverage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
agent/title_generator.py |
Adds _extract_title_content guards and updates logging to better diagnose raw SSE/transport mismatch scenarios. |
agent/auxiliary_client.py |
Inherits api_mode from main_runtime when not configured, intended to keep auxiliary calls on the correct transport. |
run_agent.py |
Tweaks _emit_auxiliary_failure messaging to label likely transport mismatches more clearly. |
tests/agent/test_title_generator.py |
Adds tests ensuring title generation returns None on raw string responses (SSE and non-SSE). |
tests/agent/test_auxiliary_client.py |
Adds a test asserting title generation inherits api_mode from main_runtime. |
tests/run_agent/test_strict_api_validation.py |
Adds a test asserting user warnings are labeled as “transport mismatch” for the relevant failure detail. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(detail) > 220: | ||
| detail = detail[:217].rstrip() + "..." | ||
| self._emit_warning(f"⚠ Auxiliary {task} failed: {detail}") | ||
| if "Title generation transport mismatch" in detail or "raw SSE text" in detail: |
There was a problem hiding this comment.
The "Title generation transport mismatch" in detail branch appears effectively dead: _emit_auxiliary_failure uses _summarize_api_error(exc) / str(exc) for detail, and none of the new title-generation exceptions include the literal text "Title generation transport mismatch" (it only appears in a log line). Consider removing that substring check or switching it to a marker that can actually appear in detail (e.g., only key off "raw SSE text" / a structured exception type).
| if "Title generation transport mismatch" in detail or "raw SSE text" in detail: | |
| if "raw SSE text" in detail: |
| # Full detail at debug level for operators who need the stack. | ||
| logger.warning("Title generation failed: %s", e) | ||
| err_text = str(e) | ||
| if "raw SSE text" in err_text or "parsed response object" in err_text: |
There was a problem hiding this comment.
The transport-mismatch log classification is currently too broad: _extract_title_content raises the phrase "parsed response object" for any string response (including non-SSE plain text), so the if "raw SSE text" in err_text or "parsed response object" in err_text condition will label non-transport errors as a "transport mismatch". Consider tightening this to only match the SSE/streaming signature (e.g., "raw SSE text" / a more specific marker) so warnings are accurate.
| if "raw SSE text" in err_text or "parsed response object" in err_text: | |
| if "raw SSE text" in err_text: |
| if task and main_runtime and not resolved_api_mode: | ||
| inherited_api_mode = str(main_runtime.get("api_mode") or "").strip() | ||
| if inherited_api_mode: | ||
| resolved_api_mode = inherited_api_mode |
There was a problem hiding this comment.
This api_mode inheritance applies to all auxiliary tasks whenever the task config doesn't set api_mode, but the PR description says the intent is to inherit the main runtime transport specifically for title generation. Applying this globally can change behavior for other tasks (e.g., forcing codex_responses wrapping for custom/OpenAI endpoints, or suppressing Codex auto-detection when the main runtime is chat_completions). Consider scoping this to task == "title_generation" (or documenting/justifying the broader behavior).
| def _extract_title_content(response) -> str: | ||
| """Return title text or raise a descriptive error for malformed payloads.""" | ||
| if response is None: | ||
| raise RuntimeError("Auxiliary title_generation: LLM returned None response") | ||
|
|
||
| if isinstance(response, str): | ||
| preview = response[:120] | ||
| if response.lstrip().startswith("data:"): | ||
| raise RuntimeError( | ||
| "Auxiliary title_generation: provider returned raw SSE text instead of a parsed response object " | ||
| f"(preview={preview!r}). This usually means the selected transport is streaming /responses output " | ||
| "but the caller is still using chat-completions parsing." | ||
| ) | ||
| raise RuntimeError( | ||
| "Auxiliary title_generation: LLM returned a string instead of a parsed response object " | ||
| f"(preview={preview!r})." | ||
| ) | ||
|
|
||
| try: | ||
| choices = response.choices | ||
| if not choices or not hasattr(choices[0], "message"): | ||
| raise AttributeError("missing choices[0].message") | ||
| content = (choices[0].message.content or "").strip() | ||
| except (AttributeError, TypeError, IndexError) as exc: | ||
| response_type = type(response).__name__ | ||
| response_preview = str(response)[:120] | ||
| raise RuntimeError( | ||
| f"Auxiliary title_generation: LLM returned invalid response " | ||
| f"(type={response_type}): {response_preview!r}. " | ||
| f"Expected object with .choices[0].message.content." | ||
| ) from exc |
There was a problem hiding this comment.
_extract_title_content largely duplicates the response-shape validation already implemented in agent.auxiliary_client._validate_llm_response (including the preview/truncation logic). Consider reusing or extending the centralized validator (e.g., teach _validate_llm_response to detect raw SSE strings and emit the transport-mismatch hint) to avoid the two drifting over time.
Summary\n- Add explicit guards for title generation responses that arrive as raw SSE text\n- Improve logs and user-facing auxiliary failure warnings for transport mismatches\n- Inherit the main runtime api_mode for title generation so Responses API sessions stay on the right transport\n\n## Validation\n- scripts/run_tests.sh tests/agent/test_title_generator.py tests/agent/test_auxiliary_client.py::TestGetTextAuxiliaryClient::test_title_generation_inherits_main_runtime_api_mode tests/run_agent/test_strict_api_validation.py -q -n 4\n\n## Notes\nThis fixes the LiteLLM / Responses API path where title generation could receive a streamed SSE string instead of a parsed completion object.