diff --git a/docs/concepts/strategies.md b/docs/concepts/strategies.md index 96046609e..b0c2f019e 100644 --- a/docs/concepts/strategies.md +++ b/docs/concepts/strategies.md @@ -54,6 +54,57 @@ variables, helper definitions persist across cells, and generated code can call visible methods and tools on `self`. The loop ends when a value validates against the return annotation. +### Recovering from a text-only CodeAct turn + +CodeAct expects every model turn to call `execute_python` or `return_result`, +but models sometimes emit plain prose instead. NOOA keeps that original +assistant turn in history, then asks a configurable callback what to do: + +```python +from nooa import CodeActStrategy, return_text_as_result, strategy + + +class Summarizer(Agent, llm=llm): + @strategy(CodeActStrategy(on_text_only=return_text_as_result)) + async def summarize(self, text: str) -> str: + """Summarize the text.""" + ... +``` + +`TextOnlyResponseAction` is the callback's return value; it is not passed to +`@strategy`. The wiring is: + +1. `@strategy(...)` attaches a `CodeActStrategy` to the method. +2. `CodeActStrategy(on_text_only=handler)` registers a sync or async callback. +3. When a text-only response occurs, the handler receives a + `TextOnlyResponseContext` and returns a `TextOnlyResponseAction`. + +The default handler, `retry_text_only_response`, appends a model-visible +`Error` telling the model to use one of its tools and retries. The opt-in +`return_text_as_result` handler validates the bare text against the method's +return type. Custom handlers can return `TextOnlyResponseAction.return_result`, +`.retry`, or `.tool_calls`; all paths preserve the provider's original turn. + +For example, an application can supply its own model-visible correction: + +```python +from nooa import TextOnlyResponseAction, TextOnlyResponseContext +from nooa.events import Error + + +def require_tool(context: TextOnlyResponseContext) -> TextOnlyResponseAction: + return TextOnlyResponseAction.retry( + Error(content=f"Plain text cannot finish {context.call.method_name}; call a tool.") + ) + + +class Investigator(Agent, llm=llm): + @strategy(CodeActStrategy(on_text_only=require_tool)) + async def investigate(self, question: str) -> str: + """Investigate the question.""" + ... +``` + ## Strategy selection is not model selection The strategy controls the interaction pattern. The LLM setting controls which diff --git a/packages/nooa-bench/src/nooa_bench/bench_agent.py b/packages/nooa-bench/src/nooa_bench/bench_agent.py index 2a0d17642..d324e3f6f 100644 --- a/packages/nooa-bench/src/nooa_bench/bench_agent.py +++ b/packages/nooa-bench/src/nooa_bench/bench_agent.py @@ -206,13 +206,7 @@ async def _run_evaluation(self, task_input: dict) -> dict: _logger.error("BenchAgent failed: %s", e) return {"response": "", "success": False, "error": str(e)} - @strategy( - CodeActStrategy( - config=CodeActConfig( - max_iterations=300, max_retries=10, text_only_stop_behavior="synthetic_comment" - ) - ) - ) + @strategy(CodeActStrategy(config=CodeActConfig(max_iterations=300, max_retries=10))) async def _solve_task(self, description: str) -> TaskResult: """Solve the task. diff --git a/skills/nooa-agent-authoring/SKILL.md b/skills/nooa-agent-authoring/SKILL.md index 9fa2375ba..f0834bb5f 100644 --- a/skills/nooa-agent-authoring/SKILL.md +++ b/skills/nooa-agent-authoring/SKILL.md @@ -195,7 +195,16 @@ async def classify(self, text: str) -> Intent: ... async def implement(self, task: str) -> str: ... ``` -**Constructors take `config=` only.** `PredictStrategy(max_retries=3)` and `CodeActStrategy(max_iterations=10)` are errors — wrap options in `PredictConfig(...)`/`CodeActConfig(...)`. Useful `CodeActConfig` fields: `max_iterations`, `max_retries`, `cell_timeout`, `max_tokens`, `temperature`, `max_consecutive_text_only`, `restrictions`. +**Configuration fields go through `config=`.** `PredictStrategy(max_retries=3)` and `CodeActStrategy(max_iterations=10)` are errors — wrap options in `PredictConfig(...)`/`CodeActConfig(...)`. Strategy-level extension points such as `CodeActStrategy(on_text_only=...)` remain direct constructor arguments. Useful `CodeActConfig` fields: `max_iterations`, `max_retries`, `cell_timeout`, `max_tokens`, `temperature`, `max_consecutive_text_only`, `restrictions`. + +CodeAct preserves a model response that contains prose but no tool call. By +default it appends an `Error` asking the model to use `execute_python` or +`return_result`, then retries. For a method where prose is a valid final value, +use `CodeActStrategy(on_text_only=return_text_as_result)`. `on_text_only` +receives a sync or async callback; the callback receives +`TextOnlyResponseContext` and returns `TextOnlyResponseAction`. The action is +the callback result—it is not passed to `@strategy`. See +`nooa-codeact-advanced` for custom retry and synthetic-tool examples. `max_iterations` is a safety net, not the main tuning dial — decompose the task instead of raising the cap. For prefill control, truncation tuning, code restrictions, and the full config surface, see `nooa-codeact-advanced`. diff --git a/skills/nooa-codeact-advanced/SKILL.md b/skills/nooa-codeact-advanced/SKILL.md index 995e45e37..76b702c85 100644 --- a/skills/nooa-codeact-advanced/SKILL.md +++ b/skills/nooa-codeact-advanced/SKILL.md @@ -1,6 +1,6 @@ --- name: nooa-codeact-advanced -description: Advanced tuning of NOOA strategies — CodeAct prefill (understanding, disabling, custom, pre-ellipsis code), loop guards (max_iterations, retries, text-only stop), truncation tuning (TruncationConfig/CaptureConfig/FormatConfig), code restrictions (RestrictionsConfig), execution environment internals, and PredictStrategy tuning (retries, param guards, output_serialization). Use when configuring CodeActConfig or PredictConfig beyond defaults, writing a custom prefill, restricting generated code, or debugging truncation/eviction behavior. +description: Advanced tuning of NOOA strategies — CodeAct prefill, loop guards, text-only recovery callbacks, truncation, code restrictions, execution internals, and PredictStrategy tuning. Use when configuring CodeActConfig or PredictConfig beyond defaults, handling models that return prose instead of tool calls, writing a custom prefill, restricting generated code, or debugging truncation and eviction. compatibility: nooa package --- @@ -10,7 +10,7 @@ The authoring basics are in `nooa-agent-authoring`. This skill covers the deep c ## Config plumbing rules (read first) -- Strategy constructors take **`config=` only**: `CodeActStrategy(config=CodeActConfig(...))`, `PredictStrategy(PredictConfig(...))`. Flat kwargs (`CodeActStrategy(max_iterations=10)`, `CodeActStrategy(prefill=...)`) do not exist, despite some docstring examples. +- Configuration fields go through **`config=`**: `CodeActStrategy(config=CodeActConfig(...))`, `PredictStrategy(PredictConfig(...))`. Flat config kwargs (`CodeActStrategy(max_iterations=10)`, `CodeActStrategy(prefill=...)`) do not exist. Strategy-level extension points such as `CodeActStrategy(on_text_only=...)` and `CodeActStrategy(error_formatter=...)` are separate keyword arguments. - All config objects are frozen Pydantic models with `merge_with(other)`: only fields explicitly set on `other` override. **Configs must be freshly constructed** — anything round-tripped through `model_dump()`/`model_validate()` has an empty `model_fields_set` and `merge_with` raises. - Truncation layers: framework default → `Agent` class kwarg `truncation=` → instance kwarg → `@strategy(..., truncation=...)` per method. `TruncationConfig.merge_with` deep-merges sub-configs field-by-field; the strategy configs merge flat. @@ -42,7 +42,6 @@ class Notifier(Agent, llm=llm): | `max_iterations` | `None` | **Unlimited.** The loop then stops only on completion, the error budget, or a hard abort. | | `max_retries` | `3` | **Cumulative session error budget, not consecutive** (the counter is never reset). LLM API errors, bad tool JSON, empty code, and `return_result` validation failures all count. | | `max_consecutive_text_only` | `3` | Consecutive no-tool-call text replies before hard abort; `0` disables. Any real tool call resets the counter. | -| `text_only_stop_behavior` | `"return_result"` | Text-only reply → try to validate the text as the final result; on failure, a visible correction `Error` is added. `"synthetic_reasoning"` instead converts the text to a no-op `reasoning(...)` cell whose tool result says the task is NOT finished. | | `cell_timeout` | `None` | Per-cell `asyncio.wait_for` limit in seconds; `None` = unlimited. Cannot interrupt a truly blocking sync syscall — that's what the blocking-call AST validation is for. | | `max_tokens` / `temperature` / `top_p` | `None` | Passed to every generation call when set (model defaults otherwise). On empty responses with `finish_reason="length"` CodeAct aborts and tells you to raise `max_tokens` (16384+ for reasoning models). | | `translate_tool_calls` | `False` | When a weak model calls an agent method directly as a tool (instead of via `execute_python`), rewrite it into equivalent code and run it — teaching the right pattern. Off = error listing the two valid tools. | @@ -50,7 +49,64 @@ class Notifier(Agent, llm=llm): | `prefill` | `InspectInputsPrefill()` | See Prefill above. | | `max_tool_calls` | `None` | **Dead — declared but never read.** Setting it does nothing. | -`tool_choice` is hardcoded `"auto"`. There is no `allow_text_response` option (older docs mention one) — text handling is entirely the two text-only knobs. +`tool_choice` is hardcoded `"auto"`. + +## Text-only recovery callback + +CodeAct expects each model turn to call `execute_python` or `return_result`. +When a model emits only prose, NOOA preserves that exact assistant turn and +then invokes the strategy's `on_text_only` callback. + +Do not pass `TextOnlyResponseAction` to `@strategy`. The objects have distinct +roles: + +```text +@strategy attaches CodeActStrategy to a method + -> CodeActStrategy(on_text_only=handler) registers the callback + -> handler(TextOnlyResponseContext) returns TextOnlyResponseAction +``` + +The callback may be synchronous or asynchronous. Its context contains the +untouched `LLMResponse`, normalized text, current method call, and declared +return type. Its action chooses one of three append-only recovery paths: + +| Action constructor | Effect | +|---|---| +| `TextOnlyResponseAction.return_result(value)` | Validate `value` through CodeAct's normal result path. Invalid values produce model-visible correction feedback. | +| `TextOnlyResponseAction.retry(*events)` | Append feedback events, then ask the model again. | +| `TextOnlyResponseAction.tool_calls(*calls)` | Run synthetic calls while retaining the original assistant turn. Use `execute_python`, not TUI-only tools. | + +The safe default is `retry_text_only_response`: it appends an `Error` asking +the model to call `return_result(value)` or `execute_python(code)`, then retries. +Opt into accepting prose only when that is appropriate for the method contract: + +```python +from nooa import Agent, CodeActStrategy, return_text_as_result, strategy + + +class Summarizer(Agent, llm=llm): + @strategy(CodeActStrategy(on_text_only=return_text_as_result)) + async def summarize(self, text: str) -> str: + """Summarize the text.""" + ... +``` + +For custom policy, return an action from the callback: + +```python +from nooa import TextOnlyResponseAction, TextOnlyResponseContext +from nooa.events import Error + + +def require_tool(context: TextOnlyResponseContext) -> TextOnlyResponseAction: + return TextOnlyResponseAction.retry( + Error(content=f"Plain text cannot finish {context.call.method_name}; call a tool.") + ) + + +@strategy(CodeActStrategy(on_text_only=require_tool)) +async def investigate(self, question: str) -> str: ... +``` ## `return_result` mechanics diff --git a/src/nooa/__init__.py b/src/nooa/__init__.py index 78e5556f8..0b5a21036 100644 --- a/src/nooa/__init__.py +++ b/src/nooa/__init__.py @@ -71,7 +71,12 @@ GenerationStrategy, InspectInputsPrefill, PredictStrategy, + TextOnlyResponseAction, + TextOnlyResponseContext, + TextOnlyResponseHandler, get_default_strategy, + retry_text_only_response, + return_text_as_result, set_default_strategy, ) from nooa.strategy_validation import ( # noqa: E402 @@ -119,6 +124,11 @@ def __getattr__(name): # Strategies "GenerationStrategy", "CodeActStrategy", + "TextOnlyResponseAction", + "TextOnlyResponseContext", + "TextOnlyResponseHandler", + "retry_text_only_response", + "return_text_as_result", "CodeActLiteStrategy", "ReflexionStrategy", "PredictStrategy", diff --git a/src/nooa/config/strategy_config.py b/src/nooa/config/strategy_config.py index 1bfa8a75c..d63766e0b 100644 --- a/src/nooa/config/strategy_config.py +++ b/src/nooa/config/strategy_config.py @@ -2,10 +2,10 @@ # SPDX-License-Identifier: Apache-2.0 """Strategy configuration for CodeAct, Predict, and Reflexion strategies.""" -from collections.abc import Sequence +from collections.abc import Mapping, Sequence from typing import TYPE_CHECKING, Any, Literal -from pydantic import BaseModel, ConfigDict, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator from nooa.runtime.restrictions import RestrictionsConfig from nooa.runtime.sandbox.config import SandboxConfig @@ -41,26 +41,31 @@ class CodeActConfig(BaseModel): max_retries: int = 3 # Maximum consecutive turns where the LLM returns plain text instead of a # tool call before the run is aborted. A real tool call resets the counter. - # Set to 0 to disable the guard (legacy behavior). See also - # text_only_stop_behavior for how each text-only response is handled. + # Set to 0 to disable the guard. max_consecutive_text_only: int = 3 - # How to handle finish_reason="stop" (text-only, no tool call) responses: - # - "return_result": Route through return_result(content) validation. If the - # return type matches, the session terminates cleanly. If not, the LLM gets - # an actionable validation error to self-correct. (Recommended — breaks - # loops faster and often terminates successfully.) - # - "synthetic_comment": Convert to an execute_python call whose code is the - # text as a `#` comment — a no-op synthetic call that preserves the text - # in traces. The LLM sees "status: complete" and must still call - # return_result() explicitly. - text_only_stop_behavior: Literal["return_result", "synthetic_comment"] = "return_result" - - @field_validator("text_only_stop_behavior", mode="before") + + @model_validator(mode="before") @classmethod - def _migrate_synthetic_reasoning(cls, v: str) -> str: - if v == "synthetic_reasoning": - return "synthetic_comment" - return v + def _reject_removed_text_only_options(cls, value: Any) -> Any: + """Fail loudly when configuration uses the superseded recovery API.""" + if not isinstance(value, Mapping): + return value + + removed = sorted( + { + "text_only_stop_behavior", + "text_only_correction", + "text_only_correction_fn", + }.intersection(value) + ) + if removed: + fields = ", ".join(repr(field) for field in removed) + raise ValueError( + f"CodeActConfig field(s) {fields} were removed. Pass recovery behavior " + "to CodeActStrategy(on_text_only=...) instead; use the default retry, " + "return_text_as_result, or a callback returning TextOnlyResponseAction." + ) + return value cell_timeout: float | None = None max_tokens: int | None = None diff --git a/src/nooa/events.py b/src/nooa/events.py index fa33b7f6b..ca371e688 100644 --- a/src/nooa/events.py +++ b/src/nooa/events.py @@ -126,11 +126,7 @@ class TextOnlyReply(EventBase): # type: ignore[misc] but ``Role.METADATA`` — never rendered to the model, so capturing a text-only reply does not change generation. This is the durable, structured record that powers ``/bug`` capture and time-travel replay; the model-visible - correction is a separate ``Error``/``Feedback`` event (``Role.USER``). - - Replaces the lossy ``DebugTrace`` previously written on the CodeAct - text-only path, which truncated the content and could not be relied on by - downstream consumers. + recovery action is recorded separately and may be supplied by the host. """ _role: ClassVar[Role] = Role.METADATA @@ -141,22 +137,17 @@ class TextOnlyReply(EventBase): # type: ignore[misc] finish_reason: Annotated[ str, Field(description="LLM finish_reason for the text-only turn (e.g. 'stop')") ] = "" - route: Annotated[ + handler: Annotated[ + str, + Field(description="Qualified name of the text-only response handler"), + ] = "" + action: Annotated[ str, - Field(description="Handling route: 'return_result' or 'synthetic_comment'"), + Field(description="Handler action: 'return_result', 'retry', or 'tool_calls'"), ] = "" consecutive_text_only: Annotated[ int, Field(description="Count of consecutive text-only turns including this one") ] = 0 - recovered: Annotated[ - bool, - Field( - description=( - "Set True on a later turn if the model issued a real tool call " - "after this text-only reply (i.e. the corrective feedback worked)." - ) - ), - ] = False class LLMOutput(EventBase): # type: ignore[misc] diff --git a/src/nooa/runtime/context_builder.py b/src/nooa/runtime/context_builder.py index f3b538a8f..a5f9d68fd 100644 --- a/src/nooa/runtime/context_builder.py +++ b/src/nooa/runtime/context_builder.py @@ -30,6 +30,7 @@ ResolvedBlock, Role, ) +from nooa.events import LLMOutput if TYPE_CHECKING: from nooa.config.truncation_config import FormatConfig @@ -454,6 +455,19 @@ def _phase_events( new_blocks: list[ResolvedBlock] = [] for event in events: + # Keep empty provider turns in the event log for persistence and + # diagnostics, but do not send an empty assistant message back to an + # API. CodeAct's text-only recovery appends its feedback after this + # event, so removing only the provider-visible block preserves the + # append-only history without producing an invalid message. + if ( + isinstance(event, LLMOutput) + and not event.content + and not getattr(event, "llm_state", None) + and not getattr(event, "reasoning", None) + ): + continue + tag = event.tag if event.tag is not None else event.id event_role = getattr(event, "_role", Role.USER) meta = BlockMetadata(expr=f'self.events["{tag}"]', tag=tag) diff --git a/src/nooa/strategies/__init__.py b/src/nooa/strategies/__init__.py index 808dcce19..09d6acf3f 100644 --- a/src/nooa/strategies/__init__.py +++ b/src/nooa/strategies/__init__.py @@ -9,7 +9,14 @@ from nooa.config import CodeActConfig from nooa.strategies.base import GenerationStrategy, RuntimeServices -from nooa.strategies.codeact import CodeActStrategy +from nooa.strategies.codeact import ( + CodeActStrategy, + TextOnlyResponseAction, + TextOnlyResponseContext, + TextOnlyResponseHandler, + retry_text_only_response, + return_text_as_result, +) from nooa.strategies.codeact_lite import CodeActLiteStrategy from nooa.strategies.composite import CompositeStrategy from nooa.strategies.current_call import CurrentCall @@ -91,6 +98,11 @@ def set_default_strategy(strategy: GenerationStrategy | None) -> None: "CompositeStrategy", "TemplateStrategy", "CodeActStrategy", + "TextOnlyResponseAction", + "TextOnlyResponseContext", + "TextOnlyResponseHandler", + "retry_text_only_response", + "return_text_as_result", "CodeActLiteStrategy", "ReflexionStrategy", "PredictStrategy", diff --git a/src/nooa/strategies/codeact.py b/src/nooa/strategies/codeact.py index c76f82586..7459aa509 100644 --- a/src/nooa/strategies/codeact.py +++ b/src/nooa/strategies/codeact.py @@ -21,13 +21,14 @@ import json import logging import types -from collections.abc import AsyncIterator, Iterator +from collections.abc import AsyncIterator, Awaitable, Callable, Iterator from contextlib import asynccontextmanager from dataclasses import dataclass, field, replace from typing import ( TYPE_CHECKING, Annotated, Any, + Literal, get_args, get_origin, ) @@ -37,7 +38,7 @@ from pydantic import ValidationError as PydanticValidationError from nooa.agentdoc._structured import format_type as _format_type -from nooa.context_blocks import DynamicContext, ResultStatus, ToolCallEvent, ToolResult +from nooa.context_blocks import DynamicContext, EventBase, ResultStatus, ToolCallEvent, ToolResult from nooa.context_blocks.exceptions import BlockSyntaxError from nooa.decorators import strategy from nooa.errors import GenerationError @@ -68,7 +69,7 @@ run_postconditions, run_preconditions, ) -from nooa.unifiedllm import Tool, ToolCall +from nooa.unifiedllm import LLMResponse, Tool, ToolCall if TYPE_CHECKING: from nooa.config.strategy_config import CodeActConfig @@ -79,11 +80,117 @@ logger = logging.getLogger(__name__) +@dataclass(frozen=True) +class TextOnlyResponseContext: + """Input passed to a CodeAct text-only response handler. + + ``CodeActStrategy`` creates this context only when the model returns no + tool call. The original response has already been preserved in history. + """ + + response: LLMResponse + content: str + call: "CurrentCall" + return_type: Any + + +@dataclass(frozen=True) +class TextOnlyResponseAction: + """Decision returned by a CodeAct text-only response handler. + + This is not a strategy or decorator argument. Pass a sync or async callback + as ``CodeActStrategy(on_text_only=handler)``; that callback receives a + :class:`TextOnlyResponseContext` and returns one of these actions. + """ + + kind: Literal["return_result", "retry", "tool_calls"] + value: Any = None + events: tuple[EventBase, ...] = () + calls: tuple[ToolCall, ...] = () + + @classmethod + def return_result(cls, value: Any) -> "TextOnlyResponseAction": + """Validate *value* using CodeAct's normal return-result path.""" + return cls(kind="return_result", value=value) + + @classmethod + def retry(cls, *events: EventBase) -> "TextOnlyResponseAction": + """Append model-visible feedback events, then ask the model again.""" + return cls(kind="retry", events=events) + + @classmethod + def tool_calls(cls, *calls: ToolCall) -> "TextOnlyResponseAction": + """Process synthetic tool calls while retaining the original model turn.""" + if not calls: + raise ValueError("TextOnlyResponseAction.tool_calls() needs at least one call") + return cls(kind="tool_calls", calls=calls) + + +type TextOnlyResponseHandler = Callable[ + [TextOnlyResponseContext], + TextOnlyResponseAction | Awaitable[TextOnlyResponseAction], +] + + +def return_text_as_result(context: TextOnlyResponseContext) -> TextOnlyResponseAction: + """Opt-in handler that validates non-empty text (or ``None``) as the result.""" + value = context.content if context.content.strip() else None + return TextOnlyResponseAction.return_result(value) + + +def retry_text_only_response(context: TextOnlyResponseContext) -> TextOnlyResponseAction: + """Default handler that returns a model-visible tool-use correction.""" + return TextOnlyResponseAction.retry(_text_only_correction(context)) + + +def _text_only_correction( + context: TextOnlyResponseContext, + validation_error: str | None = None, +) -> Error: + validation_feedback = ( + f"\n\nThe attempted result was invalid:\n{validation_error}" if validation_error else "" + ) + return Error( + content=( + "Your last reply was plain text with no tool call. It was preserved, " + "but a bare message cannot end the turn or run code. " + f"To finish `{context.call.method_name}`, call `return_result(value)`. " + "To do more work, call `execute_python(code)`. " + "Re-issue your response now as one of those tool calls." + f"{validation_feedback}" + ) + ) + + +def _handler_name(handler: TextOnlyResponseHandler) -> str: + return getattr(handler, "__qualname__", type(handler).__qualname__) + + +def _response_debug_details(response: LLMResponse) -> str: + """Summarize an incomplete provider response for internal diagnostics.""" + parts = [ + f"finish_reason={response.finish_reason!r}", + f"content={response.content!r}", + f"tool_calls={response.tool_calls!r}", + ] + raw = getattr(response, "raw_response", None) + if raw is not None and (output := getattr(raw, "output", None)) is not None: + if isinstance(output, (list, tuple)): + item_types = [ + item.get("type") if isinstance(item, dict) else getattr(item, "type", None) + for item in output + ] + parts.append(f"raw_response.output_count={len(output)}; output_types={item_types!r}") + else: + parts.append(f"raw_response.output_type={type(output).__name__!r}") + return "; ".join(parts) + + # Small, deterministic expression subset accepted inside constructor-string # arguments. The values supplied to these callables have already been reduced # to plain data by ``_safe_constructor_arg``; callbacks and object attributes # therefore cannot cross into this compatibility path. -_SAFE_CONSTRUCTOR_CALLS = { +_SAFE_CONSTRUCTOR_CALLS: dict[str, Callable[..., Any]] = { "abs": abs, "all": all, "any": any, @@ -318,6 +425,12 @@ def analyze(self, data: str) -> AnalysisResult: def quick_task(self, x: int) -> dict: '''Task with custom iteration limit.''' ... + + # Opt in when a bare prose response is a valid final result. + @strategy(CodeActStrategy(on_text_only=return_text_as_result)) + def summarize(self, text: str) -> str: + '''Summarize the text.''' + ... """ def __init__( @@ -325,6 +438,7 @@ def __init__( config: "CodeActConfig | None" = None, *, error_formatter: "ErrorFormatter | None" = None, + on_text_only: TextOnlyResponseHandler = retry_text_only_response, ): """Initialize CodeAct strategy. @@ -334,6 +448,14 @@ def __init__( error_formatter: Custom error formatter for LLM feedback. It must implement ``format(error, code=None, *, line_offset=0, max_error=None, tail_chars=None)``. + on_text_only: Callback that chooses how to recover when the model + returns text without a tool call. It receives a + ``TextOnlyResponseContext`` and returns (or awaits to) a + ``TextOnlyResponseAction``. The default preserves the model + turn, appends an ``Error`` asking it to use ``execute_python`` + or ``return_result``, and retries. Use + ``return_text_as_result`` to opt into validating bare text as + the method result. Note: Prefill is always enabled and uses InspectInputsPrefill internally. @@ -342,6 +464,7 @@ def __init__( self.config = config or _CC() self.error_formatter = error_formatter + self.on_text_only = on_text_only def _build_sampling_kwargs(self) -> dict[str, Any]: """Build sampling kwargs for llm calls, excluding None values.""" @@ -629,47 +752,6 @@ async def _tool_use_reminder(self, runtime: RuntimeServices, reason: str) -> str """{reason} Use `execute_python(code)` to run code, or `return_result(...)` to submit your answer.""" ... - @staticmethod - def _add_text_only_correction(runtime: RuntimeServices, call: "CurrentCall") -> None: - """Add a model-visible correction after a text-only turn. - - Mirrors PredictStrategy's validation-retry feedback (``Error``, - ``Role.USER``): instead of silently dropping the turn, tell the model - what it did and what to do, so it self-corrects on the next turn. The - consecutive-text-only backstop still aborts after repeated text-only replies. - """ - runtime.event_manager.add( - Error( - content=( - f"Your last reply was plain text with no tool call, so it was " - f"dropped — a bare message cannot end the turn or run code. " - f"To finish `{call.method_name}`, call `return_result(value)`. " - f"To do more work, call `execute_python(code)`. " - f"Re-issue your response now as one of those tool calls." - ) - ) - ) - - @staticmethod - def _mark_text_only_recovered(runtime: RuntimeServices) -> None: - """Flip the most recent unrecovered ``TextOnlyReply`` to recovered=True. - - Called when a real tool call lands after one or more text-only replies — - the correction worked. Lets capture/replay distinguish benign, - self-corrected replies from ones that needed an abort. - """ - # Flip every unrecovered text-only reply since the last real progress, - # not just the most recent — multiple consecutive ones can precede a - # single tool call, and all were rescued by it. Stop at the first - # already-recovered one (older runs are already resolved). - for tag in reversed(runtime.event_manager.keys()): - event = runtime.event_manager.get(tag) - if not isinstance(event, TextOnlyReply): - continue - if event.recovered: - break - runtime.event_manager.update(tag, recovered=True) - @strategy(TemplateStrategy()) async def _build_task_message( self, runtime: RuntimeServices, original_call: "CurrentCall" @@ -907,6 +989,41 @@ async def _run_generation( if response is None: continue + # Output-limit responses are incomplete even when they carry + # partial text. Preserve non-empty text in its LLMOutput for + # diagnostics, but never let a text-only handler accept it as + # a successful result. + if response.finish_reason == "length": + session.record_error() + if not response.content and not response.tool_calls: + get_harness_metrics().empty_response() + runtime.event_manager.remove(event_id) + runtime.event_manager.add( + DebugTrace( + content=f"Truncated response: {_response_debug_details(response)}" + ) + ) + turn_state.is_final = True + raise GenerationError( + "The model used all available output tokens before completing " + "a tool call. Increase `max_tokens` (16384 or more is often " + "needed for reasoning models such as GPT-5.5 and o-series)." + ) + + # A provider-declared error is incomplete even if it includes + # partial text. Preserve that output for diagnostics, but do + # not let a text-only handler turn it into a successful result. + if response.finish_reason == "error": + session.record_error() + if not response.content and not response.tool_calls: + get_harness_metrics().empty_response() + runtime.event_manager.remove(event_id) + runtime.event_manager.add( + DebugTrace(content=f"Failed response: {_response_debug_details(response)}") + ) + turn_state.is_final = True + raise GenerationError("The model returned an incomplete response.") + # ── Post-response cleanup (CodeAct) ────────────────────── # Intercept point: strategy-specific response transforms. # Handles text-only→synthetic, comment prepend, tool call @@ -936,8 +1053,6 @@ async def _run_generation( # A real tool call counts as progress: reset the consecutive # text-only guard (issue 185) before executing, so a single # exec mid-stream rescues the run from accidental drift. - if session.consecutive_text_only > 0: - self._mark_text_only_recovered(runtime) session.reset_text_only() result = await self._process_tool_calls( tool_calls, @@ -957,7 +1072,6 @@ async def _run_generation( continue # ── Text-only response (no tool call) ────────────────────── - # Normalize content for both branches below. _raw_content = response.content _text = ( _raw_content.model_dump_json() @@ -968,136 +1082,85 @@ async def _run_generation( ) _has_text = bool(_text.strip()) - # Route A: "return_result" mode — treat stop as a done signal - # and route through return_result() validation. Handles both - # stop+content and stop+no-content in one branch. - if ( - response.finish_reason == "stop" - and self.config.text_only_stop_behavior == "return_result" - and (_has_text or not _raw_content) - ): - session.record_iteration() - # Capture the drift faithfully for /bug + replay (recorded but - # Role.METADATA, so it never reaches the model). Replaces the - # old lossy DebugTrace; preserves the verbatim content (even - # when empty) before the offending event is removed. - drift_tag = runtime.event_manager.add( - TextOnlyReply( - content=_text, - finish_reason=str(response.finish_reason), - route="return_result", - consecutive_text_only=session.consecutive_text_only + 1, - ) - ) - runtime.event_manager.remove(event_id) - synthetic_id = f"synthetic_{uuid4().hex[:8]}" - result_value = _text if _has_text else None - synthetic_tool_call = ToolCall( - id=synthetic_id, - name="return_result", - arguments=json.dumps({"result": result_value}), - ) - get_harness_metrics().stop_to_return_result(result_value) - logger.info( - f"[CODEACT] finish_reason='stop' " - f"({'content=' + str(len(_text)) + ' chars' if _has_text else 'no content'}) " - f"→ synthetic return_result(). Routing through validation." + if _has_text or response.finish_reason == "stop": + context = TextOnlyResponseContext( + response=response, + content=_text, + call=call, + return_type=return_type, ) - result = await self._process_tool_calls( - [synthetic_tool_call], - runtime, - builtins, - session, - call, - return_type, - event_id or "", - ) - if result.completed: - # Recovered via the synthetic return_result — the drift was - # benign. Mark it so capture/replay can distinguish recovered - # drifts from ones that needed a correction. - runtime.event_manager.update(drift_tag, recovered=True) - turn_state.success = True - turn_state.is_final = True - self._sync_session_locals(call, session) - return result.final_value - # Validation failed — give the model a visible correction (the - # PredictStrategy pattern: a Role.USER event it sees on the next - # turn) instead of silently dropping the turn, then continue. - # The abort below is only a backstop for repeated non-compliance. - session.record_text_only() - self._add_text_only_correction(runtime, call) - max_text_only = self.config.max_consecutive_text_only - if max_text_only > 0 and session.consecutive_text_only >= max_text_only: - get_harness_metrics().text_only_loop_abort() - preview = _text if _has_text else "(empty)" - turn_state.is_final = True - raise GenerationError( - f"CodeAct aborted: LLM returned plain text without a tool call " - f"{session.consecutive_text_only} times in a row " - f"(max_consecutive_text_only={max_text_only}) for " - f"`{call.method_name}`. The agent likely thinks it is done — " - f"it must call `return_result(...)` to finish. " - f"Last text: {preview!r}" + action = self.on_text_only(context) + if inspect.isawaitable(action): + action = await action + if not isinstance(action, TextOnlyResponseAction): + raise TypeError( + "CodeAct on_text_only must return TextOnlyResponseAction, " + f"got {type(action).__name__}" ) - continue - - # Route B: "synthetic_comment" mode — convert text to a no-op - # execute_python comment that preserves content in traces. - elif _has_text: - session.record_iteration() - execution_count = session.record_execution() - # Capture the drift faithfully for /bug + replay (recorded but - # Role.METADATA, never shown to the model). Replaces the old - # lossy DebugTrace. + # Capture the drift faithfully for /bug reports. The original + # LLMOutput remains the assistant turn; the handler may only + # append recovery events after it. runtime.event_manager.add( TextOnlyReply( content=_text, finish_reason=str(response.finish_reason), - route="synthetic_comment", + handler=_handler_name(self.on_text_only), + action=action.kind, consecutive_text_only=session.consecutive_text_only + 1, ) ) - runtime.event_manager.remove(event_id) - synthetic_id = f"synthetic_{uuid4().hex[:8]}" - runtime.event_manager.add( - ToolCallEvent( - tool_call_id=synthetic_id, - name="execute_python", - arguments={"code": _as_comment(_text)}, - result=ToolResult( - tool_call_id=synthetic_id, - content="status: commentary only — task is NOT finished. You must call return_result() to complete.", - result_status=ResultStatus.COMPLETE, - ), - metadata={"synthetic": True, "synthetic_type": "text_response"}, + + if action.kind == "return_result": + session.record_iteration() + # This metric stores a bounded text preview. Arbitrary + # callback result values still take the normal + # validation path without being stringified by + # telemetry or passed to its string-only API. + result_preview = action.value if isinstance(action.value, str) else None + get_harness_metrics().stop_to_return_result(result_preview) + validated, validation_error = self._handle_return_result( + runtime, + {"result": action.value}, + return_type, + session, + call, ) - ) - runtime.event_manager.add( - PythonOutput( - tool_call_id=synthetic_id, - execution_count=execution_count, - execution_status=ResultStatus.COMPLETE, - metadata={"synthetic": True, "synthetic_type": "text_response"}, + if validation_error is None: + turn_state.success = True + turn_state.is_final = True + self._sync_session_locals(call, session) + return validated + runtime.event_manager.add(_text_only_correction(context, validation_error)) + elif action.kind == "retry": + session.record_iteration() + for feedback_event in action.events: + runtime.event_manager.add(feedback_event) + elif action.kind == "tool_calls": + get_harness_metrics().text_to_synthetic() + result = await self._process_tool_calls( + list(action.calls), + runtime, + builtins, + session, + call, + return_type, + event_id or "", + preserve_llm_output=True, ) - ) - get_harness_metrics().text_to_synthetic() - logger.debug( - f"[CODEACT] Text-only response ({len(_text)} chars) " - f"converted to synthetic comment." - ) - # No extra Error correction here: Route B already injects a - # synthetic execute_python comment whose ToolResult tells - # the model the task isn't finished. Adding a "your reply had no - # tool call" Error would contradict that synthetic tool call and - # confuse the model. The backstop below still aborts on repeated - # non-compliance. + if result.completed: + turn_state.success = True + turn_state.is_final = True + self._sync_session_locals(call, session) + return result.final_value + else: + raise ValueError(f"Unknown text-only action: {action.kind!r}") + session.record_text_only() max_text_only = self.config.max_consecutive_text_only if max_text_only > 0 and session.consecutive_text_only >= max_text_only: get_harness_metrics().text_only_loop_abort() - preview = _text + preview = _text if _has_text else "(empty)" turn_state.is_final = True raise GenerationError( f"CodeAct aborted: LLM returned plain text without a tool call " @@ -1113,33 +1176,11 @@ async def _run_generation( get_harness_metrics().empty_response() session.record_error() # Capture raw LLM response for debugging before removing the event - _debug_parts = [ - f"finish_reason={response.finish_reason!r}", - f"content={response.content!r}", - f"tool_calls={response.tool_calls!r}", - ] - raw = getattr(response, "raw_response", None) - if raw is not None: - output = getattr(raw, "output", None) - if output is not None: - _debug_parts.append(f"raw_response.output={output!r}") runtime.event_manager.add( - DebugTrace(content=f"Empty response: {'; '.join(_debug_parts)}") + DebugTrace(content=f"Empty response: {_response_debug_details(response)}") ) # Remove the empty assistant event - APIs reject empty content runtime.event_manager.remove(event_id) - # Reasoning models that exhaust max_tokens produce empty output. - # Retrying won't help — abort immediately with an actionable message. - if response.finish_reason == "length": - turn_state.is_final = True - raise GenerationError( - "Empty response: the model used all available output tokens " - "on reasoning and had none left for a tool call. " - "This typically means `max_tokens` is too low for a " - "reasoning model (e.g. GPT-5.5, o-series). " - "Increase `max_tokens` in the model config " - "(16384+ recommended for reasoning models)." - ) feedback = await self._tool_use_reminder(runtime, reason="Empty response received.") runtime.event_manager.add(Error(content=feedback)) @@ -1225,11 +1266,16 @@ async def _process_tool_calls( return_type: Any, event_id: str, reasoning_items: list[dict[str, Any]] | None = None, + preserve_llm_output: bool = False, ) -> _ToolCallsResult: """Process tool calls from a single LLM turn. Executes tool calls sequentially, stopping at the first error. Returns a _ToolCallsResult indicating whether the task completed. + + ``preserve_llm_output`` is used only for tool calls synthesized by a + text-only response handler. Real provider tool calls replace the empty + LLMOutput with their ToolCallEvent representation as before. """ # Handle tool calls - process ALL tool calls sequentially # Some LLMs return multiple tool calls in one response even when @@ -1237,9 +1283,10 @@ async def _process_tool_calls( # cell's output available to subsequent cells via session_locals. session.record_iteration() - # Remove the empty LLMOutput that runtime.generate() created - # and replace it with a proper ToolCallEvent that includes tool_calls - runtime.event_manager.remove(event_id) + if not preserve_llm_output: + # Replace the empty LLMOutput created by runtime.generate() with the + # provider's actual ToolCallEvent representation. + runtime.event_manager.remove(event_id) num_tool_calls = len(tool_calls) if num_tool_calls > 1: @@ -1298,7 +1345,7 @@ async def _process_tool_calls( # Return the final result try: validated, error_msg = self._handle_return_result( - runtime, tool_call, args, return_type, session, call + runtime, args, return_type, session, call ) except GenerationError: # _handle_return_result raises GenerationError when validation @@ -1573,7 +1620,6 @@ async def _handle_execute_python( try: validated, validation_error = self._handle_return_result( runtime, - tool_call, result.signal.result, # Extract the result dict from the signal return_type, session, @@ -1720,7 +1766,6 @@ async def _handle_execute_python( def _handle_return_result( self, runtime: RuntimeServices, - tool_call: Any, args: dict[str, Any], return_type: Any, session: CodeActSession, @@ -1787,7 +1832,7 @@ def _handle_return_result( # LLM passed direct fields (e.g., sum=100, mean=20) # Wrap them as the result value get_harness_metrics().args_normalized() - normalized_args: dict[str, Any] = {"result": args} + normalized_args = {"result": args} else: # Already has "result" key, use as-is normalized_args = args @@ -2023,7 +2068,9 @@ def _maybe_parse_json_string(self, value: Any) -> Any: return value - def _corrected_return_args(self, validated: Any, original_args: dict) -> dict: + def _corrected_return_args( + self, validated: Any, original_args: dict[str, Any] + ) -> dict[str, Any]: """Return corrected tool_call arguments showing correct JSON syntax. When coercion transformed the result (e.g. from a constructor-call @@ -2422,7 +2469,7 @@ def _render_return_type_doc(self, return_type: Any, *, max_chars: int = 1200) -> # Activate doc() adapters for installed libs (pandas, plotly, …) so the # rendering is concise rather than the library's full constructor docstring. # Idempotent and installed-gated; only runs on this (opaque-type) path. - register_all() + register_all() # type: ignore[no-untyped-call] rendered = _doc(return_type) except Exception: # noqa: BLE001 — doc() is advisory; never break tool build return None diff --git a/src/nooa/unifiedllm/unifiedllm.py b/src/nooa/unifiedllm/unifiedllm.py index 569c6b589..7251d1b81 100644 --- a/src/nooa/unifiedllm/unifiedllm.py +++ b/src/nooa/unifiedllm/unifiedllm.py @@ -1489,9 +1489,7 @@ def _map_completion_finish_reason( litellm/OpenAI report the provider's stop condition on ``raw_response.choices[0].finish_reason``. We surface ``"length"`` (output tokens exhausted) and ``"error"`` (e.g. ``content_filter``) so downstream - logic (e.g. CodeAct's max-tokens abort) can react. Callers that have already - detected tool calls should keep ``finish_reason="tool_calls"`` rather than - calling this. + logic (e.g. CodeAct's max-tokens abort) can react. """ raw = None try: @@ -1516,8 +1514,7 @@ def _map_responses_finish_reason( The Responses API reports truncation via ``status == "incomplete"`` with ``incomplete_details.reason == "max_output_tokens"`` (rather than a per-choice finish_reason). A ``status == "failed"`` response is surfaced as - ``"error"``. Callers that have already detected tool calls should keep - ``finish_reason="tool_calls"`` rather than calling this. + ``"error"``. """ status = getattr(raw_response, "status", None) @@ -1534,6 +1531,15 @@ def _map_responses_finish_reason( return "stop" +def _finish_reason_for_tool_calls( + provider_finish_reason: Literal["stop", "tool_calls", "length", "error"], +) -> Literal["tool_calls", "length", "error"]: + """Keep provider failure/truncation authoritative over parsed tool calls.""" + if provider_finish_reason in ("length", "error"): + return provider_finish_reason + return "tool_calls" + + def _extract_reasoning_and_usage(raw_response: Any) -> tuple[str | None, dict[str, int] | None]: """Extract reasoning and usage from raw LLM response.""" reasoning = None @@ -1892,7 +1898,9 @@ def _make_call(): raw_response=raw_response, content="", tool_calls=tool_calls, - finish_reason="tool_calls", + finish_reason=_finish_reason_for_tool_calls( + _map_completion_finish_reason(raw_response) + ), assistant_message=_completion_assistant_message( response_message, tool_calls=raw_tool_calls ), @@ -1911,7 +1919,9 @@ def _make_call(): raw_response=raw_response, content="", tool_calls=xml_tool_calls, - finish_reason="tool_calls", + finish_reason=_finish_reason_for_tool_calls( + _map_completion_finish_reason(raw_response) + ), assistant_message={ "role": "assistant", "content": text_content, @@ -2062,7 +2072,9 @@ async def _make_call(): raw_response=raw_response, content="", tool_calls=tool_calls, - finish_reason="tool_calls", + finish_reason=_finish_reason_for_tool_calls( + _map_completion_finish_reason(raw_response) + ), assistant_message=_completion_assistant_message( response_message, tool_calls=raw_tool_calls ), @@ -2081,7 +2093,9 @@ async def _make_call(): raw_response=raw_response, content="", tool_calls=xml_tool_calls, - finish_reason="tool_calls", + finish_reason=_finish_reason_for_tool_calls( + _map_completion_finish_reason(raw_response) + ), assistant_message={ "role": "assistant", "content": text_content, @@ -2415,7 +2429,9 @@ def _make_call(): raw_response=raw_response, content="", tool_calls=tool_calls, - finish_reason="tool_calls", + finish_reason=_finish_reason_for_tool_calls( + _map_responses_finish_reason(raw_response) + ), assistant_message={"_batch": assistant_messages}, reasoning=None, # Responses API doesn't have reasoning usage=usage, @@ -2546,7 +2562,9 @@ async def _make_call(): raw_response=raw_response, content="", tool_calls=tool_calls, - finish_reason="tool_calls", + finish_reason=_finish_reason_for_tool_calls( + _map_responses_finish_reason(raw_response) + ), assistant_message={"_batch": assistant_messages}, reasoning=None, usage=usage, diff --git a/tests/config/test_strategy_configs.py b/tests/config/test_strategy_configs.py index 6ae172ffb..80460bc90 100644 --- a/tests/config/test_strategy_configs.py +++ b/tests/config/test_strategy_configs.py @@ -35,6 +35,18 @@ def test_merge_with(self): assert merged.temperature == 0.7 assert merged.max_retries == 3 # not overridden + @pytest.mark.parametrize( + ("field", "value"), + [ + ("text_only_stop_behavior", "return_result"), + ("text_only_correction", "custom"), + ("text_only_correction_fn", lambda text: text), + ], + ) + def test_removed_text_only_options_fail_with_migration_help(self, field, value): + with pytest.raises(ValidationError, match=rf"{field}.*on_text_only"): + CodeActConfig(**{field: value}) + class TestPredictConfig: """Tests for PredictConfig defaults and merging.""" diff --git a/tests/runtime/test_context_builder.py b/tests/runtime/test_context_builder.py index b0e9380c9..958838994 100644 --- a/tests/runtime/test_context_builder.py +++ b/tests/runtime/test_context_builder.py @@ -557,6 +557,20 @@ def test_non_tool_event_carries_original_event(self): assert result[0].event is event assert result[0].content == "" # Deferred — serialized at render time + def test_empty_llm_output_is_persisted_but_not_provider_visible(self): + from nooa.events import LLMOutput + from nooa.runtime.context_builder import _phase_events + + empty = LLMOutput(content="", tag="1") + visible = LLMOutput(content="answer", tag="2") + events = [empty, visible] + em = _make_event_manager(events) + + result = _phase_events([], em) + + assert [block.event for block in result] == [visible] + assert em.values() == events + def test_current_call_query_keeps_task_event(self): """EventQuery.current_call() must keep the task so LLM gets system + task. diff --git a/tests/runtime/test_token_calibration.py b/tests/runtime/test_token_calibration.py index 08aa4d61b..80f0d67b9 100644 --- a/tests/runtime/test_token_calibration.py +++ b/tests/runtime/test_token_calibration.py @@ -9,7 +9,7 @@ import pytest -from nooa import Agent +from nooa import Agent, CodeActStrategy, return_text_as_result, strategy from nooa.context_blocks.events import ResultStatus, ToolCallEvent, ToolResult from nooa.events import Message from nooa.unifiedllm import FakeLLMClient, LLMResponse @@ -63,6 +63,7 @@ async def test_context_stats_populated_after_llm_call(self): ) class A(Agent, llm=llm): + @strategy(CodeActStrategy(on_text_only=return_text_as_result)) async def respond(self, prompt: str) -> str: """Respond to {prompt}.""" ... @@ -112,6 +113,7 @@ async def test_headline_is_raw_provider_total_no_ratio(self): ) class A(Agent, llm=llm): + @strategy(CodeActStrategy(on_text_only=return_text_as_result)) async def respond(self, prompt: str) -> str: """Respond to {prompt}.""" ... @@ -151,6 +153,7 @@ async def test_provider_response_recalibrates_tokens_per_char(self): ) class A(Agent, llm=llm): + @strategy(CodeActStrategy(on_text_only=return_text_as_result)) async def respond(self, prompt: str) -> str: """Respond to {prompt}.""" ... @@ -225,6 +228,7 @@ async def test_response_usage_overwrites_context_stats_total_tokens(self): ) class A(Agent, llm=llm): + @strategy(CodeActStrategy(on_text_only=return_text_as_result)) async def respond(self, prompt: str) -> str: """Respond to {prompt}.""" ... @@ -259,6 +263,7 @@ async def test_missing_usage_leaves_total_tokens_none(self): ) class A(Agent, llm=llm): + @strategy(CodeActStrategy(on_text_only=return_text_as_result)) async def respond(self, prompt: str) -> str: """Respond to {prompt}.""" ... diff --git a/tests/strategies/test_codeact_max_tokens_error.py b/tests/strategies/test_codeact_max_tokens_error.py index 58bf8c497..b55675bdd 100644 --- a/tests/strategies/test_codeact_max_tokens_error.py +++ b/tests/strategies/test_codeact_max_tokens_error.py @@ -3,12 +3,14 @@ """Test that empty response with finish_reason='length' raises immediately with an actionable message.""" import json +from types import SimpleNamespace import pytest -from nooa import Agent, strategy +from nooa import Agent, return_text_as_result, strategy from nooa.config import CodeActConfig from nooa.errors import GenerationError +from nooa.events import DebugTrace from nooa.strategies.codeact import CodeActStrategy from nooa.unifiedllm import FakeLLMClient, LLMResponse, ToolCall @@ -68,19 +70,72 @@ async def my_task(self) -> str: f"max_tokens error should not be an Error event (LLM-visible), got: {max_tokens_errors}" ) + @pytest.mark.asyncio + async def test_finish_reason_length_does_not_return_partial_text(self): + """A text-only handler cannot accept output truncated at max_tokens.""" + + class TestAgent(Agent, llm=_TEST_LLM): + @strategy(CodeActStrategy(on_text_only=return_text_as_result)) + async def my_task(self) -> str: + """A task.""" + ... + + agent_instance = TestAgent( + llm=FakeLLMClient( + scripted_responses=[ + _resp("truncated partial", finish_reason="length"), + ] + ) + ) + + with pytest.raises(GenerationError, match="max_tokens"): + await agent_instance.my_task() + + events = agent_instance.event_manager.values() + assert [event.content for event in events if event.event_type == "LLMOutput"] == [ + "truncated partial" + ] + assert not any(event.event_type == "TextOnlyReply" for event in events) + + @pytest.mark.asyncio + async def test_truncation_diagnostics_do_not_persist_provider_output(self): + """Debug metadata records output shape without opaque provider payloads.""" + + class TestAgent(Agent, llm=_TEST_LLM): + @strategy(CodeActStrategy()) + async def my_task(self) -> str: + """A task.""" + ... + + response = _resp("partial", finish_reason="length") + response.raw_response = SimpleNamespace( + output=[ + { + "type": "reasoning", + "encrypted_content": "must-never-enter-debug-events", + }, + {"type": "message", "content": []}, + ] + ) + agent_instance = TestAgent(llm=FakeLLMClient(scripted_responses=[response])) + + with pytest.raises(GenerationError, match="max_tokens"): + await agent_instance.my_task() + + debug = next( + event.content + for event in agent_instance.event_manager.values() + if isinstance(event, DebugTrace) + ) + assert "must-never-enter-debug-events" not in debug + assert "raw_response.output_count=2; output_types=['reasoning', 'message']" in debug + @pytest.mark.asyncio async def test_empty_response_without_length_retries_normally(self): """When empty response has finish_reason != 'length', normal retry logic applies.""" class TestAgent(Agent, llm=_TEST_LLM): - @strategy( - CodeActStrategy( - config=CodeActConfig( - max_retries=2, - text_only_stop_behavior="synthetic_comment", - ) - ) - ) + @strategy(CodeActStrategy(config=CodeActConfig(max_retries=2))) async def my_task(self) -> str: """A task.""" ... diff --git a/tests/strategies/test_codeact_strategy.py b/tests/strategies/test_codeact_strategy.py index 7ffdb984d..89c732cbe 100644 --- a/tests/strategies/test_codeact_strategy.py +++ b/tests/strategies/test_codeact_strategy.py @@ -20,7 +20,7 @@ from nooa import Agent, strategy from nooa.config import CodeActConfig from nooa.events import PythonOutput, ResultStatus -from nooa.strategies.codeact import CodeActStrategy +from nooa.strategies.codeact import CodeActStrategy, return_text_as_result from nooa.strategies.codeact_errors import ( format_validation_error, get_type_example, @@ -1158,13 +1158,18 @@ async def test_text_only_stop_response_routes_through_return_result(self): """Text-only LLM response (finish_reason=stop) routes through return_result validation. When the LLM returns finish_reason="stop" with text content, the strategy - constructs a synthetic return_result(content) tool call and routes it through - validation. If the return type matches (e.g. str), the session terminates - successfully with the content as the return value. + validates the content as a return_result value without persisting a tool + call the model did not make. If the return type matches (e.g. str), the + session terminates successfully with the content as the return value. """ class TestAgent(Agent, llm=_TEST_LLM): - @strategy(CodeActStrategy(config=CodeActConfig())) + @strategy( + CodeActStrategy( + config=CodeActConfig(), + on_text_only=return_text_as_result, + ) + ) async def think_and_answer(self) -> str: """A task that requires thinking.""" ... @@ -1185,13 +1190,12 @@ async def think_and_answer(self) -> str: events = agent_instance.event_manager.values() event_types = [e.event_type for e in events] - # The synthetic return_result ToolCallEvent should be present + # The original assistant turn is preserved; no synthetic provider tool + # exchange is added to history. + llm_outputs = [e for e in events if e.event_type == "LLMOutput"] + assert [event.content for event in llm_outputs] == ["The answer is 42."] tool_call_events = [e for e in events if e.event_type == "ToolCallEvent"] - assert len(tool_call_events) == 1 - synthetic = tool_call_events[0] - assert synthetic.name == "return_result" - assert synthetic.result is not None - assert synthetic.result.result_status == ResultStatus.COMPLETE + assert tool_call_events == [] # No error event should be added assert "Error" not in event_types @@ -1210,7 +1214,12 @@ class ThoughtModel(PydanticBaseModel): thought: str class TestAgent(Agent, llm=_TEST_LLM): - @strategy(CodeActStrategy(config=CodeActConfig())) + @strategy( + CodeActStrategy( + config=CodeActConfig(), + on_text_only=return_text_as_result, + ) + ) async def think_and_answer(self) -> str: """A task that requires thinking.""" ... @@ -1235,10 +1244,11 @@ async def think_and_answer(self) -> str: assert "I need to reason carefully here." in result events = agent_instance.event_manager.values() + llm_outputs = [e for e in events if e.event_type == "LLMOutput"] + assert len(llm_outputs) == 1 + assert "I need to reason carefully here." in llm_outputs[0].content tool_call_events = [e for e in events if e.event_type == "ToolCallEvent"] - assert len(tool_call_events) == 1 - assert tool_call_events[0].name == "return_result" - assert tool_call_events[0].result.result_status == ResultStatus.COMPLETE + assert tool_call_events == [] @pytest.mark.asyncio async def test_text_only_stop_with_typed_return_gives_validation_error(self): @@ -1251,7 +1261,12 @@ async def test_text_only_stop_with_typed_return_gives_validation_error(self): """ class TestAgent(Agent, llm=_TEST_LLM): - @strategy(CodeActStrategy(config=CodeActConfig())) + @strategy( + CodeActStrategy( + config=CodeActConfig(), + on_text_only=return_text_as_result, + ) + ) async def compute_stats(self) -> dict: """Compute statistics and return a dict.""" ... @@ -1272,21 +1287,18 @@ async def compute_stats(self) -> dict: assert result == {"mean": 42, "count": 10} events = agent_instance.event_manager.values() - # Should see: Task → synthetic return_result (with error) → real return_result (success) + # The text-only assistant turn is retained, followed by a user correction + # and the model's real return_result call. + llm_outputs = [e for e in events if e.event_type == "LLMOutput"] + assert [event.content for event in llm_outputs] == [ + "I have successfully completed the computation!" + ] + corrections = [e for e in events if e.event_type == "Error"] + assert any("attempted result was invalid" in e.content for e in corrections) tool_call_events = [e for e in events if e.event_type == "ToolCallEvent"] - assert len(tool_call_events) == 2 - - # First tool call is the synthetic return_result that failed validation - first = tool_call_events[0] - assert first.name == "return_result" - assert first.result is not None - assert first.result.result_status == ResultStatus.ERROR - assert "Invalid result" in first.result.content - - # Second tool call is the real return_result that succeeded - second = tool_call_events[1] - assert second.name == "return_result" - assert second.result.result_status == ResultStatus.COMPLETE + assert len(tool_call_events) == 1 + assert tool_call_events[0].name == "return_result" + assert tool_call_events[0].result.result_status == ResultStatus.COMPLETE @pytest.mark.asyncio async def test_stop_no_content_with_none_return_type_terminates(self): @@ -1297,7 +1309,12 @@ async def test_stop_no_content_with_none_return_type_terminates(self): """ class TestAgent(Agent, llm=_TEST_LLM): - @strategy(CodeActStrategy(config=CodeActConfig())) + @strategy( + CodeActStrategy( + config=CodeActConfig(), + on_text_only=return_text_as_result, + ) + ) async def do_side_effects(self) -> None: """Perform work via side effects.""" ... @@ -1315,18 +1332,14 @@ async def do_side_effects(self) -> None: assert result is None events = agent_instance.event_manager.values() + llm_outputs = [e for e in events if e.event_type == "LLMOutput"] + assert [event.content for event in llm_outputs] == [""] tool_call_events = [e for e in events if e.event_type == "ToolCallEvent"] - assert len(tool_call_events) == 1 - assert tool_call_events[0].name == "return_result" - assert tool_call_events[0].result.result_status == ResultStatus.COMPLETE + assert tool_call_events == [] @pytest.mark.asyncio - async def test_text_only_whitespace_response_treated_as_empty(self): - """Whitespace-only text response (no tool calls) is treated as empty, not synthetic. - - " " is truthy but str.strip() is falsy, so it should fall through to the - empty-response error handler rather than creating a synthetic comment. - """ + async def test_text_only_whitespace_response_uses_default_retry(self): + """Whitespace-only stop output uses text-only recovery without synthetic calls.""" from nooa.errors import GenerationError class TestAgent(Agent, llm=_TEST_LLM): @@ -1429,16 +1442,15 @@ async def empty_task(self) -> str: assert result == "corrected answer" - # The synthetic return_result(None) should have failed validation + # The empty assistant turn is retained and the only tool event is the + # model's successful self-correction. all_events = agent_instance.event_manager.values() + llm_outputs = [e for e in all_events if e.event_type == "LLMOutput"] + assert [event.content for event in llm_outputs] == [""] tool_call_events = [e for e in all_events if e.event_type == "ToolCallEvent"] - assert len(tool_call_events) == 2 - # First is the failed synthetic return_result(None) + assert len(tool_call_events) == 1 assert tool_call_events[0].name == "return_result" - assert tool_call_events[0].result.result_status == ResultStatus.ERROR - # Second is the successful self-correction - assert tool_call_events[1].name == "return_result" - assert tool_call_events[1].result.result_status == ResultStatus.COMPLETE + assert tool_call_events[0].result.result_status == ResultStatus.COMPLETE @pytest.mark.asyncio async def test_multiple_tool_calls_event_sequence(self): @@ -1603,15 +1615,14 @@ async def stuck_task(self) -> dict: f"Expected last text preview in message, got: {msg!r}" ) - # Should have exactly 3 synthetic return_result events (all failed validation) - # then abort fires before the 4th + # The three assistant turns remain in history without fabricated tool calls. events = agent_instance.event_manager.values() + llm_outputs = [e for e in events if e.event_type == "LLMOutput"] + assert len(llm_outputs) == 3 return_result_calls = [ e for e in events if e.event_type == "ToolCallEvent" and e.name == "return_result" ] - assert len(return_result_calls) == 3, ( - f"Expected exactly 3 return_result attempts before abort, got {len(return_result_calls)}" - ) + assert return_result_calls == [] @pytest.mark.asyncio async def test_text_only_counter_resets_on_real_tool_call(self): @@ -1687,14 +1698,7 @@ async def test_text_only_loop_abort_records_after_turn_event(self): from nooa.errors import GenerationError class TestAgent(Agent, llm=_TEST_LLM): - @strategy( - CodeActStrategy( - config=CodeActConfig( - max_consecutive_text_only=2, - text_only_stop_behavior="synthetic_comment", - ) - ) - ) + @strategy(CodeActStrategy(config=CodeActConfig(max_consecutive_text_only=2))) async def stuck(self) -> str: """Task.""" ... diff --git a/tests/strategies/test_codeact_text_only_reply.py b/tests/strategies/test_codeact_text_only_reply.py index 3ad2771f4..609102afa 100644 --- a/tests/strategies/test_codeact_text_only_reply.py +++ b/tests/strategies/test_codeact_text_only_reply.py @@ -1,23 +1,25 @@ # SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 -"""Tests for CodeAct text-only-reply capture + PredictStrategy-style recovery. - -Covers the fix for the "model returns text instead of a tool call" failure mode: -- a TextOnlyReply event is recorded (but never shown to the model), -- a model-visible Error correction is added so the model self-corrects, -- recovered=True is set when a real tool call follows, -- the consecutive-text-only backstop still aborts on repeated non-compliance. -""" +"""Tests for append-only, extensible CodeAct text-only recovery.""" import json import pytest -from nooa import Agent, strategy +from nooa import ( + Agent, + CodeActStrategy, + TextOnlyResponseAction, + return_text_as_result, + strategy, +) from nooa.config import CodeActConfig +from nooa.context_blocks import ToolCallEvent from nooa.errors import GenerationError -from nooa.events import PythonOutput -from nooa.strategies.codeact import CodeActStrategy +from nooa.events import LLMOutput, PythonOutput, TextOnlyReply +from nooa.runtime.event_manager import EventManager +from nooa.runtime.harness_metrics import HarnessMetrics +from nooa.storage import SQLiteStorageManager from nooa.unifiedllm import FakeLLMClient, LLMResponse, ToolCall _TEST_LLM = FakeLLMClient() @@ -35,197 +37,275 @@ def _resp(content="", tool_calls=None, finish_reason=None): ) -def _ret(val, cid="c_ret"): - return ToolCall(id=cid, name="return_result", arguments=json.dumps({"result": val})) - - -def _drifts(agent): - return [e for e in agent.event_manager.values() if e.event_type == "TextOnlyReply"] +def _ret(value, call_id="c_ret"): + return ToolCall( + id=call_id, + name="return_result", + arguments=json.dumps({"result": value}), + ) -def _errors(agent): - return [e for e in agent.event_manager.values() if e.event_type == "Error"] +def _events(agent, event_type): + return [event for event in agent.event_manager.values() if isinstance(event, event_type)] @pytest.mark.asyncio -async def test_text_only_drift_recorded_and_recovers(): - """A text-only stop (Route A, non-str return) records a TextOnlyReply, adds a - model-visible Error correction, and recovers when a real tool call follows.""" - +async def test_default_preserves_text_adds_error_and_retries(): class TestAgent(Agent, llm=_TEST_LLM): @strategy(CodeActStrategy(config=CodeActConfig(max_retries=5, max_iterations=10))) async def my_task(self) -> dict: - """Return a dict — a bare string won't validate, forcing the correction path.""" + """Return a dict.""" ... fake_llm = FakeLLMClient( scripted_responses=[ - _resp("I think the answer is ready."), # text-only drift (won't validate as dict) - _resp(tool_calls=[_ret({"ok": True})]), # real tool call → recovery + _resp("I think the answer is ready."), + _resp(tool_calls=[_ret({"ok": True})]), ] ) agent = TestAgent(llm=fake_llm) - result = await agent.my_task() - assert result == {"ok": True} - - drifts = _drifts(agent) - assert len(drifts) == 1, f"expected one TextOnlyReply, got {len(drifts)}" - d = drifts[0] - assert d.route == "return_result" - assert d.finish_reason == "stop" - assert d.content == "I think the answer is ready." - assert d.recovered is True, "drift should be marked recovered after the real tool call" - # The correction is a model-visible Error event. - errs = [e for e in _errors(agent) if "no tool call" in e.content] - assert errs, "expected a model-visible Error correction after the drift" + assert await agent.my_task() == {"ok": True} + + outputs = _events(agent, LLMOutput) + assert [event.content for event in outputs] == ["I think the answer is ready."] + + diagnostics = _events(agent, TextOnlyReply) + assert len(diagnostics) == 1 + assert diagnostics[0].content == "I think the answer is ready." + assert diagnostics[0].finish_reason == "stop" + assert diagnostics[0].handler == "retry_text_only_response" + assert diagnostics[0].action == "retry" + + corrections = [ + event + for event in agent.event_manager.values() + if event.event_type == "Error" and "no tool call" in event.content + ] + assert len(corrections) == 1 + events = agent.event_manager.values() + assert events.index(outputs[0]) < events.index(corrections[0]) + + # The provider sees the exact assistant text followed by user feedback. + assert any( + message.get("role") == "assistant" + and message.get("content") == "I think the answer is ready." + for message in fake_llm.last_messages + ) + assert not any(message.get("tool_calls") for message in fake_llm.last_messages) @pytest.mark.asyncio -async def test_text_only_drift_not_shown_to_model(): - """TextOnlyReply is Role.METADATA — recorded but never rendered into the prompt.""" - from nooa.context_blocks.roles import Role - from nooa.events import TextOnlyReply +async def test_return_text_as_result_is_opt_in(): + class TestAgent(Agent, llm=_TEST_LLM): + @strategy(CodeActStrategy(on_text_only=return_text_as_result)) + async def my_task(self) -> str: + """Return a string.""" + ... - assert TextOnlyReply._role == Role.METADATA + agent = TestAgent(llm=FakeLLMClient(scripted_responses=[_resp("done")])) + + assert await agent.my_task() == "done" + assert [event.content for event in _events(agent, LLMOutput)] == ["done"] + assert _events(agent, ToolCallEvent) == [] + diagnostic = _events(agent, TextOnlyReply)[0] + assert diagnostic.handler == "return_text_as_result" + assert diagnostic.action == "return_result" @pytest.mark.asyncio -async def test_text_only_backstop_aborts_after_threshold(): - """Repeated text-only drift (no recovery) still aborts via the backstop.""" +async def test_error_finish_reason_never_calls_text_only_handler(): + calls = [] + + def accept_text(context): + calls.append(context) + return TextOnlyResponseAction.return_result(context.content) class TestAgent(Agent, llm=_TEST_LLM): - @strategy( - CodeActStrategy( - config=CodeActConfig( - max_retries=10, - max_iterations=10, - max_consecutive_text_only=3, - ) - ) - ) - async def my_task(self) -> dict: - """Return a dict.""" + @strategy(CodeActStrategy(on_text_only=accept_text)) + async def my_task(self) -> str: + """Return a string.""" ... - fake_llm = FakeLLMClient(scripted_responses=[_resp("still chatting") for _ in range(6)]) - agent = TestAgent(llm=fake_llm) - with pytest.raises(GenerationError, match="plain text without a tool call"): + agent = TestAgent( + llm=FakeLLMClient(scripted_responses=[_resp("partial", finish_reason="error")]) + ) + + with pytest.raises(GenerationError, match="incomplete response"): await agent.my_task() - # Three drifts recorded, none recovered. - drifts = _drifts(agent) - assert len(drifts) == 3, f"expected 3 drifts before abort, got {len(drifts)}" - assert all(d.recovered is False for d in drifts) - # A correction was offered on each drift. - assert len([e for e in _errors(agent) if "no tool call" in e.content]) == 3 + assert calls == [] + assert [event.content for event in _events(agent, LLMOutput)] == ["partial"] + assert _events(agent, TextOnlyReply) == [] @pytest.mark.asyncio -async def test_multiple_drifts_all_marked_recovered(): - """Several consecutive drifts before a real tool call: all flip recovered=True.""" +async def test_default_does_not_treat_valid_string_as_result(): + class TestAgent(Agent, llm=_TEST_LLM): + @strategy(CodeActStrategy()) + async def my_task(self) -> str: + """Return a string.""" + ... + fake_llm = FakeLLMClient(scripted_responses=[_resp("prose"), _resp(tool_calls=[_ret("done")])]) + agent = TestAgent(llm=fake_llm) + + assert await agent.my_task() == "done" + assert [event.content for event in _events(agent, LLMOutput)] == ["prose"] + assert any(event.event_type == "Error" for event in agent.event_manager.values()) + + +@pytest.mark.asyncio +async def test_default_preserves_empty_stop_without_replaying_empty_assistant_message(): class TestAgent(Agent, llm=_TEST_LLM): - @strategy(CodeActStrategy(config=CodeActConfig(max_retries=8, max_iterations=12))) + @strategy(CodeActStrategy()) async def my_task(self) -> dict: """Return a dict.""" ... - fake_llm = FakeLLMClient( - scripted_responses=[ - _resp("thinking 1"), # drift 1 (Route A, non-str → correction) - _resp("thinking 2"), # drift 2 - _resp(tool_calls=[_ret({"ok": True})]), # recovery - ] - ) + fake_llm = FakeLLMClient(scripted_responses=[_resp(""), _resp(tool_calls=[_ret({"ok": True})])]) agent = TestAgent(llm=fake_llm) - result = await agent.my_task() - assert result == {"ok": True} - drifts = _drifts(agent) - assert len(drifts) == 2 - assert all(d.recovered is True for d in drifts), ( - "all drifts before the recovering tool call must be marked recovered" + assert await agent.my_task() == {"ok": True} + assert [event.content for event in _events(agent, LLMOutput)] == [""] + assert [event.content for event in _events(agent, TextOnlyReply)] == [""] + assert not any( + message.get("role") == "assistant" and not message.get("content") + for message in fake_llm.last_messages ) @pytest.mark.asyncio -async def test_route_b_does_not_add_contradictory_correction(): - """Route B (synthetic_comment) injects its own synthetic tool result, so it must - NOT also add a 'no tool call' Error — that would contradict the synthetic call.""" - - class TestAgent(Agent, llm=_TEST_LLM): - @strategy( - CodeActStrategy( - config=CodeActConfig( - max_retries=8, - max_iterations=12, - text_only_stop_behavior="synthetic_comment", - ) +async def test_callback_can_synthesize_execute_python_without_replacing_output(): + def execute_text(context): + return TextOnlyResponseAction.tool_calls( + ToolCall( + id="synthetic_cell", + name="execute_python", + arguments=json.dumps({"code": f"print({context.content!r})"}), ) ) + + class TestAgent(Agent, llm=_TEST_LLM): + @strategy(CodeActStrategy(on_text_only=execute_text)) async def my_task(self) -> str: """Return a string.""" ... - fake_llm = FakeLLMClient( - scripted_responses=[ - _resp("some prose, no tool call"), # Route B drift - _resp(tool_calls=[_ret("done")]), # recovery - ] - ) + fake_llm = FakeLLMClient(scripted_responses=[_resp("hello"), _resp(tool_calls=[_ret("done")])]) agent = TestAgent(llm=fake_llm) - result = await agent.my_task() - assert result == "done" - - # Drift recorded and recovered. - drifts = _drifts(agent) - assert len(drifts) == 1 - assert drifts[0].route == "synthetic_comment" - assert drifts[0].recovered is True - # No contradictory "no tool call" Error correction in Route B. - assert not [e for e in _errors(agent) if "no tool call" in e.content], ( - "Route B must not add a corrective Error (it has its own synthetic result)" - ) + + assert await agent.my_task() == "done" + assert [event.content for event in _events(agent, LLMOutput)] == ["hello"] + assert [event.tool_call_id for event in _events(agent, ToolCallEvent)] == [ + "synthetic_cell", + "c_ret", + ] + python_output = _events(agent, PythonOutput)[0] + assert python_output.tool_call_id == "synthetic_cell" + assert python_output.stdout == "hello\n" + diagnostic = _events(agent, TextOnlyReply)[0] + assert diagnostic.handler.endswith("execute_text") + assert diagnostic.action == "tool_calls" @pytest.mark.asyncio -async def test_route_b_synthetic_comment_does_not_reuse_next_cell_number(): - """A synthetic comment and the next real cell have distinct execution counts.""" +async def test_async_callback_is_supported(): + async def return_text(context): + return TextOnlyResponseAction.return_result(context.content.upper()) + class TestAgent(Agent, llm=_TEST_LLM): + @strategy(CodeActStrategy(on_text_only=return_text)) + async def my_task(self) -> str: + """Return a string.""" + ... + + agent = TestAgent(llm=FakeLLMClient(scripted_responses=[_resp("done")])) + assert await agent.my_task() == "DONE" + + +@pytest.mark.asyncio +async def test_callback_can_return_non_string_result(monkeypatch): + def return_integer(context): + return TextOnlyResponseAction.return_result(42) + + class TestAgent(Agent, llm=_TEST_LLM): + @strategy(CodeActStrategy(on_text_only=return_integer)) + async def my_task(self) -> int: + """Return an integer.""" + ... + + metrics = HarnessMetrics() + monkeypatch.setattr("nooa.strategies.codeact.get_harness_metrics", lambda: metrics) + agent = TestAgent(llm=FakeLLMClient(scripted_responses=[_resp("forty-two")])) + + assert await agent.my_task() == 42 + assert metrics.stop_to_return_result_count == 1 + assert metrics.stop_to_return_result_previews == [] + + +@pytest.mark.asyncio +async def test_text_only_backstop_still_aborts(): class TestAgent(Agent, llm=_TEST_LLM): @strategy( CodeActStrategy( config=CodeActConfig( - max_retries=8, - max_iterations=12, - text_only_stop_behavior="synthetic_comment", + max_retries=10, + max_iterations=10, + max_consecutive_text_only=3, ) ) ) - async def my_task(self) -> str: - """Return a string.""" + async def my_task(self) -> dict: + """Return a dict.""" + ... + + agent = TestAgent( + llm=FakeLLMClient(scripted_responses=[_resp("still chatting") for _ in range(4)]) + ) + with pytest.raises(GenerationError, match="plain text without a tool call"): + await agent.my_task() + + diagnostics = _events(agent, TextOnlyReply) + assert len(diagnostics) == 3 + assert [event.consecutive_text_only for event in diagnostics] == [1, 2, 3] + + +def test_text_only_diagnostic_is_not_model_visible(): + from nooa.context_blocks.roles import Role + + assert TextOnlyReply._role == Role.METADATA + + +@pytest.mark.asyncio +async def test_text_only_output_survives_sqlite_resume(tmp_path): + class TestAgent(Agent, llm=_TEST_LLM): + @strategy(CodeActStrategy()) + async def my_task(self) -> dict: + """Return a dict.""" ... + db_path = tmp_path / "text-only-recovery.db" + storage = SQLiteStorageManager(db_path) fake_llm = FakeLLMClient( scripted_responses=[ - _resp("some prose, no tool call"), - _resp( - tool_calls=[ - ToolCall( - id="real_cell", - name="execute_python", - arguments=json.dumps({"code": "'computed'"}), - ) - ] - ), - _resp(tool_calls=[_ret("done")]), + _resp("I should have used a tool."), + _resp(tool_calls=[_ret({"ok": True})]), ] ) - agent = TestAgent(llm=fake_llm) + agent = TestAgent(llm=fake_llm, storage=storage) - assert await agent.my_task() == "done" - outputs = [event for event in agent.event_manager.values() if isinstance(event, PythonOutput)] - assert [event.execution_count for event in outputs] == [1, 2] - assert outputs[1].tool_call_id == "real_cell" - assert outputs[1].value == "computed" + assert await agent.my_task() == {"ok": True} + storage.close() + + reopened = SQLiteStorageManager(db_path) + try: + resumed = EventManager(backend=reopened.event_backend).values() + assert [event.content for event in resumed if isinstance(event, LLMOutput)] == [ + "I should have used a tool." + ] + diagnostics = [event for event in resumed if isinstance(event, TextOnlyReply)] + assert len(diagnostics) == 1 + assert diagnostics[0].action == "retry" + finally: + reopened.close() diff --git a/tests/strategies/test_toolcall_result_none_regression.py b/tests/strategies/test_toolcall_result_none_regression.py index 6154ee822..87bc46886 100644 --- a/tests/strategies/test_toolcall_result_none_regression.py +++ b/tests/strategies/test_toolcall_result_none_regression.py @@ -34,7 +34,7 @@ ) from nooa.errors import GenerationError from nooa.events import ResultStatus -from nooa.strategies.codeact import CodeActStrategy +from nooa.strategies.codeact import CodeActStrategy, return_text_as_result from nooa.unifiedllm import FakeLLMClient, LLMResponse, ToolCall # --------------------------------------------------------------------------- @@ -550,32 +550,36 @@ async def compute(self) -> int: # --------------------------------------------------------------------------- -# 8. stop_to_return_result path — synthetic return_result fails → exhausted +# 8. stop_to_return_result path — append-only validation at exhaustion # --------------------------------------------------------------------------- class TestStopToReturnResultPath: - """When a text-only stop response is converted to synthetic return_result - and validation fails with session exhausted, ToolCallEvent must be safe.""" + """Text-only validation does not persist a tool call the model did not make.""" @pytest.mark.asyncio - async def test_stop_to_synthetic_return_result_exhausted(self): - """stop + text content → synthetic return_result → validation fails → exhausted. + async def test_stop_to_return_result_exhausted_is_append_only(self): + """stop + text content → direct return validation → failure at exhaustion. For a method returning int, a text-only stop with "hello" should be - routed through return_result("hello") → validation fails → exhausted. - The synthetic ToolCallEvent must have result != None. + validated as return_result("hello") and fail without fabricating a + provider-visible ToolCallEvent. """ class TestAgent(Agent, llm=_TEST_LLM): - @strategy(CodeActStrategy(config=CodeActConfig(max_retries=1))) + @strategy( + CodeActStrategy( + config=CodeActConfig(max_retries=1), + on_text_only=return_text_as_result, + ) + ) async def get_number(self) -> int: """Return an integer.""" ... fake_llm = FakeLLMClient( scripted_responses=[ - # LLM returns text with stop (will be converted to synthetic return_result) + # LLM returns text with stop (validated as the result) _resp("hello world"), # finish_reason="stop" ] ) @@ -586,28 +590,29 @@ async def get_number(self) -> int: await agent_instance.get_number() events = agent_instance.event_manager.values() - # Find the synthetic return_result ToolCallEvent + llm_outputs = [e for e in events if e.event_type == "LLMOutput"] + assert [e.content for e in llm_outputs] == ["hello world"] tool_call_events = [e for e in events if e.event_type == "ToolCallEvent"] - - for tc_event in tool_call_events: - assert tc_event.result is not None, ( - f"Synthetic return_result ToolCallEvent {tc_event.tool_call_id!r} has " - f"result=None — would corrupt the next session." - ) + assert tool_call_events == [] @pytest.mark.asyncio async def test_stop_empty_content_none_return_type(self): """stop + no content for -> None method should succeed without corruption.""" class TestAgent(Agent, llm=_TEST_LLM): - @strategy(CodeActStrategy(config=CodeActConfig(max_retries=1))) + @strategy( + CodeActStrategy( + config=CodeActConfig(max_retries=1), + on_text_only=return_text_as_result, + ) + ) async def do_something(self) -> None: """Do something.""" ... fake_llm = FakeLLMClient( scripted_responses=[ - # Empty stop → synthetic return_result(None) → should succeed for -> None + # Empty stop → validate None directly → should succeed for -> None LLMResponse( raw_response=None, content="", @@ -623,9 +628,10 @@ async def do_something(self) -> None: assert result is None events = agent_instance.event_manager.values() + llm_outputs = [e for e in events if e.event_type == "LLMOutput"] + assert [e.content for e in llm_outputs] == [""] tool_call_events = [e for e in events if e.event_type == "ToolCallEvent"] - for tc_event in tool_call_events: - assert tc_event.result is not None + assert tool_call_events == [] # --------------------------------------------------------------------------- diff --git a/tests/unifiedllm/test_finish_reason_propagation.py b/tests/unifiedllm/test_finish_reason_propagation.py index 2ef459214..785cc1960 100644 --- a/tests/unifiedllm/test_finish_reason_propagation.py +++ b/tests/unifiedllm/test_finish_reason_propagation.py @@ -19,7 +19,7 @@ from nooa.config import CodeActConfig from nooa.errors import GenerationError from nooa.strategies.codeact import CodeActStrategy -from nooa.unifiedllm import CompletionClient, ResponsesClient +from nooa.unifiedllm import CompletionClient, ResponsesClient, create_tool_from_callable from nooa.unifiedllm.unifiedllm import ( _map_completion_finish_reason, _map_responses_finish_reason, @@ -51,6 +51,10 @@ def make_tool_call(id: str, name: str, arguments: str) -> ChatCompletionMessageT ) +def do_thing() -> None: + """A test tool.""" + + class TestMapCompletionFinishReason: """Unit tests for the Chat-Completions finish_reason mapper.""" @@ -142,14 +146,48 @@ def test_content_filter_maps_to_error(self, client): out = client.call([{"role": "user", "content": "Hi"}]) assert out.finish_reason == "error" - def test_tool_calls_preserved_even_if_provider_reports_length(self, client): - # When tool calls are present the client keeps "tool_calls" regardless of - # the provider's raw finish_reason. + def test_sync_length_takes_precedence_over_parsed_tool_calls(self, client): tc = make_tool_call("call_1", "do_thing", "{}") resp = make_mock_response(content=None, tool_calls=[tc], finish_reason="length") with patch("litellm.completion", return_value=resp): out = client.call([{"role": "user", "content": "Hi"}]) - assert out.finish_reason == "tool_calls" + assert out.finish_reason == "length" + assert len(out.tool_calls) == 1 + + @pytest.mark.asyncio + async def test_async_length_takes_precedence_over_parsed_tool_calls(self, client): + tc = make_tool_call("call_1", "do_thing", "{}") + resp = make_mock_response(content=None, tool_calls=[tc], finish_reason="length") + with patch("litellm.acompletion", new_callable=AsyncMock, return_value=resp): + out = await client.acall([{"role": "user", "content": "Hi"}]) + assert out.finish_reason == "length" + assert len(out.tool_calls) == 1 + + def test_sync_length_takes_precedence_over_xml_tool_fallback(self, client): + resp = make_mock_response( + content='{"name":"do_thing","arguments":{}}', + finish_reason="length", + ) + with patch("litellm.completion", return_value=resp): + out = client.call( + [{"role": "user", "content": "Hi"}], + tools=[create_tool_from_callable(do_thing)], + ) + assert out.finish_reason == "length" + assert len(out.tool_calls) == 1 + + @pytest.mark.asyncio + async def test_async_length_takes_precedence_over_xml_tool_fallback(self, client): + resp = make_mock_response( + content='{"name":"do_thing","arguments":{}}', + finish_reason="length", + ) + with patch("litellm.acompletion", new_callable=AsyncMock, return_value=resp): + out = await client.acall( + [{"role": "user", "content": "Hi"}], + tools=[create_tool_from_callable(do_thing)], + ) + assert out.finish_reason == "length" assert len(out.tool_calls) == 1 @@ -164,6 +202,22 @@ def _make_responses_api_response(status: str, reason: str | None = None): ) +def _make_incomplete_responses_tool_response(): + return SimpleNamespace( + output=[ + SimpleNamespace( + type="function_call", + call_id="call_1", + name="do_thing", + arguments="{}", + ) + ], + usage=None, + status="incomplete", + incomplete_details=SimpleNamespace(reason="max_output_tokens"), + ) + + class TestResponsesClientPropagation: """The real ResponsesClient return path surfaces the derived finish_reason.""" @@ -196,6 +250,21 @@ def test_sync_failed_maps_to_error(self, client): out = client.call([{"role": "user", "content": "Hi"}]) assert out.finish_reason == "error" + def test_sync_length_takes_precedence_over_parsed_tool_calls(self, client): + resp = _make_incomplete_responses_tool_response() + with patch("litellm.responses", return_value=resp): + out = client.call([{"role": "user", "content": "Hi"}]) + assert out.finish_reason == "length" + assert len(out.tool_calls) == 1 + + @pytest.mark.asyncio + async def test_async_length_takes_precedence_over_parsed_tool_calls(self, client): + resp = _make_incomplete_responses_tool_response() + with patch("litellm.aresponses", new_callable=AsyncMock, return_value=resp): + out = await client.acall([{"role": "user", "content": "Hi"}]) + assert out.finish_reason == "length" + assert len(out.tool_calls) == 1 + class TestCodeActAbortOnRealLengthPath: """End-to-end: a real CompletionClient truncation triggers CodeAct's abort.""" @@ -225,3 +294,30 @@ async def my_task(self) -> str: # Abort must be immediate: a single call, no retry loop. assert mock_acompletion.call_count == 1 + + @pytest.mark.asyncio + async def test_truncated_tool_call_is_never_executed(self): + length_response = make_mock_response( + content="", + tool_calls=[make_tool_call("call_1", "execute_python", '{"code":"x = 42"}')], + finish_reason="length", + ) + real_llm = CompletionClient(model="test-model") + + class TestAgent(Agent, llm=real_llm): + @strategy(CodeActStrategy(config=CodeActConfig(max_retries=3, max_iterations=10))) + async def my_task(self) -> str: + """A task.""" + ... + + agent_instance = TestAgent(llm=real_llm) + with patch( + "litellm.acompletion", new_callable=AsyncMock, return_value=length_response + ) as mock_acompletion: + with pytest.raises(GenerationError, match="max_tokens"): + await agent_instance.my_task() + + assert mock_acompletion.call_count == 1 + assert all( + event.event_type != "ToolCallEvent" for event in agent_instance.event_manager.values() + )