Skip to content

fix(llm): enhance fallback logic to handle empty completions and impr…#5491

Merged
alteixeira20 merged 2 commits into
odysseus-dev:devfrom
Abhiiishek44:fix/empty-stream-fallback
Jul 18, 2026
Merged

fix(llm): enhance fallback logic to handle empty completions and impr…#5491
alteixeira20 merged 2 commits into
odysseus-dev:devfrom
Abhiiishek44:fix/empty-stream-fallback

Conversation

@Abhiiishek44

@Abhiiishek44 Abhiiishek44 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

…ove metadata handling

Summary

Updates stream_llm_with_fallback() so empty or metadata-only streams correctly move to the next fallback model.

A candidate is now accepted only after it produces text, reasoning, or a completed tool call. Buffered metadata is discarded when switching models.

Added regression tests for [DONE]-only streams, metadata-only streams, valid output, tool calls, and cases where all candidates return empty responses.

Target branch

  • This PR targets dev, not main. All PRs land in dev; main is curated by the maintainer at each release. If your PR is on main by accident, click "Edit" on this PR and change the base.

Linked Issue

Fixes #5489

Type of Change

  • Bug fix (non-breaking — fixes a confirmed issue)
  • New feature (non-breaking — adds new behaviour)
  • Breaking change (changes or removes existing behaviour)
  • Refactor / cleanup (behaviour unchanged)
  • Documentation only
  • CI / tooling / configuration

Checklist

  • I searched open issues and open PRs — this is not a duplicate.
  • This PR targets dev
  • My changes are limited to the scope described above — no unrelated refactors or whitespace changes mixed in.
  • I actually ran the app (docker compose up or uvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough.

How to Test

  1. Run the focused fallback tests:

    pytest -q tests/test_llm_core_fallback.py

  2. Run the broader LLM core test suite:

    pytest -q tests/test_llm_core*.py

  3. Verify the following cases:

    • A primary stream containing only [DONE] invokes the fallback.
    • Usage metadata followed by [DONE] invokes the fallback and discards the primary metadata.
    • Text, reasoning, whitespace deltas, and completed tool calls prevent switching.
    • If every candidate returns no substantive output, one clear terminal error is emitted.

Visual / UI changes — REQUIRED if you touched anything that renders

Anything that changes what the UI looks like — buttons, icons, padding, colors, fonts, spacing, layout, CSS, HTML, SVG, or any static/js/ module that draws to the DOM — needs all of the following. PRs that change rendering without these WILL be closed.

  • Screenshot or short clip of the change in the running app, attached below. Mobile screenshot too if the change affects mobile.
  • Style match: the change uses Odysseus's existing visual language. Specifically:
    • Reuse existing CSS variables (--red, --fg, --bg, --card, --border, etc.) — do not introduce new color values, font sizes, or spacing units.
    • Reuse existing button/input/card/border classes. Don't invent parallel styling.
    • No Unicode emoji in UI or code. Use inline SVG (matching the monochrome icon style already in static/index.html) or plain text.
    • Monospaced font (Fira Code) for primary UI text. Don't override.
    • Dark theme is the default; any light-mode work must be wired through the existing theme system, not hard-coded.
  • No new component patterns. If a similar widget already exists in the app, extend it instead of writing a parallel one.
  • I am not an LLM agent submitting a bulk PR. If you are, please open an issue describing the problem first — bulk auto-generated PRs that don't match the project's visual style are closed on sight, even when the underlying fix is correct.

Screenshots / clips

@github-actions github-actions Bot added needs work PR description incomplete — please update before review ready for review Description complete — ready for maintainer review and removed needs work PR description incomplete — please update before review labels Jul 12, 2026
@RonitSachdev

Copy link
Copy Markdown

I ran into this same bug and verified this patch's approach end-to-end against a live SSE endpoint: a [DONE]-only primary and a metadata-only (usage + [DONE]) primary both correctly fall through to the backup, and a healthy primary is unaffected. The core approach here looks right — metadata no longer commits a candidate, a failed candidate's usage/model_actual chunks don't leak into the fallback's response, and surfacing a terminal error when every candidate comes back empty is a nice improvement over a silent blank.

One thing I noticed reviewing the diff:

tool_call_delta chunks are classified as metadata and get buffered. The substantive check only accepts non-empty text deltas and completed tool_calls:

substantive = (
    isinstance(delta, str) and bool(delta)
) or (
    event_data.get("type") == "tool_calls"
    and bool(event_data.get("calls"))
)

But stream_llm also emits {"type": "tool_call_delta", ...} chunks — the live-streamed tool arguments for create_document / update_document / edit_document (the arg_delta yields in stream_llm). With this patch, a response that is purely a native doc-tool call buffers every arg_delta in pending_metadata until the final tool_calls event arrives, so document content stops streaming live in the UI and instead lands in one burst at the end of the response.

Suggestion — count streamed tool arguments as substantive output too:

or event_data.get("type") in ("tool_call_delta", "tool_calls")

Pre-patch, tool_call_delta chunks were always passed straight through to the client, so treating them as substantive preserves the existing live-streaming behavior (and once a chunk has reached the client, switching candidates would risk duplicated output anyway, which is the same reason text deltas commit the candidate).

@Abhiiishek44

Copy link
Copy Markdown
Contributor Author

@RonitSachdev You were right about tool_call_delta. I verified the behavior, and those chunks were being buffered until the final tool_calls event, which caused document tool output to arrive in one burst instead of streaming progressively.

I’ve updated the fallback logic to treat tool_call_delta as substantive output. The candidate is now committed as soon as streamed tool arguments begin, and the chunks are forwarded immediately, preserving the existing live-streaming behavior.

@Abhiiishek44

Copy link
Copy Markdown
Contributor Author

Hi @alteixeira20 , this PR is ready for review. Could you please take a look when you have time? Please let me know if any changes are needed. Thanks!

@alteixeira20 alteixeira20 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Verified at exact head cb358b6305fffb3c7aec4f8a73bd1f213b9c3c62.

The empty and metadata-only stream fallback behavior is correctly fixed. Metadata no longer commits a candidate, failed-candidate metadata is discarded, substantive text/reasoning and completed tool calls preserve the selected candidate, and tool_call_delta remains progressively streamed.

The PR merges cleanly into current dev. Compilation, focused fallback tests, the broader LLM core suite, and all effective GitHub checks pass.

No blocking findings.

@alteixeira20
alteixeira20 merged commit 68ba51c into odysseus-dev:dev Jul 18, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review Description complete — ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Empty or metadata-only primary streams prevent configured fallback models

3 participants