From 1dcef77a2b8686af745b6aa43ec66c964eb3bcbd Mon Sep 17 00:00:00 2001 From: James Veitch <1722315+darth-veitcher@users.noreply.github.com> Date: Mon, 20 Jul 2026 08:59:06 +0100 Subject: [PATCH] fix: keep chat_structured retry's nested seed in sync with the top-level one beacon-reviewer caught a real gap: when a caller pins options["seed"], the retry loop set the new seed on ModelSettings' top-level "seed" field but left the same request's extra_body.options.seed (the Ollama-native passthrough) at the original pinned value. A backend that honors the nested field over the top-level OpenAI one would keep sending the identical effective seed on every retry, silently defeating this fix for exactly the pinned-seed case it needs to cover. Now both fields are kept in sync on every retry attempt, built via fresh copies rather than mutating the caller's options dict in place (that dict is shared across every attempt, and possibly across other calls). Added a regression test asserting the caller's options dict is untouched after a multi-attempt retry. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj --- src/comfydv/_llm/chat.py | 21 ++++++++++++++++++++- tests/test_llm_chat_structured.py | 28 +++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/comfydv/_llm/chat.py b/src/comfydv/_llm/chat.py index b45283c..dca8bef 100644 --- a/src/comfydv/_llm/chat.py +++ b/src/comfydv/_llm/chat.py @@ -136,7 +136,26 @@ async def chat_structured( # top-level "seed" param, which works against both Ollama's and # llama-server's OpenAI-compatible endpoints) and give it a beat # via RETRY_BACKOFF_SECS in case it's still finishing loading. - attempt_settings["seed"] = next_seed(options, attempt) + seed = next_seed(options, attempt) + attempt_settings["seed"] = seed + if "extra_body" in attempt_settings: + # beacon-reviewer caught this: if a caller pinned options["seed"], + # it's also sitting in extra_body.options.seed (the Ollama-native + # passthrough). Left untouched, a backend that honors that nested + # field over the top-level OpenAI "seed" above would keep sending + # the same old seed on every retry — silently defeating this fix + # for exactly the pinned-seed case. Copy rather than mutate in + # place: extra_body/options here are the caller's own dicts, + # shared across every attempt (and possibly other calls). + # ModelSettings declares extra_body as `object` (it's an + # opaque passthrough field), so a plain dict() call on it + # doesn't type-check — cast first, this module always builds + # it as a dict (see model_settings above). + extra_body = dict(cast(dict, attempt_settings["extra_body"])) + nested_options = dict(extra_body.get("options") or {}) + nested_options["seed"] = seed + extra_body["options"] = nested_options + attempt_settings["extra_body"] = extra_body try: result = await agent.run( prompt, diff --git a/tests/test_llm_chat_structured.py b/tests/test_llm_chat_structured.py index b9f2ce2..ea57ed6 100644 --- a/tests/test_llm_chat_structured.py +++ b/tests/test_llm_chat_structured.py @@ -244,7 +244,33 @@ def test_chat_structured_retry_seed_starts_from_pinned_base(monkeypatch): assert fake.calls[0][2] == {"extra_body": {"options": {"seed": 42}}} assert fake.calls[1][2]["seed"] == 43 # base(42) + (attempt 2 - 1) - assert fake.calls[1][2]["extra_body"] == {"options": {"seed": 42}} + # The nested Ollama-native options.seed must track the same retry seed as + # the top-level one — a backend that honors the nested field over the + # top-level OpenAI "seed" must not keep seeing the stale pinned value. + assert fake.calls[1][2]["extra_body"] == {"options": {"seed": 43}} + + +def test_chat_structured_retry_does_not_mutate_callers_options_dict(monkeypatch): + """Regression guard for the fix above: syncing the nested seed must copy, + not mutate, the caller's options dict — otherwise a second call reusing + the same options object would start from the wrong base seed.""" + bad = ValidationError.from_exception_data("Widget", []) + fake = _FakeAgent([bad, _Widget(name="c", count=3)]) + monkeypatch.setattr(chat_mod, "_build_agent", lambda **kw: fake) + + caller_options = {"seed": 42} + _run_async( + chat_mod.chat_structured( + base_url="http://localhost:11434/v1", + model="llama3", + messages=_messages(), + schema=_Widget, + options=caller_options, + max_retries=2, + ) + ) + + assert caller_options == {"seed": 42} def test_chat_structured_retry_sleeps_between_attempts(monkeypatch):