diff --git a/effectful/handlers/llm/harness/durability/transaction.py b/effectful/handlers/llm/harness/durability/transaction.py index 067fc19e2..4e078f403 100644 --- a/effectful/handlers/llm/harness/durability/transaction.py +++ b/effectful/handlers/llm/harness/durability/transaction.py @@ -13,7 +13,10 @@ call_tool, call_user, ) -from effectful.handlers.llm.harness.serialization import ToolCallID +from effectful.handlers.llm.harness.serialization import ( + ToolCallID, + _is_empty_text_block, +) from effectful.ops.semantics import fwd, handler from effectful.ops.syntax import ObjectInterpretation, implements from effectful.ops.types import Operation @@ -34,14 +37,11 @@ def get_history(cls) -> collections.abc.MutableSequence[Message]: @classmethod def append_message(cls, message: Message) -> None: - """Append `message` to the ambient history, if it is legal where it lands. - - Both checks are about position rather than content, which is why they sit - here: every message the harness records passes through this method, - including the ones a failed attempt records on its way out, and those are - the ones that get a history into a shape no provider will accept. - """ + """Append `message` to the ambient history, if it is legal where it lands.""" history = cls.get_history() + assert cls._carries_no_empty_block(message), ( + f"a message may not carry an empty text block: {message}" + ) if message["role"] == "tool": assert cls._tool_call_answers_request(message, history) elif message["role"] == "assistant": @@ -51,6 +51,14 @@ def append_message(cls, message: Message) -> None: ) history.append(message) + @staticmethod + def _carries_no_empty_block(message: Message) -> bool: + """Whether `message` is free of the empty text blocks Anthropic rejects.""" + content = message.get("content") + return not isinstance(content, list) or not any( + _is_empty_text_block(block) for block in content + ) + @staticmethod def _assistant_speaks_in_turn( history: collections.abc.Sequence[Message], diff --git a/effectful/handlers/llm/harness/hooks.py b/effectful/handlers/llm/harness/hooks.py index d45441976..dcbf340e0 100644 --- a/effectful/handlers/llm/harness/hooks.py +++ b/effectful/handlers/llm/harness/hooks.py @@ -267,8 +267,11 @@ def call_assistant[T]( result = None if not tool_calls: serialized_result = message.get("content") or message.get("reasoning_content") - assert isinstance(serialized_result, str) try: + assert isinstance(serialized_result, str), ( + f"the model replied with neither content nor a tool call " + f"(finish_reason={choice.finish_reason!r})" + ) # A text answer is the model's own prose, and anything else is JSON # shaped like the response format. Both are boxed and validated # through the same model, so whatever the return annotation carries diff --git a/effectful/handlers/llm/harness/provision/litellm.py b/effectful/handlers/llm/harness/provision/litellm.py index 05e1aa203..d77ad3536 100644 --- a/effectful/handlers/llm/harness/provision/litellm.py +++ b/effectful/handlers/llm/harness/provision/litellm.py @@ -11,6 +11,7 @@ ResultDecodingError, completion, ) +from effectful.handlers.llm.harness.serialization import _is_empty_text_block from effectful.ops.semantics import fwd from effectful.ops.syntax import ObjectInterpretation, implements @@ -28,32 +29,43 @@ def __init__(self, model="gpt-4o", **config): } @staticmethod - def _mark(msg: Message) -> Message: - """`msg`, with a `cache_control` breakpoint on its last content block. + def _mark(msg: Message) -> Message | None: + """`msg`, with a `cache_control` breakpoint on its last non-empty content + block, or `None` if it has no block to put one on. A message whose content is a plain string -- which the assembled system prompt is not, but a hand-written or externally supplied message may be -- takes the message-level key instead, the only form litellm reads a breakpoint from for string content. + + Empty blocks are skipped: Anthropic answers ``cache_control cannot be + set for empty text blocks``. An already-marked message returns itself. """ content = msg.get("content") if isinstance(content, str): + if not content: + return None return typing.cast(Message, {**msg, "cache_control": {"type": "ephemeral"}}) - if not isinstance(content, list) or not content: - return msg - last_block = content[-1] - if not isinstance(last_block, dict) or "cache_control" in last_block: - return msg - return typing.cast( - Message, - { - **msg, - "content": [ - *content[:-1], - {**last_block, "cache_control": {"type": "ephemeral"}}, - ], - }, - ) + if not isinstance(content, list): + return None + for i in reversed(range(len(content))): + block = content[i] + if not isinstance(block, dict) or _is_empty_text_block(block): + continue + if "cache_control" in block: + return msg + return typing.cast( + Message, + { + **msg, + "content": [ + *content[:i], + {**block, "cache_control": {"type": "ephemeral"}}, + *content[i + 1 :], + ], + }, + ) + return None def _add_cache_control( self, @@ -81,15 +93,23 @@ def _add_cache_control( Returns a new list, leaving the caller's messages untouched, so these transport-level annotations never reach the stored history -- and so never reach an `Agent`'s checkpointed transcript. + + If `_mark` declines a message, the scan continues to the one before it. """ out = list(messages) for i in reversed(range(len(out))): if out[i]["role"] in ("user", "tool"): - out[i] = self._mark(out[i]) + marked = self._mark(out[i]) + if marked is None: + continue + out[i] = marked break for i in range(len(out)): if out[i]["role"] == "system": - out[i] = self._mark(out[i]) + # No earlier system message to fall back to. + marked = self._mark(out[i]) + if marked is not None: + out[i] = marked break return out diff --git a/effectful/handlers/llm/harness/serialization.py b/effectful/handlers/llm/harness/serialization.py index a1a229800..2df501130 100644 --- a/effectful/handlers/llm/harness/serialization.py +++ b/effectful/handlers/llm/harness/serialization.py @@ -60,6 +60,22 @@ ) +def _is_empty_text_block(block: typing.Any) -> bool: + """Whether `block` is a text block with no text.""" + return ( + isinstance(block, dict) + and block.get("type") == "text" + and not block.get("text") + ) + + +def _text_blocks(text: str) -> list[OpenAIMessageContentListBlock]: + """`text` as one content block, or no block at all when `_is_empty_text_block` + would reject it.""" + block = ChatCompletionTextObject(type="text", text=text) + return [] if _is_empty_text_block(block) else [block] + + @pydantic.validate_call(validate_return=True) def to_content_blocks( value: typing.Any, @@ -73,17 +89,19 @@ def to_content_blocks( Inside JSON structures, separators match ``json.dumps`` defaults so that the linearization law holds for non-string encoded values: ``linearize(to_content_blocks(v)) == json.dumps(v)``. + + Every text block goes through `_text_blocks`, so none of them is empty; + Anthropic rejects a request containing one. """ if isinstance(value, str): - return [ChatCompletionTextObject(type="text", text=value)] + return _text_blocks(value) buf: list[str] = [] blocks: list[OpenAIMessageContentListBlock] = [] def flush() -> None: - if buf: - blocks.append(ChatCompletionTextObject(type="text", text="".join(buf))) - buf.clear() + blocks.extend(_text_blocks("".join(buf))) + buf.clear() def walk(v: typing.Any) -> None: if isinstance(v, dict) and v.get("type") in CONTENT_BLOCK_TYPES: @@ -120,6 +138,9 @@ def format_as_content_blocks( Format a template applied to arguments into a list of content blocks. This is similar to str.format() but produces a list of content blocks instead of a single string, so that non-text content is preserved. + + A conversion or format spec runs even on a value that encodes to ``""``, so + ``{x!r}`` renders ``''``. Text that formats to nothing produces no block. """ formatter = string.Formatter() parts: list[OpenAIMessageContentListBlock] = [] @@ -127,9 +148,8 @@ def format_as_content_blocks( buf: list[str] = [] def flush_text() -> None: - if buf: - parts.append(ChatCompletionTextObject(type="text", text="".join(buf))) - buf.clear() + parts.extend(_text_blocks("".join(buf))) + buf.clear() for literal, field_name, format_spec, conversion in formatter.parse( textwrap.dedent(template) @@ -140,19 +160,25 @@ def flush_text() -> None: if field_name is None: continue + def formatted(text: str, conversion=conversion, spec=format_spec) -> str: + if conversion: + text = formatter.convert_field(text, conversion) + return formatter.format_field(text, spec or "") + obj, _ = formatter.get_field(field_name, (), env) encoder: pydantic.TypeAdapter[typing.Any] = pydantic.TypeAdapter( Encodable[nested_type(obj).value] # type: ignore[misc] ) encoded_obj = encoder.dump_python(obj, mode="json", context=env) + if isinstance(encoded_obj, str): + # Formatted here rather than through `to_content_blocks`, which + # drops an empty string before a conversion or format spec could + # turn it into something. + buf.append(formatted(encoded_obj)) + continue for part in to_content_blocks(encoded_obj): if part["type"] == "text": - text = ( - formatter.convert_field(part["text"], conversion) - if conversion - else part["text"] - ) - buf.append(formatter.format_field(text, format_spec or "")) + buf.append(formatted(part["text"])) else: flush_text() parts.append(part) diff --git a/tests/test_handlers_llm_harness_provision.py b/tests/test_handlers_llm_harness_provision.py index f10ef9494..eb7064268 100644 --- a/tests/test_handlers_llm_harness_provision.py +++ b/tests/test_handlers_llm_harness_provision.py @@ -51,7 +51,12 @@ ) from effectful.handlers.llm.harness.observability.rich import RichTerminalRenderer from effectful.handlers.llm.harness.provision.litellm import LiteLLMConfigurer -from effectful.handlers.llm.harness.serialization import _NameAndTool +from effectful.handlers.llm.harness.serialization import ( + _is_empty_text_block, + _NameAndTool, + format_as_content_blocks, + to_content_blocks, +) from effectful.handlers.llm.harness.synthesis.body import ( FinalBodySynthesizer, ) @@ -3094,6 +3099,58 @@ def _has_block_cache_control(msg: dict) -> bool: ) +def _assert_no_turn_dropped(msgs) -> None: + """Assert litellm's Anthropic transform carries every turn of `msgs` through. + + `anthropic_messages_pt` emits an assistant turn only ``if assistant_content``, + so a message with nothing in it disappears from the request silently. This + checks our own messages survive the transform; it makes no claim about which + requests Anthropic accepts. + """ + from litellm.llms.anthropic.chat.transformation import AnthropicConfig + + # A deep copy, because the transform rewrites messages in place and `msgs` + # is the captured request the caller goes on to assert against. + transformed = AnthropicConfig().transform_request( + model="claude-sonnet-4-5", + messages=json.loads(json.dumps(list(msgs))), + optional_params={}, + litellm_params={}, + headers={}, + ) + + # Consecutive same-role turns merge, so compare role runs, not counts. + def runs(roles): + out = [] + for role in roles: + if not out or out[-1] != role: + out.append(role) + return out + + sent = runs( + "user" if m["role"] in ("user", "tool") else m["role"] + for m in msgs + if m["role"] != "system" + ) + assert runs(m["role"] for m in transformed["messages"]) == sent, ( + f"a turn was dropped by the Anthropic transform: sent {sent}, " + f"got {[m['role'] for m in transformed['messages']]}" + ) + + +def _empty_text_blocks(msgs) -> list: + """Every empty text block in `msgs`, with whether it carries a breakpoint.""" + return [ + (msg["role"], "cache_control" in block) + for msg in msgs + if isinstance(msg.get("content"), list) + for block in msg["content"] + if isinstance(block, dict) + and block.get("type") == "text" + and not block.get("text") + ] + + class CachingAgent(Agent): """A test agent with persistent history.""" @@ -3215,6 +3272,7 @@ def test_exactly_one_breakpoint_beyond_the_system_message(self): assert [m["role"] for m in marked] == ["system", "tool"], ( f"Expected the system message plus the last input message. Got: {marked}" ) + _assert_no_turn_dropped(msgs) def test_breakpoint_advances_to_the_newest_message(self): """The breakpoint tracks the end of the conversation across turns, so @@ -3245,6 +3303,7 @@ def test_breakpoint_advances_to_the_newest_message(self): assert marked == [0, len(second) - 1], ( f"Expected the system message and the last message only. Got: {marked}" ) + _assert_no_turn_dropped(second) def test_cache_control_never_enters_stored_history(self): """The annotation is added to the outgoing request, not the transcript, @@ -3375,6 +3434,260 @@ def test_litellm_strips_cache_control_for_openai(self): assert "cache_control" not in block +# ============================================================================ +# Empty content blocks +# +# Regression tests for GitHub issue #762. +# ============================================================================ + + +@Tool.define +def silent_tool() -> str: + """A tool whose output is empty -- an empty file, a search with no hits.""" + return "" + + +@Skill.define +def use_silent_tool() -> str: + """Consult the note and report what it said.""" + raise NotHandled + + +class TestEmptyContentBlocks: + """No message the harness builds may carry an empty text block.""" + + @staticmethod + def _consult_the_note(): + capture = MockCompletionHandler( + [ + make_tool_call_response("silent_tool", "{}"), + make_text_response("it was empty"), + ] + ) + # `capture` is terminal, so it goes below the provider: the breakpoint is + # only added if `LiteLLMConfigurer.completion` runs and forwards into it. + with ( + handler(capture), + handler(AgentLoop()), + handler(LexicalToolExtractor()), + handler(LiteLLMConfigurer(model="claude-sonnet-4-5")), + handler(HistoryBuilder()), + ): + use_silent_tool() + return capture + + def test_empty_tool_result_carries_no_empty_block(self): + """A tool returning ``""`` used to encode to a single empty text block, + in the one place `_add_cache_control` puts its second breakpoint.""" + sent = self._consult_the_note().received_messages[-1] + + assert _empty_text_blocks(sent) == [], ( + f"empty text block(s) in the request: {_empty_text_blocks(sent)}" + ) + _assert_no_turn_dropped(sent) + + def test_breakpoint_moves_off_an_empty_tool_result(self): + """With no block to mark, the breakpoint falls back to the previous + message rather than being dropped.""" + sent = self._consult_the_note().received_messages[-1] + + marked = [m["role"] for m in sent if _has_cache_control(m)] + assert marked == ["system", "user"], ( + f"expected the breakpoint to fall back to the user message. Got: {marked}" + ) + + def test_a_value_that_merely_contains_an_empty_string_is_unchanged(self): + """`to_content_blocks` still satisfies its linearization law: an empty + string inside an encoded value keeps its JSON quotes.""" + assert to_content_blocks("") == [] + assert to_content_blocks({"a": ""}) == [{"type": "text", "text": '{"a": ""}'}] + assert to_content_blocks([]) == [{"type": "text", "text": "[]"}] + + @pytest.mark.parametrize( + "value", ["", " ", "x", {}, [], {"a": ""}, {"a": [1, ""]}, 0, None] + ) + def test_to_content_blocks_agrees_with_is_empty_text_block(self, value): + """The invariant `HistoryBuilder.append_message` asserts.""" + assert not any(_is_empty_text_block(b) for b in to_content_blocks(value)) + + @pytest.mark.parametrize( + ("template", "expected"), + [ + ("a{x}b", "ab"), + ("a{x!r}b", "a''b"), + ("a{x:>5}b", "a b"), + ("a{x!r:>6}b", "a ''b"), + ], + ) + def test_a_conversion_still_runs_on_an_empty_value(self, template, expected): + """A conversion or format spec can turn an empty value into something.""" + assert format_as_content_blocks(template, {"x": ""}) == [ + {"type": "text", "text": expected} + ] + + def test_a_hole_that_formats_to_nothing_produces_no_block(self): + assert format_as_content_blocks("{x}", {"x": ""}) == [] + + def test_empty_block_never_enters_stored_history(self): + """`SQLitePersister` checkpoints the transcript, so a block repaired + only on the way out would still be durable.""" + capture = MockCompletionHandler( + [ + make_tool_call_response("silent_tool", "{}"), + make_text_response("it was empty"), + ] + ) + agent = CachingAgent() + + with ( + handler(capture), + handler(AgentLoop()), + handler(LexicalToolExtractor()), + handler(LiteLLMConfigurer(model="claude-sonnet-4-5")), + handler(HistoryBuilder()), + ): + agent.ask("what does the note say?") + + assert _empty_text_blocks(agent.__history__) == [] + + def test_exec_code_with_no_output_is_sendable(self): + """`exec_code` returns the empty string for a snippet that printed + nothing, and is in the default `harness()` stack.""" + + def run_silent(exec_code): + bound_args = inspect.signature(exec_code).bind( + pydantic.TypeAdapter(Encodable[CodeType]).validate_python("x = 1 + 1") + ) + tc = DecodedToolCall(exec_code, bound_args, "call_exec", "exec_code") + return call_tool(tc)[0] + + msg = _drive_repl(run_silent) + assert msg["role"] == "tool" + assert _empty_text_blocks([msg]) == [], ( + f"exec_code produced an empty block: {msg}" + ) + + def test_call_user_never_emits_an_empty_block(self): + """`call_user` and `call_system` render through `_render_prompt_section`, + which drops empty text.""" + + @Skill.define + def ask_about(topic: str, note: str) -> str: + """Say something about {topic}. {note}""" + raise NotHandled + + capture = MockCompletionHandler([make_text_response("ok")]) + with ( + handler(capture), + handler(AgentLoop()), + handler(LexicalToolExtractor()), + handler(LiteLLMConfigurer(model="claude-sonnet-4-5")), + handler(HistoryBuilder()), + ): + # Both holes encode to "", so the rendered prompt ends on one. + ask_about("", "") + + sent = capture.received_messages[0] + assert [m["role"] for m in sent] == ["system", "user"] + assert _empty_text_blocks(sent) == [] + _assert_no_turn_dropped(sent) + + def test_breakpoint_skips_an_unmarkable_message(self): + """The same fallback, over messages the harness did not build.""" + provider = LiteLLMConfigurer(model="claude-sonnet-4-5") + msgs = [ + {"role": "system", "content": [{"type": "text", "text": "sys"}]}, + {"role": "user", "content": [{"type": "text", "text": "q"}]}, + {"role": "user", "content": [{"type": "text", "text": ""}]}, + ] + + marked = provider._add_cache_control(msgs) + + assert [_has_block_cache_control(m) for m in marked] == [True, True, False], ( + f"breakpoint should fall back to the previous message. Got: {marked}" + ) + + def test_whitespace_is_content(self): + """A block of spaces is not empty, so it keeps its block.""" + provider = LiteLLMConfigurer(model="claude-sonnet-4-5") + blank = {"role": "user", "content": [{"type": "text", "text": " "}]} + + assert to_content_blocks(" ") == [{"type": "text", "text": " "}] + assert _empty_text_blocks([blank]) == [] + assert _has_block_cache_control(provider._mark(blank)) + + +class TestEmptyReply: + """A reply with nothing to decode is retried, and reports the finish_reason.""" + + @staticmethod + def _response(content, finish_reason="stop", **extra): + return ModelResponse( + id="test", + choices=[ + { + "index": 0, + "message": {"role": "assistant", "content": content, **extra}, + "finish_reason": finish_reason, + } + ], + model="test-model", + ) + + @pytest.mark.parametrize("content", [None, ""]) + def test_contentless_reply_names_the_finish_reason(self, content): + """The finish_reason is the only thing that says why the reply is empty.""" + capture = MockCompletionHandler( + [self._response(content, finish_reason="length")] + ) + + with ( + handler(capture), + handler(AgentLoop()), + handler(LexicalToolExtractor()), + handler(LiteLLMConfigurer(model="claude-sonnet-4-5")), + handler(HistoryBuilder()), + ): + with pytest.raises(ResultDecodingError, match="finish_reason='length'"): + simple_prompt("test") + + @pytest.mark.parametrize("content", [None, "", " "]) + def test_an_empty_reply_is_retried_like_any_other_bad_output(self, content): + capture = MockCompletionHandler( + [self._response(content), make_text_response(json.dumps({"value": 4}))] + ) + + # `TenacityRetryer` goes inside `HistoryBuilder`: it appends the reply + # that finally succeeded itself, so wrapping the other way records it + # twice. + with ( + handler(capture), + handler(AgentLoop()), + handler(LexicalToolExtractor()), + handler(LiteLLMConfigurer(model="claude-sonnet-4-5")), + handler(HistoryBuilder()), + handler(TenacityRetryer(stop=tenacity.stop_after_attempt(3))), + ): + assert generate_number(10) == 4 + + assert capture.call_count == 2, "the empty reply should have been retried" + + def test_a_reply_carrying_only_reasoning_content_still_decodes(self): + """The `content or reasoning_content` fallback is untouched.""" + capture = MockCompletionHandler( + [self._response("", reasoning_content=json.dumps({"value": 7}))] + ) + + with ( + handler(capture), + handler(AgentLoop()), + handler(LexicalToolExtractor()), + handler(LiteLLMConfigurer(model="claude-sonnet-4-5")), + handler(HistoryBuilder()), + ): + assert generate_number(10) == 7 + + # ============================================================================ # Scoping a call to a different model # ============================================================================