diff --git a/ms_agent/agent/agent.yaml b/ms_agent/agent/agent.yaml index 7144a213e..6ce749f45 100644 --- a/ms_agent/agent/agent.yaml +++ b/ms_agent/agent/agent.yaml @@ -4,6 +4,50 @@ llm: modelscope_api_key: modelscope_base_url: https://api-inference.modelscope.cn/v1 + # Whether THIS model may be shown image attachments. Left unset on purpose: + # it is read as a tri-state, so absent means "nobody has said", which falls + # back to the provider's declared capability and then to runtime learning + # (a model that rejects an image is remembered and never shown one again). + # A hard default either way is worse — false makes a capable model silently + # ignore attachments, true burns a 400 on every text-only model's first use. + # supports_vision: true + + # Image encoding, applied at the wire boundary (ms_agent/llm/multimodal.py). + vision: + enabled: true + # Long-edge cap. 2560 sits inside DashScope's recommended range and at + # Anthropic's high-resolution tier while cutting a 4K upload ~4x. NOT 1568 + # (Anthropic's standard tier): it downsamples rather than rejecting, so + # forcing that would throw away resolution the newer tier can use. + max_edge: 2560 + # Hard ceiling on the base64 STRING length — DashScope's 10 MB limit is + # expressed that way; 8 MB leaves headroom. + max_bytes: 8388608 + # OpenAI-family `detail`. 'low' is an explicit cost lever, not a default. + detail: auto + # GIF/BMP/TIFF/HEIC -> PNG first frame. DashScope's vision docs do not list + # GIF, so transcoding gives one answer that works on every provider. + transcode: true + # Keep only the most recent N images in context (0 = unlimited). + max_images: 0 + + # OPTIONAL escape hatch for a model that cannot see images at all: a + # separately configured VISION model that describes an image as text, which + # the main model then reasons over. Lossy by construction, so it is the + # fallback, never the preferred path — when the main model can see images the + # transports show it the real pixels and this is not used. + # + # Unset by default. The `image_reader` tool is registered ONLY when a + # `model` is present here, because a tool that can only ever fail is worse + # than no tool at all. Enabling it also needs `tools.image_reader` below. + # + # auxiliary: + # service: dashscope + # model: qwen3.8-max # must be a model that CAN see images + # api_key: # optional; falls back to env / provider spec + # base_url: # optional; same + # protocol: openai # optional; 'anthropic' for that wire format + generation_config: temperature: 0.3 top_k: 20 @@ -36,6 +80,12 @@ tools: - edit_file - grep - glob + # Ask a separately configured vision model to describe an image, for a main + # model that cannot see one. Needs `llm.vision.auxiliary.model` set above; + # without it the tool is not registered. `mcp: false` marks it a built-in — + # every `tools.` without that flag is treated as an MCP server. + # image_reader: + # mcp: false code_executor: mcp: false implementation: python_env diff --git a/ms_agent/agent/llm_agent.py b/ms_agent/agent/llm_agent.py index 7ab37e209..2733eaa1f 100644 --- a/ms_agent/agent/llm_agent.py +++ b/ms_agent/agent/llm_agent.py @@ -20,6 +20,8 @@ from ms_agent.callbacks import Callback, callbacks_mapping from ms_agent.knowledge_search import SirchmunkSearch from ms_agent.llm.llm import LLM +from ms_agent.llm.message_text import (append_text, flatten_message_text, + prepend_text) from ms_agent.llm.utils import Message, ToolResult from ms_agent.memory import Memory, get_memory_meta_safe, memory_mapping from ms_agent.memory.memory_manager import SharedMemoryManager @@ -44,7 +46,8 @@ ErrorRaised, PlanEntry, PlanUpdated, ReasoningDelta, ReasoningEnded, ReasoningStarted, ToolCallCompleted, - ToolCallStarted, TurnCompleted, UsageInfo) + ToolCallComposing, ToolCallStarted, + TurnCompleted, UsageInfo) from ms_agent.utils import (async_retry, is_retryable_error, read_history, save_history) from ms_agent.utils.constants import DEFAULT_TAG, DEFAULT_USER @@ -315,6 +318,13 @@ def __init__( # When None, the legacy sync console_io / input() path is used. self._input_source = kwargs.get('input_source', None) + # Attachments belonging to the FIRST user turn, parked between the + # interactive read in run_loop and create_messages (whose input is a + # bare string). Cleared as soon as create_messages consumes them, so a + # later turn can never inherit the first turn's images. Mid-conversation + # turns bypass this entirely — InputCallback builds their Message. + self._pending_attachments: List[Dict[str, Any]] = [] + # Personalization (lazy-loaded in _build_personalization_section) self._profile_manager = ProfileManager() @@ -665,7 +675,7 @@ async def on_task_begin(self, messages: List[Message]): self.log_output(f'Agent {self.tag} task beginning.') if self.resolve_enable_snapshots(self.config): _user_content = next( - ((getattr(m, 'content', '') or '')[:80] + (flatten_message_text(getattr(m, 'content', ''))[:80] for m in messages if getattr(m, 'role', '') == 'user'), '', ) @@ -759,6 +769,10 @@ def _on_result(index: int, tool_call, raw, duration_s: float) -> None: tool_detail=tool_call_result_format.tool_detail, hook_attachments=tool_call_result_format.hook_attachments, is_error=tool_call_result_format.is_error, + # Images the tool produced. Carried on the tool Message so the + # transports can put them in the IMAGE channel; the text channel + # keeps only the short status. + attachments=tool_call_result_format.attachments, ) if _new_message.tool_call_id is None: @@ -1122,6 +1136,38 @@ def _emit_content_end(self) -> None: else: sys.stdout.write('\n') + #: Bytes of tool-call arguments between two ``ToolCallComposing`` events. + #: Small enough that a multi-file write reports progress several times a + #: second, large enough that a short call emits once and stops. + _COMPOSING_STEP = 256 + + def _emit_tool_composing(self, message, announced: Dict[int, int]) -> None: + """Report tool calls the model is still writing. + + Streaming hands us the assistant message repeatedly, with each tool + call's ``arguments`` growing chunk by chunk. Nothing has run yet — this + is purely so the UI can say "preparing write_file…" instead of showing + nothing at all while a large call is transmitted. + + Silent for a UI-less run (no event sink), and throttled so short calls + emit once rather than once per chunk. + """ + if self._event_sink is None: + return + for index, call in enumerate(getattr(message, 'tool_calls', None) or []): + if not isinstance(call, dict): + continue + name = str(call.get('tool_name') or '') + if not name: + continue # the name always precedes the arguments; wait for it + size = len(str(call.get('arguments') or '')) + last = announced.get(index) + if last is not None and size - last < self._COMPOSING_STEP: + continue + announced[index] = size + self._event_sink.emit( + ToolCallComposing(index=index, name=name, arguments_len=size)) + @staticmethod def _extract_plan_from_tool_result(msg): """Parse a todo / split_task tool result into a list of PlanEntry, or @@ -1245,8 +1291,17 @@ async def create_messages( ), f'inputs can be either a list or a string, but current is {type(messages)}' messages = [ Message(role='system', content=''), - Message(role='user', content=messages or self.query), + Message( + role='user', + content=messages or self.query, + # Attachments for the FIRST turn. The interactive read that + # produced this prompt happens in run_loop, which stashes + # them here — the string-in signature cannot carry them, and + # a session's first message is exactly when a user attaches + # something. + attachments=self._pending_attachments or []), ] + self._pending_attachments = [] messages[0].content = self._build_system_content() @@ -1437,8 +1492,11 @@ async def _attach_memory_recall(self, messages: List[Message]) -> None: last = messages[-1] if getattr(last, 'role', None) != 'user': return - content = last.content - if not isinstance(content, str): + # Read the text out of whatever shape the content is in, rather than + # bailing on a block list: a multimodal turn that silently got no memory + # recall is a far worse outcome than one whose query came from its text. + content = flatten_message_text(last.content) + if not content: return # The turn may already carry other blocks (skill # update notice prefixed by the host, prompt-files update notice) — @@ -1462,7 +1520,9 @@ async def _attach_memory_recall(self, messages: List[Message]) -> None: if block: if block in content: return # marker-less backend, identical block attached - last.content = f'{last.content}\n\n{block}' + # append_text keeps the shape: a str grows, a block list gains a + # trailing text block (concatenating onto a list would raise). + last.content = append_text(last.content, block) return # ── prompt-files update notices (hot-reload perception) ────────────── @@ -1535,8 +1595,7 @@ def _attach_prompt_update_notice(self, messages: List[Message]): if not messages: return None last = messages[-1] - if getattr(last, 'role', None) != 'user' or not isinstance( - last.content, str): + if getattr(last, 'role', None) != 'user': return None baseline = self._prompt_surface @@ -1559,7 +1618,10 @@ def _attach_prompt_update_notice(self, messages: List[Message]): return None notice = workspace_files.render_update_notice(changed) - last.content = f'{notice}\n\n{last.content}' + # Shape-preserving prepend; on a block list the notice becomes the first + # text block, which also matches the providers' label-before-payload + # preference. + last.content = prepend_text(last.content, notice) return lambda: self._commit_prompt_surface(current) async def condense_memory(self, messages: List[Message]) -> List[Message]: @@ -1880,6 +1942,10 @@ async def step( _response_message = None _printed_reasoning_header = False _printed_reasoning_footer = False + # index -> arguments length already announced, so a long tool + # call reports progress instead of going silent (see + # ui.events.ToolCallComposing). + _composing: Dict[int, int] = {} _gen = self.llm.generate(messages, tools=tools) _loop = asyncio.get_running_loop() _NO_MORE = object() @@ -1928,6 +1994,7 @@ def _next_chunk(_g=_gen): _printed_reasoning_footer = True self._emit_content(new_content) _content = _response_message.content + self._emit_tool_composing(_response_message, _composing) messages[-1] = _response_message yield messages finally: @@ -2222,6 +2289,12 @@ def _msg_to_dict(msg: Message) -> Dict[str, Any]: d: Dict[str, Any] = {'role': msg.role, 'content': msg.content or ''} if msg.tool_calls: d['tool_calls'] = msg.tool_calls + # Image refs must survive to disk: the SessionLog is the source of truth + # a resumed session rebuilds context from, so dropping them here means + # attached images vanish on reload (and on every context reassembly). + # They are references, not bytes — cheap to persist. + if getattr(msg, 'attachments', None): + d['attachments'] = msg.attachments if hasattr(msg, 'tool_call_id') and msg.tool_call_id: d['tool_call_id'] = msg.tool_call_id if hasattr(msg, 'name') and msg.name: @@ -2369,6 +2442,10 @@ async def run_loop(self, messages: Union[List[Message], str], await self.cleanup_tools() return messages = turn.text + # create_messages() below builds the user Message from + # this string, so hand the turn's attachments over + # out-of-band rather than widening that signature. + self._pending_attachments = turn.attachments else: # Non-interactive with no task: accept piped stdin as the # query; otherwise fail clearly instead of blocking input(). @@ -2506,10 +2583,16 @@ async def run_loop(self, messages: Union[List[Message], str], # conversational truth. Advance the ingest ledger past it # (sync, in-memory + small file write) so the next turn's # delta does not sweep the partial content in either. + # THIS ROUND ONLY -- the same slice `_persist_partial_round` + # takes. Handing over the whole history would mark earlier + # rounds as ingested too, including one a background ingest + # is still writing (extraction takes seconds), which loses + # it: the write finds an empty delta, or fails and is denied + # its retry. for _mem_tool in self.memory_tools: if hasattr(_mem_tool, 'mark_ingested'): try: - _mem_tool.mark_ingested(messages) + _mem_tool.mark_ingested(messages[pre_step_len:]) except Exception: # noqa: E722 - never mask cancel pass raise diff --git a/ms_agent/callbacks/input_callback.py b/ms_agent/callbacks/input_callback.py index 9b4df84b7..7eb8e84b9 100644 --- a/ms_agent/callbacks/input_callback.py +++ b/ms_agent/callbacks/input_callback.py @@ -58,4 +58,8 @@ async def after_tool_call(self, runtime: Runtime, messages: List[Message]): runtime.should_stop = True return runtime.should_stop = False - messages.append(Message(role='user', content=turn.text)) + messages.append( + Message( + role='user', + content=turn.text, + attachments=turn.attachments)) diff --git a/ms_agent/command/interactive.py b/ms_agent/command/interactive.py index c0b940819..ef12bf3c6 100644 --- a/ms_agent/command/interactive.py +++ b/ms_agent/command/interactive.py @@ -11,8 +11,8 @@ """ from __future__ import annotations -from dataclasses import dataclass -from typing import Any, List, Optional +from dataclasses import dataclass, field +from typing import Any, Dict, List, Optional from ms_agent.command.router import CommandRouter from ms_agent.command.types import CommandContext, CommandResultType @@ -28,6 +28,10 @@ class InteractiveTurn: action: str text: Optional[str] = None + #: Non-text parts the input source attached to this turn (images). Stays + #: separate from ``text`` all the way to ``Message.attachments`` — a CLI + #: never sets it, a WebUI composer does. + attachments: List[Dict[str, Any]] = field(default_factory=list) class InteractiveSession: @@ -49,6 +53,26 @@ def __init__(self, # When None, plain print() is used (CLI). self._event_sink = event_sink + def _take_attachments(self) -> List[Dict[str, Any]]: + """Non-text parts the input source queued for the prompt just read. + + Optional protocol method: a plain CLI/TUI has no attachments and does + not implement it, so this returns ``[]`` and nothing downstream changes. + Called immediately after ``read_prompt`` returns, so the attachments and + the text belong to the same submission — hence "take": the source hands + them over once and clears them. + """ + source = self._input_source + if source is None: + return [] + take = getattr(source, 'take_attachments', None) + if take is None: + return [] + try: + return list(take() or []) + except Exception: # an input source must never break the turn + return [] + async def run_turn( self, messages: Optional[List[Any]] = None, @@ -84,7 +108,10 @@ async def run_turn( if self._event_sink is not None: from ms_agent.ui.events import UserMessage self._event_sink.emit(UserMessage(text=query)) - return InteractiveTurn(action='submit', text=query) + return InteractiveTurn( + action='submit', + text=query, + attachments=self._take_attachments()) cmd_name, args = self._router.parse_input(query) ctx = CommandContext( diff --git a/ms_agent/hooks/context.py b/ms_agent/hooks/context.py index 0186eaf5d..2b752190d 100644 --- a/ms_agent/hooks/context.py +++ b/ms_agent/hooks/context.py @@ -100,8 +100,16 @@ def condense_hook_attachments_for_llm( def extract_latest_user_prompt(messages: list[Message]) -> str: + """The latest user turn's text, for hooks that inspect what was asked. + + A block list is reduced to its text rather than ``str()``-ed: a hook that + matches on the prompt would otherwise be handed a Python repr and silently + stop matching (and UserPromptSubmit echoes this value back into the + conversation on a block, so the repr would become visible). + """ + from ms_agent.llm.message_text import flatten_message_text + for msg in reversed(messages): if msg.role == 'user': - return msg.content if isinstance(msg.content, str) else str( - msg.content) + return flatten_message_text(msg.content) return '' diff --git a/ms_agent/llm/anthropic_llm.py b/ms_agent/llm/anthropic_llm.py index 11f7ab8bc..728f99016 100644 --- a/ms_agent/llm/anthropic_llm.py +++ b/ms_agent/llm/anthropic_llm.py @@ -5,10 +5,13 @@ from typing import Any, Dict, Generator, Iterator, List, Optional, Union from ms_agent.llm import LLM +from ms_agent.llm.thinking import create_with_thinking_fallback from ms_agent.llm.utils import Message, Tool, ToolCall -from ms_agent.utils import assert_package_exist, retry +from ms_agent.utils import assert_package_exist, get_logger, retry from ms_agent.utils.constants import get_service_config +logger = get_logger() + class _SSEEventInjector(httpx.SyncByteStream): """Injects SSE ``event:`` lines into DashScope's streaming response. @@ -278,10 +281,20 @@ def _call_llm(self, kwargs['extra_body'] = extra_body params.update(kwargs) - if stream: - return self.client.messages.stream(**params) - else: - return self.client.messages.create(**params) + def _send(**call): + call.setdefault('model', self.model) + if stream: + return self.client.messages.stream(**call) + return self.client.messages.create(**call) + + # This legacy engine owned no repair at all: a model that cannot think + # rejected the `thinking` block with a hard 400 and the error went + # straight to the caller, while the transport port of this same engine + # recovered. Bring it in line — `model` is passed through `_send` so it + # cannot collide with the wrapper's own named argument. + rest = {k: v for k, v in params.items() if k != 'model'} + return create_with_thinking_fallback(_send, self.client, self.model, + logger, **rest) @retry(max_attempts=LLM.retry_count, delay=3.0) def generate(self, diff --git a/ms_agent/llm/message_text.py b/ms_agent/llm/message_text.py new file mode 100644 index 000000000..0149e22cb --- /dev/null +++ b/ms_agent/llm/message_text.py @@ -0,0 +1,125 @@ +# Copyright (c) ModelScope Contributors. All rights reserved. +"""One way to read the text out of a message, whatever shape its content is in. + +``Message.content`` is typed ``Union[str, List[Dict[str, str]]]``, so a caller +may legitimately hand the framework a provider-shaped block list. About twenty +places read a user message's content expecting a string — memory extraction, +full-text indexing, session auto-naming, summary compaction, snapshot labels, +hook prompt extraction — and none of them fails loudly on a list: they store a +Python repr, or a guard skips the message entirely. Both are silent, and the +symptom shows up weeks later as a garbled memory row or a nonsense session name. + +Image attachments in this codebase ride on ``Message.attachments`` precisely so +that ``content`` stays a string and those call sites keep working untouched. This +module is the belt to that suspenders: the highest-value of those sites route +through it, so a block list arriving from anywhere degrades to "the text of it" +rather than to garbage. + +Mirrors hermes-agent's ``agent/message_content.py``, which exists for the same +reason. +""" +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any, List + +#: Block kinds that carry no readable text. Listed rather than inferred so a new +#: modality is a deliberate edit here instead of silently stringifying its bytes. +_NON_TEXT_BLOCKS = frozenset({ + 'image', + 'image_url', + 'input_image', + 'audio', + 'input_audio', + 'video', + 'input_video', + 'file', + 'document', +}) + +#: Keys a text-bearing block may use, in preference order. ``content`` is last: +#: it is the most generic and the most likely to hold something structured. +_TEXT_KEYS = ('text', 'input_text', 'output_text', 'summary_text', 'content') + + +def _field(value: Any, key: str) -> Any: + if isinstance(value, Mapping): + return value.get(key) + return getattr(value, key, None) + + +def _block_text(block: Any) -> str: + if block is None: + return '' + if isinstance(block, str): + return block + kind = str(_field(block, 'type') or '').strip().lower() + if kind in _NON_TEXT_BLOCKS: + return '' + for key in _TEXT_KEYS: + value = _field(block, key) + if isinstance(value, str): + return value + return '' + + +def flatten_message_text(content: Any, *, sep: str = '\n') -> str: + """The readable text of ``content``, for any shape it can legitimately take. + + * ``str`` -> itself (the overwhelmingly common case, returned unchanged so + no caller's behaviour shifts); + * ``list`` of blocks -> the text blocks joined by ``sep``; image/audio/video + blocks contribute nothing rather than their base64; + * anything else -> its own text field if it has one, else ``str()``. + + Never raises, and never returns None — callers use the result in prompts, + hashes and filenames. + """ + if content is None: + return '' + if isinstance(content, str): + return content + if isinstance(content, (list, tuple)): + parts: List[str] = [_block_text(block) for block in content] + return sep.join(part for part in parts if part) + text = _block_text(content) + if text: + return text + try: + return str(content) + except Exception: + return '' + + +def append_text(content: Any, extra: str, *, sep: str = '\n\n') -> Any: + """``content`` with ``extra`` appended, preserving its shape. + + A string grows; a block list gains a trailing text block. Used where the + framework augments a user turn in place (memory recall, update notices) — + concatenating a string onto a list would raise, and replacing the list with + a string would drop whatever non-text blocks it carried. + """ + if not extra: + return content + if isinstance(content, str) or content is None: + base = content or '' + return f'{base}{sep}{extra}' if base else extra + if isinstance(content, (list, tuple)): + return [*content, {'type': 'text', 'text': extra}] + return f'{flatten_message_text(content)}{sep}{extra}' + + +def prepend_text(content: Any, extra: str, *, sep: str = '\n\n') -> Any: + """``content`` with ``extra`` in front, preserving its shape. + + A block list gets the text block FIRST, which also matches the providers' + preference for a short label ahead of the payload. + """ + if not extra: + return content + if isinstance(content, str) or content is None: + base = content or '' + return f'{extra}{sep}{base}' if base else extra + if isinstance(content, (list, tuple)): + return [{'type': 'text', 'text': extra}, *content] + return f'{extra}{sep}{flatten_message_text(content)}' diff --git a/ms_agent/llm/multimodal.py b/ms_agent/llm/multimodal.py new file mode 100644 index 000000000..391b722a8 --- /dev/null +++ b/ms_agent/llm/multimodal.py @@ -0,0 +1,638 @@ +# Copyright (c) ModelScope Contributors. All rights reserved. +"""Image attachments: internal references in, provider-native blocks out. + +A user turn carries images on ``Message.attachments`` as REFERENCES:: + + {'type': 'image', 'path': 'user_files/a.png', + 'media_type': 'image/png', 'label': 'Image 1: a.png'} + +Nothing upstream of the wire ever holds the bytes. This module is the single +place that resolves a reference to actual pixels, and it does so at the last +possible moment — inside a transport's ``_format_input_message``. That timing is +the whole point: + +* **The right encoding is provider- and model-specific.** Anthropic auto-downsamples + above 1568 px (2576 px on its high-resolution tier) and takes GIF; DashScope's + vision docs list only JPEG/PNG/WebP and cap the base64 string at 10 MB. Baking + bytes into the SessionLog would freeze one provider's answer forever. +* **A session can change models mid-conversation.** With references, switching to a + text-only model degrades the same log to text placeholders, and switching back + makes the images visible again. With inlined base64 neither direction works. +* **Re-encoding is cheap and cacheable**, while re-writing a log is not. + +Ordering follows Anthropic's guidance (image-then-text reads best) and each image +is introduced by its own ``Image N: `` text block, which is what lets a +follow-up question say "the second image" and land on the right one. +""" +from __future__ import annotations + +import base64 +import io +import mimetypes +import os +from dataclasses import dataclass, field +from functools import lru_cache +from typing import Any, Dict, List, Optional, Sequence, Tuple + +from ms_agent.utils import get_logger + +logger = get_logger() + +#: Media types every supported provider accepts. GIF is deliberately absent: +#: Anthropic and OpenAI take it, DashScope's vision documentation does not list +#: it, so it is transcoded (first frame) to PNG for a single cross-provider +#: answer — which also resolves "animations are unsupported, first frame is used". +SUPPORTED_MEDIA_TYPES = frozenset({'image/png', 'image/jpeg', 'image/webp'}) + +#: Transcoded to PNG rather than rejected. +TRANSCODE_MEDIA_TYPES = frozenset( + {'image/gif', 'image/bmp', 'image/tiff', 'image/heic', 'image/heif'}) + +#: Flat per-image token cost for the LOCAL context estimator. Not a billing +#: figure — the providers charge by patch count off the pixel dimensions +#: (Anthropic ``⌈w/28⌉ × ⌈h/28⌉``; DashScope reports the real number back as +#: ``prompt_tokens_details.image_tokens``). This exists so the estimator stops +#: measuring base64 character count, which over-counts by ~26x even for a 22 KB +#: image (measured: 282 real vs 7,293 estimated). +IMAGE_TOKEN_ESTIMATE = 800 + + +@dataclass(frozen=True) +class VisionOptions: + """Resolved ``llm.vision`` config plus the root relative paths resolve against.""" + + #: False => never send pixels; every image degrades to a text placeholder. + enabled: bool = True + #: Long-edge cap. 2560 sits inside all three vendors' safe range while + #: cutting a 4K upload ~4x. Deliberately NOT 1568: Anthropic downsamples + #: rather than rejecting, so forcing its standard-tier limit would throw + #: away resolution its high-resolution tier (2576 px) can use. + max_edge: int = 2560 + #: Hard ceiling on the base64 STRING length (DashScope's limit is expressed + #: that way). 8 MB leaves 2 MB of headroom under its 10 MB. + max_bytes: int = 8 * 1024 * 1024 + #: OpenAI-family ``detail``; 'low' is an explicit cost lever. + detail: str = 'auto' + #: Transcode GIF/BMP/TIFF/HEIC to PNG instead of skipping them. + transcode: bool = True + #: Keep only the most recent N images in context (0 = unlimited). + max_images: int = 0 + #: Directory workspace-relative ``path`` values resolve against. + workspace_root: str = '' + + @classmethod + def from_config(cls, + config: Any, + workspace_root: str = '') -> 'VisionOptions': + """Build from an agent ``config`` (``llm.vision`` block, all optional).""" + vision = None + llm = getattr(config, 'llm', None) + if llm is not None: + vision = getattr(llm, 'vision', None) + root = workspace_root or str(getattr(config, 'output_dir', '') or '') + + def pick(name: str, default): + if vision is None: + return default + value = getattr(vision, name, None) + return default if value is None else value + + return cls( + enabled=bool(pick('enabled', True)), + max_edge=int(pick('max_edge', cls.max_edge)), + max_bytes=int(pick('max_bytes', cls.max_bytes)), + detail=str(pick('detail', cls.detail)), + transcode=bool(pick('transcode', True)), + max_images=int(pick('max_images', 0)), + workspace_root=root, + ) + + +@dataclass(frozen=True) +class ImageRef: + """One normalized image attachment reference.""" + + path: str + media_type: str + label: str = '' + + @property + def filename(self) -> str: + return os.path.basename(self.path) or self.path + + +def _guess_media_type(path: str, declared: str = '') -> str: + if declared and declared.startswith('image/'): + return declared.lower() + guessed = mimetypes.guess_type(path)[0] or '' + return guessed.lower() if guessed.startswith('image/') else '' + + +def image_refs(attachments: Optional[Sequence[Dict[str, Any]]], + opts: Optional[VisionOptions] = None) -> List[ImageRef]: + """The image attachments of a message, in order, as normalized refs. + + Non-image entries and entries whose type this build cannot render are + dropped; ``max_images`` keeps the most RECENT ones (a later image is more + likely what the current question is about). + """ + if not attachments: + return [] + opts = opts or VisionOptions() + refs: List[ImageRef] = [] + for index, item in enumerate(attachments): + if not isinstance(item, dict) or item.get('type') != 'image': + continue + path = str(item.get('path') or '') + if not path: + continue + media_type = _guess_media_type(path, str(item.get('media_type') or '')) + if not media_type: + logger.warning( + '[vision] attachment %s has no recognizable image type; skipped', + path) + continue + label = str(item.get('label') or '') + refs.append(ImageRef(path=path, media_type=media_type, label=label)) + if opts.max_images and len(refs) > opts.max_images: + dropped = len(refs) - opts.max_images + logger.info( + '[vision] %d image(s) dropped from context (max_images=%d); ' + 'the most recent %d are kept', dropped, opts.max_images, + opts.max_images) + refs = refs[-opts.max_images:] + return refs + + +def _resolve_path(path: str, root: str) -> str: + if os.path.isabs(path): + return os.path.normpath(path) + if root: + return os.path.normpath(os.path.join(root, path)) + return os.path.normpath(path) + + +def _encode(data: bytes) -> str: + return base64.b64encode(data).decode('ascii') + + +def _pil(): + try: + from PIL import Image # noqa: F401 + return Image + except ImportError: # pragma: no cover - pillow is a base dependency + return None + + +#: Progressive JPEG quality ladder used only when a resize alone cannot get the +#: encoded size under budget. Mirrors opencode's approach. +_JPEG_QUALITIES = (85, 75, 60, 45) + + +def _shrink(raw: bytes, media_type: str, + opts: VisionOptions) -> Tuple[str, str]: + """Return ``(base64, media_type)`` within ``opts`` limits. + + Proactive rather than send-and-retry: Anthropic silently downsamples instead + of rejecting, so a reactive strategy would never fire there and we would + upload full-resolution images for nothing. + """ + Image = _pil() + encoded = _encode(raw) + needs_transcode = media_type not in SUPPORTED_MEDIA_TYPES + if Image is None: + if needs_transcode: + raise ValueError( + f'{media_type} needs transcoding but Pillow is unavailable') + return encoded, media_type + + with Image.open(io.BytesIO(raw)) as img: + # Animated source: the vendors only look at the first frame anyway. + try: + img.seek(0) + except (EOFError, ValueError): + pass + width, height = img.size + oversize = max(width, height) > opts.max_edge + if not needs_transcode and not oversize and len( + encoded) <= opts.max_bytes: + return encoded, media_type + + has_alpha = img.mode in ('RGBA', 'LA') or (img.mode == 'P' and + 'transparency' in img.info) + frame = img.convert('RGBA' if has_alpha else 'RGB') + if oversize: + scale = opts.max_edge / float(max(width, height)) + frame = frame.resize( + (max(1, round(width * scale)), max(1, round(height * scale))), + Image.LANCZOS) + + # PNG first when it is likely to both fit and matter: transparency must + # not be flattened, and the images users attach to a chat are mostly + # screenshots/diagrams/text, where JPEG ringing is exactly what makes + # small type unreadable — the one thing the model is being asked to + # read. For a large photo PNG would be huge and pointless, so only try + # it under ~2 MP; the JPEG ladder below is the fallback either way. + pixels = frame.size[0] * frame.size[1] + prefer_png = has_alpha or pixels <= 2_000_000 + candidates: List[Tuple[str, str]] = [] + if prefer_png: + buf = io.BytesIO() + frame.save(buf, format='PNG', optimize=True) + candidates.append((_encode(buf.getvalue()), 'image/png')) + for quality in _JPEG_QUALITIES: + buf = io.BytesIO() + frame.convert('RGB').save( + buf, format='JPEG', quality=quality, optimize=True) + candidates.append((_encode(buf.getvalue()), 'image/jpeg')) + if not prefer_png: + # Lossless last resort for a big image the ladder could not fit. + buf = io.BytesIO() + frame.save(buf, format='PNG', optimize=True) + candidates.append((_encode(buf.getvalue()), 'image/png')) + + for encoded_candidate, candidate_type in candidates: + if len(encoded_candidate) <= opts.max_bytes: + return encoded_candidate, candidate_type + + # Still too big at the lowest quality: halve the edge and recurse once + # per step until it fits or the image is degenerate. + edge = max(frame.size) + while edge > 64: + edge = int(edge * 0.6) + scale = edge / float(max(frame.size)) + small = frame.convert('RGB').resize( + (max(1, round(frame.size[0] * scale)), + max(1, round(frame.size[1] * scale))), Image.LANCZOS) + buf = io.BytesIO() + small.save(buf, format='JPEG', quality=60, optimize=True) + encoded_candidate = _encode(buf.getvalue()) + if len(encoded_candidate) <= opts.max_bytes: + return encoded_candidate, 'image/jpeg' + raise ValueError( + f'cannot bring image under {opts.max_bytes} base64 bytes') + + +@lru_cache(maxsize=64) +def _load_cached(abs_path: str, mtime: float, size: int, media_type: str, + max_edge: int, max_bytes: int, + transcode: bool) -> Tuple[str, str]: + """``(base64, media_type)``, memoized on the file identity + encode params. + + The whole history is re-sent every round, so without this the same image is + re-read and re-encoded on every single request of a conversation. + """ + with open(abs_path, 'rb') as handle: + raw = handle.read() + if media_type in TRANSCODE_MEDIA_TYPES and not transcode: + raise ValueError(f'{media_type} is not accepted and transcode is off') + opts = VisionOptions( + max_edge=max_edge, max_bytes=max_bytes, transcode=transcode) + return _shrink(raw, media_type, opts) + + +def load_image(ref: ImageRef, + opts: VisionOptions) -> Optional[Tuple[str, str]]: + """``(base64, media_type)`` for one ref, or None when it cannot be sent. + + Never raises: a missing file or an un-encodable image must degrade to a text + placeholder, not kill the turn. + """ + abs_path = _resolve_path(ref.path, opts.workspace_root) + try: + stat = os.stat(abs_path) + except OSError as exc: + logger.warning('[vision] cannot read %s: %s', abs_path, exc) + return None + try: + return _load_cached(abs_path, stat.st_mtime, stat.st_size, + ref.media_type, opts.max_edge, opts.max_bytes, + opts.transcode) + except Exception as exc: # encode/transcode failure + logger.warning('[vision] cannot encode %s: %s', abs_path, exc) + return None + + +def placeholder_for(ref: ImageRef, reason: str = '') -> str: + """The text a model sees in place of an image it cannot be shown. + + Written for the model to be able to explain itself: a user who asks "what's + in this picture" must get an answer that says why it cannot see it and what + to do, not a silent non-answer. + """ + head = ref.label or f'Image: {ref.filename}' + body = (f'[{head} — not shown as an image. {reason} ' + f'The file is in the workspace at "{ref.path}".]') + return body + + +#: Reason strings, kept here so the wording is identical across transports. +#: Both spell out that any earlier image descriptions in the conversation came +#: from a model that could see the pictures. Without that sentence, a model +#: switched in mid-session sees "not shown" placeholders NEXT TO confident +#: assistant answers about the same images, resolves the contradiction as "so I +#: did see them after all", and claims present-tense sight (measured on +#: qwen3.7-max: it answered 能看到 and repeated its predecessor's reading). +#: The shared middle sentence: what to do about earlier descriptions. +_HISTORY_NOTE = ( + 'Earlier replies in this conversation that describe it were written while ' + 'a vision-capable model was active: treat them as reliable history, but do ' + 'not claim to see the image yourself.') + +#: The switch is off (the default). The remedy is to turn it on. +REASON_DISABLED = ( + 'Image understanding is not enabled for the current model, so you cannot ' + f'see this image now. {_HISTORY_NOTE} Tell the user they can turn on ' + '"image understanding" for this model in Settings → Models, or switch to a ' + 'model that supports it.') + +#: The switch is ON but the endpoint rejected the image. Telling this user to +#: "enable image understanding" would point at a box they already ticked, so +#: this wording names the real situation and offers the remedy that is left. +REASON_REJECTED = ( + 'This model rejected image input, so you cannot see this image even though ' + f'image understanding is enabled for it. {_HISTORY_NOTE} Tell the user this ' + 'model cannot accept images and that they should switch to one that can.') + +REASON_UNREADABLE = ('The file could not be read or decoded as an image.') + + +def _label_block(ref: ImageRef, index: int) -> Dict[str, str]: + """The ``Image N: `` introducer. + + Anthropic's own guidance: label each image with a short text block so it can + be referred to by name in this turn and in later ones. The ordinal carries + "the second image"; the filename carries "the chart one". + """ + return { + 'type': 'text', + 'text': ref.label or f'Image {index}: {ref.filename}', + } + + +def _degrade(text: str, refs: Sequence[ImageRef], reason: str) -> str: + """Fold every image into the text turn as placeholders.""" + notes = [placeholder_for(ref, reason) for ref in refs] + joined = '\n'.join(notes) + return f'{joined}\n\n{text}' if text else joined + + +def openai_content(text: Any, + attachments: Optional[Sequence[Dict[str, Any]]], + opts: VisionOptions, + vision_supported: bool = True, + disabled_reason: str = REASON_DISABLED) -> Any: + """Content for an OpenAI-compatible (Chat Completions) user message. + + Returns a plain string when there is nothing to attach — keeping the + overwhelmingly common text-only request byte-identical to before, which also + means prefix caching is unaffected. + + ``disabled_reason`` lets the caller say WHY the pixels are absent: the + default blames the switch, and a transport that knows the endpoint rejected + this model's images passes :data:`REASON_REJECTED` instead, so the model + never tells a user to enable something they already enabled. + """ + refs = image_refs(attachments, opts) + if not refs: + return text + if not (opts.enabled and vision_supported): + return _degrade( + text if isinstance(text, str) else '', refs, disabled_reason) + + blocks: List[Dict[str, Any]] = [] + unreadable: List[ImageRef] = [] + for index, ref in enumerate(refs, start=1): + loaded = load_image(ref, opts) + if loaded is None: + unreadable.append(ref) + continue + encoded, media_type = loaded + blocks.append(_label_block(ref, index)) + image_url: Dict[str, Any] = { + 'url': f'data:{media_type};base64,{encoded}' + } + if opts.detail and opts.detail != 'auto': + image_url['detail'] = opts.detail + blocks.append({'type': 'image_url', 'image_url': image_url}) + + if not blocks: # every image failed to load + return _degrade( + text if isinstance(text, str) else '', unreadable, + REASON_UNREADABLE) + + tail = text if isinstance(text, str) else '' + if unreadable: + tail = _degrade(tail, unreadable, REASON_UNREADABLE) + if tail: + blocks.append({'type': 'text', 'text': tail}) + return blocks + + +def anthropic_content(text: Any, + attachments: Optional[Sequence[Dict[str, Any]]], + opts: VisionOptions, + vision_supported: bool = True, + disabled_reason: str = REASON_DISABLED) -> Any: + """Content blocks for an Anthropic Messages user message. + + Same contract as :func:`openai_content`; only the block shape differs + (``{'type':'image','source':{'type':'base64',...}}``). + """ + refs = image_refs(attachments, opts) + if not refs: + return text + if not (opts.enabled and vision_supported): + return _degrade( + text if isinstance(text, str) else '', refs, disabled_reason) + + blocks: List[Dict[str, Any]] = [] + unreadable: List[ImageRef] = [] + for index, ref in enumerate(refs, start=1): + loaded = load_image(ref, opts) + if loaded is None: + unreadable.append(ref) + continue + encoded, media_type = loaded + blocks.append(_label_block(ref, index)) + blocks.append({ + 'type': 'image', + 'source': { + 'type': 'base64', + 'media_type': media_type, + 'data': encoded, + }, + }) + + if not blocks: + return _degrade( + text if isinstance(text, str) else '', unreadable, + REASON_UNREADABLE) + + tail = text if isinstance(text, str) else '' + if unreadable: + tail = _degrade(tail, unreadable, REASON_UNREADABLE) + if tail: + blocks.append({'type': 'text', 'text': tail}) + return blocks + + +def has_image_blocks(content: Any) -> bool: + """True when already-built content carries a provider-native image block. + + Used by the refusal fallback to know whether THIS request actually shipped + pixels — the only reliable signal, since a provider's rejection text may not + mention images at all (measured on DashScope: "Unexpected item type in + content", no mention of image/multimodal/vision). + """ + if not isinstance(content, list): + return False + for item in content: + if not isinstance(item, dict): + continue + if item.get('type') in ('image_url', 'image', 'input_image'): + return True + return False + + +#: What the model is told in place of an image the endpoint just refused. +#: Deliberately as informative as the proactive placeholder: the user asked +#: about a picture, so a bare "not available" makes the model reply "please +#: upload the image" — which is both wrong (it WAS uploaded) and unactionable. +#: Measured before this text existed, qwen3.7-max answered exactly that. +REASON_REFUSED = ( + 'not visible: this model rejected image input. The file was uploaded and is ' + 'in the workspace under the name shown above. Any earlier replies that ' + 'describe this image came from a model that could see it. Tell the user ' + 'this model cannot view images, and that they can enable "image ' + 'understanding" for it in Settings → Models or switch to a model that ' + 'supports vision.') + + +def strip_image_blocks(content: Any) -> Any: + """``content`` with image blocks replaced by an explanatory text marker. + + The retry after a refusal must still say WHAT was dropped and WHY, or the + model answers a question about an image it was never told about. The + preceding ``Image N: `` label block survives, so the marker only + has to supply the reason and the remedy. + """ + if not isinstance(content, list): + return content + texts: List[str] = [] + for item in content: + if not isinstance(item, dict): + continue + if item.get('type') == 'text': + value = str(item.get('text') or '') + if value: + texts.append(value) + elif item.get('type') in ('image_url', 'image', 'input_image'): + texts.append(f'[{REASON_REFUSED}]') + return '\n'.join(texts) + + +def estimate_content_tokens(content: Any, text_estimator) -> int: + """Token estimate for possibly-multimodal content. + + ``text_estimator`` scores a string. Image blocks get a flat + :data:`IMAGE_TOKEN_ESTIMATE` each instead of having their base64 measured as + text — the bug this exists to prevent inflates a single 2 MiB PNG to + ~699k tokens against a ~108k budget, which re-fires compaction every round. + """ + if content is None: + return 0 + if isinstance(content, str): + return text_estimator(content) + if not isinstance(content, list): + return text_estimator(str(content)) + total = 0 + for item in content: + if not isinstance(item, dict): + total += text_estimator(str(item)) + continue + kind = item.get('type') + if kind in ('image_url', 'image', 'input_image'): + total += IMAGE_TOKEN_ESTIMATE + elif kind == 'text': + total += text_estimator(str(item.get('text') or '')) + else: + # Unknown block: measure its text-ish payload, never its raw bytes. + total += text_estimator(str(item.get('text') or '')) + return total + + +#: Introduces images hoisted out of a tool result into their own user turn. +#: +#: Why hoist at all: the Chat Completions SCHEMA restricts a ``role: "tool"`` +#: message to text. OpenAI's own generated types say +#: ``Union[str, Iterable[ChatCompletionContentPartTextParam]]`` for a tool +#: message, versus the wider union (text | image_url | input_audio | file) for a +#: user message. The Responses API is different — its ``function_call_output`` +#: does allow image content — which is why AI-SDK-based clients report "OpenAI +#: supports media in tool results"; they are on that API, we are on this one. +#: +#: Measured (2026-08) against five OpenAI-compatible providers — DashScope, +#: ModelScope, OpenRouter, Kimi, MiniMax — inline image parts in a tool message +#: were accepted and read correctly by all five, i.e. they are more permissive +#: than the schema. Hoisting is kept anyway because it is valid under BOTH the +#: schema and every provider tested, whereas inline is valid only under the +#: latter; real OpenAI (the one endpoint whose schema forbids it) was not +#: testable here. Same reasoning as any other spec-vs-practice split: prefer the +#: form that cannot be wrong. +#: +#: The Anthropic transport does NOT hoist — that protocol allows image blocks +#: inside ``tool_result``, so there the image stays attached to the call that +#: produced it. Mirrors opencode's SYNTHETIC_ATTACHMENT_PROMPT. +TOOL_MEDIA_PROMPT = 'Images returned by the tool call above:' + + +def openai_tool_media_message( + attachments: Sequence[Dict[str, Any]], + opts: VisionOptions, + vision_supported: bool = True) -> Optional[Dict[str, Any]]: + """A synthetic user message carrying a tool result's images, or None. + + Returns None when there is nothing to show — no images, images disabled, or + none of them could be loaded — so the caller appends nothing and the tool's + own text stands on its own. + """ + refs = image_refs(attachments, opts) + if not refs or not (opts.enabled and vision_supported): + return None + content = openai_content( + TOOL_MEDIA_PROMPT, attachments, opts, vision_supported=True) + if not isinstance(content, list): + return None # every image failed to load; the tool text already says so + return {'role': 'user', 'content': content} + + +def anthropic_tool_result_blocks( + attachments: Sequence[Dict[str, Any]], + opts: VisionOptions, + vision_supported: bool = True) -> List[Dict[str, Any]]: + """Image blocks to nest INSIDE an Anthropic ``tool_result``. + + Anthropic allows image blocks in tool_result content, so the image can stay + attached to the call that produced it — strictly better than hoisting, since + the association survives without relying on message order. + """ + refs = image_refs(attachments, opts) + if not refs or not (opts.enabled and vision_supported): + return [] + blocks: List[Dict[str, Any]] = [] + for index, ref in enumerate(refs, start=1): + loaded = load_image(ref, opts) + if loaded is None: + continue + encoded, media_type = loaded + blocks.append(_label_block(ref, index)) + blocks.append({ + 'type': 'image', + 'source': { + 'type': 'base64', + 'media_type': media_type, + 'data': encoded, + }, + }) + return blocks diff --git a/ms_agent/llm/openai_llm.py b/ms_agent/llm/openai_llm.py index 28af611bf..bd9112963 100644 --- a/ms_agent/llm/openai_llm.py +++ b/ms_agent/llm/openai_llm.py @@ -10,15 +10,17 @@ ChatCompletionMessageToolCall, Function) from typing import Any, Dict, Generator, Iterable, List, Optional -from ms_agent.llm import LLM +from ms_agent.llm import LLM, multimodal +from ms_agent.llm.thinking import apply_effort, create_with_thinking_fallback from ms_agent.llm.utils import Message, Tool, ToolCall +from ms_agent.llm.vision import create_with_vision_fallback +from ms_agent.llm.vision import disabled_reason as vision_disabled_reason from ms_agent.utils import (MAX_CONTINUE_RUNS, assert_package_exist, get_logger, retry) from ms_agent.utils.constants import get_service_config logger = get_logger() - class _DashScopeResponsesTransport(httpx.HTTPTransport): """Rewrite /v1/responses -> /v1/chat/completions for DashScope proxy. @@ -97,6 +99,20 @@ def __init__( float(_read_timeout), connect=float(_connect_timeout)), ) self.base_url = base_url or '' + + # Image attachments (legacy non-router path). Resolution mirrors the + # router's: an explicit per-model switch wins, else the service's + # declared capability, else runtime learning from a refusal. + from ms_agent.llm.spec import get_registry + from ms_agent.llm.vision import resolve_supports_vision + self._vision = multimodal.VisionOptions.from_config(config) + _service = getattr(config.llm, 'service', None) + self._vision_supported = resolve_supports_vision( + config, + spec=get_registry().get(_service) + or get_registry().resolve_by_model(self.model), + model=self.model, + base_url=self.base_url) self.args: Dict = OmegaConf.to_container( getattr(config, 'generation_config', DictConfig({}))) @@ -252,6 +268,12 @@ def generate(self, if not stream: args.pop('stream_options', None) + # Lower the canonical knob first: the Responses path below reads + # `reasoning_effort` straight into `reasoning.effort`, so it must see a + # real OpenAI tier rather than a canonical `auto`/`off`. + args = apply_effort( + args, base_url=str(getattr(self.client, 'base_url', ''))) + if self._use_responses_api: if stream: return self._responses_stream_generate(messages, tools, **args) @@ -295,8 +317,24 @@ def _call_llm(self, if is_streaming and stream_options_config.get('include_usage', True): kwargs.setdefault('stream_options', {})['include_usage'] = True - return self.client.chat.completions.create( - model=self.model, messages=messages, tools=tools, **kwargs) + # Thinking is per-model and a refusal is a hard 400 (see llm/thinking.py). + # Image content is a per-model hard 400 on text-only models; retry once + # with the images folded into text and remember the model. Composes with + # the thinking fallback (each retries for its own reason). + sent_images = any( + multimodal.has_image_blocks(m.get('content')) + for m in messages if isinstance(m, dict)) + return create_with_vision_fallback( + lambda messages, **kw: create_with_thinking_fallback( + lambda **kw2: self.client.chat.completions.create( + model=self.model, messages=messages, tools=tools, **kw2), + self.client, self.model, logger, **kw), + base_url=getattr(self.client, 'base_url', ''), + model=self.model, + messages=messages, + sent_images=sent_images, + logger_=logger, + **kwargs) @staticmethod def _extract_cache_info(usage_obj: Any) -> tuple: @@ -870,9 +908,16 @@ def _responses_generate(self, if resp_tools: kwargs['tools'] = resp_tools - response = self._responses_client.responses.create( - model=self.model, - input=input_items, + # Same per-model hard-400 as the Chat Completions branch: a model that + # cannot think rejects the reasoning parameters outright. This branch + # used to lower the knob (`apply_effort`) without owning the repair, so + # the refusal reached the caller raw. + response = create_with_thinking_fallback( + lambda **kw: self._responses_client.responses.create( + model=self.model, input=input_items, **kw), + self._responses_client, + self.model, + logger, **kwargs, ) text = getattr(response, 'output_text', '') or '' @@ -922,9 +967,12 @@ def _responses_stream_generate(self, if resp_tools: kwargs['tools'] = resp_tools - stream = self._responses_client.responses.create( - model=self.model, - input=input_items, + stream = create_with_thinking_fallback( + lambda **kw: self._responses_client.responses.create( + model=self.model, input=input_items, **kw), + self._responses_client, + self.model, + logger, stream=True, **kwargs, ) @@ -1030,6 +1078,9 @@ def _format_input_message(self, openai_messages = [] for idx, message in enumerate(messages): + # Read image refs BEFORE to_dict_clean(), which strips them. + attachments = (message.attachments if isinstance(message, Message) + else message.get('attachments')) or [] if isinstance(message, Message): # Only strip string content, keep list content as-is for multimodal if isinstance(message.content, str): @@ -1037,12 +1088,30 @@ def _format_input_message(self, message = message.to_dict_clean() else: message = dict(message) + message.pop('attachments', None) content = message.get('content', '') # Only strip string content, multimodal content (list) should be kept as-is if isinstance(content, str): content = content.strip() + # Image refs -> native image_url blocks. A turn with no attachments + # returns the same plain string, so text-only requests are unchanged. + # Not for a tool message: the Chat Completions SCHEMA allows + # only text parts in `role: "tool"`, so its images go to the + # synthetic user turn appended after it (below). Five compatible + # providers were measured to accept inline image parts here anyway, + # but hoisting is valid under the schema AND under all of them — see + # multimodal.TOOL_MEDIA_PROMPT for the full measurement. + if attachments and message.get('role') != 'tool': + content = multimodal.openai_content( + content, + attachments, + self._vision, + vision_supported=self._vision_supported, + disabled_reason=vision_disabled_reason( + self.base_url, self.model)) + # Apply prefix cache structured content transformation # Only for string content, multimodal content is already structured if cache_indice is not None and idx == cache_indice: @@ -1075,4 +1144,13 @@ def _format_input_message(self, openai_messages.append(formatted_message) + # Tool-result images: same hoist as OpenAICompatTransport (the + # Chat Completions schema restricts a tool message to text parts). + if attachments and message.get('role') == 'tool': + media = multimodal.openai_tool_media_message( + attachments, self._vision, + vision_supported=self._vision_supported) + if media is not None: + openai_messages.append(media) + return openai_messages diff --git a/ms_agent/llm/router.py b/ms_agent/llm/router.py index 54a38954f..c20a2fac1 100644 --- a/ms_agent/llm/router.py +++ b/ms_agent/llm/router.py @@ -31,8 +31,13 @@ logger = get_logger() -def _build_transport(spec: ProviderSpec, model: str, api_key: Optional[str], - base_url: str, gen_config: dict) -> Transport: +def _build_transport(spec: ProviderSpec, + model: str, + api_key: Optional[str], + base_url: str, + gen_config: dict, + vision: Optional['VisionOptions'] = None, + vision_supported: bool = True) -> Transport: if spec.transport == TRANSPORT_ANTHROPIC_MESSAGES: from .transport.anthropic_messages import AnthropicMessagesTransport return AnthropicMessagesTransport( @@ -40,6 +45,8 @@ def _build_transport(spec: ProviderSpec, model: str, api_key: Optional[str], api_key=api_key, base_url=base_url, generation_config=gen_config, + vision=vision, + vision_supported=vision_supported, ) if spec.transport == TRANSPORT_OPENAI_COMPAT: from .transport.openai_compat import OpenAICompatTransport @@ -51,6 +58,8 @@ def _build_transport(spec: ProviderSpec, model: str, api_key: Optional[str], continue_gen_mode=spec.continue_gen_mode, continue_gen_stop=spec.continue_gen_stop, strip_reasoning_tags=spec.strip_reasoning_tags, + vision=vision, + vision_supported=vision_supported, ) raise ValueError(f'Unknown transport: {spec.transport}') @@ -154,6 +163,16 @@ def create(self, config: DictConfig) -> LLMProvider: getattr(config, 'generation_config', DictConfig({}))) gen_config = {**spec.default_generation_config, **(gen_config or {})} - transport = _build_transport(spec, model, api_key, base_url, - gen_config) + # Image attachments: encode options + whether this model may be shown + # pixels. Resolved here (the one place that has spec, model and base_url + # together) rather than inside the transports. + from .multimodal import VisionOptions + from .vision import resolve_supports_vision + vision = VisionOptions.from_config(config) + vision_supported = resolve_supports_vision( + config, spec=spec, model=model, base_url=base_url) + + transport = _build_transport(spec, model, api_key, base_url, gen_config, + vision=vision, + vision_supported=vision_supported) return LLMProvider(config=config, spec=spec, transport=transport) diff --git a/ms_agent/llm/stream_retry.py b/ms_agent/llm/stream_retry.py new file mode 100644 index 000000000..538124d6b --- /dev/null +++ b/ms_agent/llm/stream_retry.py @@ -0,0 +1,64 @@ +# Copyright (c) ModelScope Contributors. All rights reserved. +"""Retry a request whose rejection arrives *after* the HTTP response starts. + +``llm/thinking.py`` and ``llm/vision.py`` both repair a request the endpoint +refuses (drop the thinking parameters / drop the image blocks) and try once +more. Both used to guard only the ``create(...)`` call, on the assumption +spelled out in their docstrings: "the client performs the request — and raises — +before it returns an iterator". + +That holds for the OpenAI Python SDK, which does issue the HTTP request eagerly. +It does **not** hold for gateways that answer 200 and then put the error in the +stream. Measured on an Aliyun-family endpoint:: + + APIError: <400> InternalError.Algo.InvalidParameter: The thinking_budget + parameter must be a positive integer and not greater than 0 + +arrives while the first chunk is being read, i.e. *outside* the ``try`` — so +neither fallback saw it, nothing was retried, nothing was remembered, and the +raw provider error reached the user. + +The window this module reopens is deliberately narrow: only the FIRST advance of +the stream is guarded. Until then nothing has been handed to the caller, so +replacing the stream wholesale is invisible and safe. Once a single chunk has +been delivered the turn is already partly rendered, and silently restarting it +would duplicate or contradict what the user has seen — so a later failure is +re-raised untouched. +""" +from __future__ import annotations + +from typing import Any, Callable, Iterator + + +def retry_on_first_chunk(result: Any, repair: Callable[[BaseException], + Any]) -> Any: + """Guard the first advance of ``result`` with ``repair``. + + ``result`` is whatever the provider client returned. Non-iterators (a + non-streaming response object, Anthropic's stream *manager*) are handed back + untouched — there is no first chunk to guard, and their errors already + surface eagerly. + + ``repair(exc)`` is the same callable the eager path uses: it either returns + a replacement result or re-raises. Its replacement is streamed in full, so + the caller cannot tell which attempt produced the data. + """ + if not hasattr(result, '__next__'): + return result + + def _guarded() -> Iterator[Any]: + source = result + try: + first = next(source) + except StopIteration: + return + except Exception as exc: # noqa: BLE001 — handed to the same repair + replacement = repair(exc) + if replacement is not None: + yield from replacement + return + # Past this point the caller has seen output; a failure now is real. + yield first + yield from source + + return _guarded() diff --git a/ms_agent/llm/thinking.py b/ms_agent/llm/thinking.py new file mode 100644 index 000000000..753249e78 --- /dev/null +++ b/ms_agent/llm/thinking.py @@ -0,0 +1,692 @@ +# Copyright (c) ModelScope Contributors. All rights reserved. +"""One semantic knob for "how hard should the model think", plus the fallback +for models that refuse to be asked at all. + +Every vendor spells thinking differently and none of them spells it the same way +for long. Per-model real tiers, from each vendor's own docs and cross-checked +against opencode's model catalog (both agree line for line), 2026-08-18: + +================= ================================== ==================== ======== +model / endpoint modern field DISTINCT tiers default +================= ================================== ==================== ======== +xAI grok-4.5 ``reasoning_effort`` low/medium/high high +xAI grok-4.6 ``reasoning_effort`` + xhigh high +DeepSeek v4-* ``reasoning_effort`` / high/max ON, high + ``thinking: {type}`` (+low; med/xhigh→high) +Zhipu glm-5.2 ``reasoning_effort`` high/max ``max`` +Zhipu glm-5.3 ``reasoning_effort`` low/high/max ``max`` +Zhipu glm-5/5.1 ``thinking: {type}`` only — (no effort field) ON +Moonshot kimi-k3 ``reasoning_effort`` low/high/max ``max`` +Moonshot kimi-k2.x ``thinking: {type}`` only — (no effort field) varies +MiniMax M3 ``thinking: {type}`` adaptive/disabled ON via + (+ ``reasoning_split`` for the (NOT a depth knob) OpenAI, + output format, not the depth) OFF via + Anthropic +DashScope ``enable_thinking`` AND, on low/medium/xhigh per model + qwen3.8-max ``reasoning_effort``; the two (high/max→xhigh, + CANNOT travel with minimal→low) + ``thinking_budget`` +DashScope other ``enable_thinking`` only — (no effort field) per model +ModelScope ``enable_thinking`` (gateway) / — (no effort field) per model + ``chat_template_kwargs`` +OpenRouter ``reasoning: {effort|max_tokens}`` per model, published inferred + in ``/api/v1/models`` +Anthropic ``output_config.effort`` + low…max high + ``thinking: {type: adaptive}`` +================= ================================== ==================== ======== + +Four things follow, and they are the whole design: + +1. ``reasoning_effort`` is the de-facto standard. It is the name callers use + here, so anyone who knows one vendor already knows this one — and it survives + the OpenAI SDK's signature filter, unlike an invented name. + +2. Defaults are per-MODEL and they move. On DashScope alone, qwen3.5 and later + default thinking ON while qwen-plus/turbo/flash and qwen3-max default it OFF; + ``kimi-k2.6`` defaults OFF on Alibaba's deployment and ON on Moonshot's, same + name; MiniMax M3 defaults it ON through the OpenAI-compatible API and OFF + through the Anthropic-compatible one, same model. Any table of defaults we + wrote would be wrong within a release. So we do not write one: ``auto`` sends + NOTHING and inherits whatever the vendor tuned, and the lowering table below + is consulted ONLY when a caller asked for a specific tier. A bug in it can + then only affect someone who explicitly configured thinking, who will see it + immediately — rather than silently changing every request. + +3. **"Accepted" is not "distinct", and a wire enum is not a capability list.** + Two traps, both of which this module fell into once. OpenRouter's rejection + message lists its whole GATEWAY vocabulary, identically for every model, then + maps unsupported-but-valid tiers to the nearest one the model has — sending + ``max`` to grok-4.5 returns 200, not an error. And DeepSeek's ``/v1`` enum + went from five values to seven between 2026-07-27 and 2026-08-17 (``minimal`` + flipped from a hard 400 to accepted, and the declaration order changed), so a + vocabulary derived from probing has a measured shelf life of about three + weeks. The table below therefore records only what an endpoint REJECTS, and + leaves each vendor's documented aliasing to the vendor. + +4. **The tier is a request, not a promise.** Measured in billed reasoning tokens + (3 samples per cell, 2026-08-18), the effect ranges from crisp to absent: + glm-5.2 moves monotonically and treats ``minimal`` as off (0 tokens, 3/3); + glm-5.1 does not move at all (it has no effort field); grok-4.5 through + OpenRouter shows no trend across six tiers; qwen3.8-max is non-monotonic. + So this module translates the knob faithfully and does not pretend to know + what the model will do with it. +""" +from __future__ import annotations + +from typing import Any, Dict, Optional, Tuple +from urllib.parse import urlparse + +#: Wire keys that carry a thinking request, in any vendor's spelling. Used both +#: to strip a request back down and to recognise a refusal. +THINKING_PARAM_KEYS = ('enable_thinking', 'thinking_budget', 'thinking', + 'reasoning_effort', 'reasoning') + +#: The canonical knob callers set, in ``generation_config``. +EFFORT_KEY = 'reasoning_effort' + +#: Canonical ladder, weakest to strongest. ``auto`` is not a rung — it means +#: "no opinion", which is the default and is never sent anywhere. +#: +#: These are not our invention: every endpoint that VALIDATES the field reports +#: the same seven values (probed 2026-08-17 by sending a bogus one and reading +#: the error) — GLM-5.2 "none、minimal、low、medium、high、xhigh、max", +#: OpenRouter "max|xhigh|high|medium|low|minimal|none", DeepSeek the same list, +#: DashScope the same minus ``max``. Matching their vocabulary exactly means a +#: value the user types usually reaches the model untouched, instead of being +#: clamped onto a smaller set we made up. +EFFORT_TIERS = ('off', 'minimal', 'low', 'medium', 'high', 'xhigh', 'max') + +#: Ranks for clamping a requested tier onto what an endpoint accepts. Gaps leave +#: room for rungs a vendor may add later without renumbering. +EFFORT_RANKS = { + 'off': 0, + 'minimal': 10, + 'low': 20, + 'medium': 30, + 'high': 40, + 'xhigh': 60, + 'max': 70, +} + +_EFFORT_ALIASES = { + 'none': 'off', + 'disabled': 'off', + 'disable': 'off', + 'false': 'off', + 'no': 'off', + 'min': 'minimal', + 'med': 'medium', + 'extrahigh': 'xhigh', + 'maximum': 'max', + 'true': 'high', + 'on': 'high', + 'enabled': 'high', +} + +#: ``(base_url, model)`` pairs observed to refuse the thinking parameters. +MODELS_REFUSING_THINKING: set = set() + +#: ``(base_url, model)`` pairs that refuse to STOP thinking. The opposite +#: complaint, and it needs the opposite repair — OpenRouter answers +#: "Reasoning is mandatory for this endpoint and cannot be disabled" for +#: x-ai/grok-4.5, and healing that by forcing thinking off (the other set's +#: repair) both misses the point and poisons every later request for the model. +MODELS_REQUIRING_THINKING: set = set() + + +# --------------------------------------------------------------------------- # +# Endpoint families +# --------------------------------------------------------------------------- # +# Keyed on the endpoint HOST, not the model name. Hosts are stable — a vendor +# ships new model names every few weeks but keeps the same API surface, and two +# earlier attempts at a model-name table were both wrong within days. + +#: host substring -> family name. +_HOST_FAMILIES = ( + ('dashscope.aliyuncs.com', 'dashscope'), + ('maas.aliyuncs.com', 'dashscope'), # ATokenPlan et al. speak DashScope + ('api-inference.modelscope.cn', 'modelscope'), + ('api.deepseek.com', 'deepseek'), + ('open.bigmodel.cn', 'zhipu'), + ('bigmodel.cn', 'zhipu'), + ('z.ai', 'zhipu'), + ('api.moonshot.cn', 'moonshot'), # CN + ('api.moonshot.ai', 'moonshot'), # international, per Moonshot's docs + ('api.kimi.com', 'moonshot'), + ('api.minimax', 'minimax'), + ('openrouter.ai', 'openrouter'), + ('api.openai.com', 'openai'), +) + + +def endpoint_family(base_url: str, protocol: str = '') -> str: + """Which dialect of "thinking" this endpoint speaks. + + ``anthropic`` wins over the host: a vendor's Anthropic-compatible gateway + (DeepSeek serves one at ``api.deepseek.com/anthropic``) takes Messages-API + shapes, not its own OpenAI ones. + """ + if (protocol or '').lower() == 'anthropic': + return 'anthropic' + host = (urlparse(base_url or '').hostname or str(base_url or '')).lower() + for needle, family in _HOST_FAMILIES: + if needle in host: + return family + return 'unknown' + + +#: Tiers each family actually accepts, weakest to strongest. A request outside +#: the set is clamped (see ``clamp_effort``). +# What each family ACCEPTS without erroring — deliberately NOT "which tiers are +# distinct behaviours there". Those are two different questions and only the +# first one is ours: every vendor documents its own collapse (DashScope maps +# `high`/`max` onto `xhigh` and `minimal` onto `low`; Zhipu maps `low`/`medium` +# onto `high` and `xhigh` onto `max`; DeepSeek publishes a five-row table) and +# applies it per MODEL, which a host-keyed table cannot express and should not +# try to. So we only subtract values an endpoint is measured to REJECT, and let +# the vendor alias the rest. +_FAMILY_TIERS = { + # `max` is the single measured rejection on this host: qwen3.7-plus answers + # 400 for it while accepting none/minimal/low/medium/high/xhigh, and + # qwen3.8-max accepts all seven (each value probed individually, + # 2026-08-18). Excluding it protects the qwen3.7 family and costs the one + # model that does take it nothing, because DashScope documents `max` as an + # alias of `xhigh` — exactly where the downward clamp lands. The canonical + # set for qwen3.8-max, the only Qwen with a real effort field, is + # low / medium / xhigh. + 'dashscope': ('off', 'minimal', 'low', 'medium', 'high', 'xhigh'), + # No effort field at all: the switch is a boolean, so every "how hard" + # collapses onto "on". Corroborated for MiniMax M3 and the ModelScope-hosted + # Qwen3.5 family by opencode's model catalog, which lists them as `toggle`. + 'modelscope': ('off', 'high'), + 'minimax': ('off', 'high'), + 'anthropic': ('off', 'high'), + # Everyone else accepts the whole vocabulary. Endpoints that do not validate + # it (glm-5.1, glm-5, every Kimi, MiniMax, ModelScope) ignore an unknown + # value rather than failing, so passing a tier through costs nothing. + 'deepseek': EFFORT_TIERS, + 'zhipu': EFFORT_TIERS, + 'moonshot': EFFORT_TIERS, + 'openrouter': EFFORT_TIERS, + 'openai': EFFORT_TIERS, + 'unknown': EFFORT_TIERS, +} + +#: Extra raw keys a family understands, surfaced to users as an example of what +#: they may add by hand. We never send these ourselves — their defaults are +#: vendor-tuned and would be one more thing to keep in sync. +FAMILY_EXTRA_HINTS = { + 'dashscope': 'thinking_budget (1-32768)', + 'modelscope': 'chat_template_kwargs', + 'openrouter': 'reasoning.max_tokens', + 'anthropic': 'thinking_budget', +} + +#: Raw keys that cannot travel with our lowered ``reasoning_effort``. DashScope +#: rejects the pair outright ("'reasoning_effort' and 'thinking_budget' cannot +#: be set simultaneously") — and ``thinking_budget`` is precisely what we invite +#: people to add by hand above, so the two suggestions would collide. +_EFFORT_CONFLICTS = ('thinking_budget', ) + + +def offered_tiers(family: str) -> Tuple[str, ...]: + """The tiers worth OFFERING for this endpoint, weakest to strongest. + + Not the same list as ``_FAMILY_TIERS``, which answers "what will this + endpoint accept" — a question about avoiding 400s. This one answers "what is + worth showing a person", and on a switch-only endpoint that is two entries, + not eight. Listing a ladder where none exists is the UI promising control + the model does not have. + """ + supported = _FAMILY_TIERS.get(family, _FAMILY_TIERS['unknown']) + thinking = [t for t in supported if t != 'off'] + if len(thinking) <= 1: # a switch, however many rungs we may send it + # `on` is an alias of the single thinking tier, and it is the honest + # word for a knob with two positions. + return ('auto', 'off', 'on') + return ('auto', ) + tuple( + sorted(supported, key=lambda t: EFFORT_RANKS[t])) + + +def normalize_effort(raw: Any) -> Optional[str]: + """Free-form input -> a canonical tier, ``'auto'``, or ``None`` if garbage. + + Booleans are accepted because that is what the old ``enable_thinking`` + spelling used, and some config files carry it through as a YAML bool. + """ + if raw is None: + return 'auto' + if isinstance(raw, bool): + return 'high' if raw else 'off' + if not isinstance(raw, str): + return None + key = raw.strip().lower().replace('-', '').replace('_', '').replace(' ', '') + if key in ('', 'auto', 'default', 'inherit'): + return 'auto' + key = _EFFORT_ALIASES.get(key, key) + return key if key in EFFORT_TIERS else None + + +def clamp_effort(tier: str, supported: Tuple[str, ...]) -> str: + """Nearest tier the endpoint accepts, preferring the next WEAKER one. + + Direction matters, and the intuitive choice is the wrong one. An effort tier + is a quality FLOOR the caller is willing to pay for, not a ceiling — so + landing above the request spends money they did not ask for. A published + post-mortem of the opposite choice (zlxlabs/llm-compat#11, merged + 2026-08-05) describes exactly that: a request for the middle rung clamped + UP into a support set's interior gap, jumped three tiers, was billed at the + top tier, and said nothing louder than a log line. + + ``off`` is not treated as the bottom of the ladder: it is a different + request, so a thinking tier never collapses into it. On an endpoint that + only has a switch, the weakest thinking tier is "on"; on one that cannot + stop thinking at all (Kimi K3 per Moonshot's FAQ), ``off`` lands on the + weakest tier — which is also the remedy Zhipu prescribes for GLM-5.3 + ("change disabled to enabled and set reasoning_effort to low"). + """ + if tier in supported: + return tier + thinking = [t for t in supported if t != 'off'] + if not thinking: # switch-only, and we were not asked to switch off + return supported[0] + ranked = sorted(thinking, key=lambda t: EFFORT_RANKS[t]) + want = EFFORT_RANKS[tier] + weaker = [t for t in ranked if EFFORT_RANKS[t] <= want] + return weaker[-1] if weaker else ranked[0] + + +def _merge_extra_body(params: Dict[str, Any], extra: Dict[str, Any]) -> None: + body = dict(params.get('extra_body') or {}) + body.update(extra) + params['extra_body'] = body + + +def lower_effort(tier: str, family: str) -> Dict[str, Any]: + """The wire parameters that express ``tier`` on ``family``. + + ``tier`` must already be clamped to what the family supports. + """ + params: Dict[str, Any] = {} + if family == 'dashscope': + # BOTH knobs, because they cover disjoint sets of models on this host. + # `enable_thinking` is the only lever for the many models that take no + # effort field (every Qwen except qwen3.8-max) and it is what turns + # thinking on for the ones that default it off (qwen-plus: 0 characters + # of reasoning from `reasoning_effort: high` alone, 543 with the flag — + # because qwen-plus does not support the effort field at all). Alibaba's + # own CLI and opencode both send the flag here for the same reason. + # Models that understand only one of the two ignore the other. + # + # Known imprecision, deliberate: DashScope also hosts GLM, DeepSeek and + # Kimi, whose switch dialect on this host is `thinking.enabled` / + # `thinking: {type}` rather than `enable_thinking`. Getting that right + # needs per-model branching on a host-keyed table; the flag is ignored + # rather than rejected there, so the cost is a no-op field, not an error. + _merge_extra_body(params, {'enable_thinking': tier != 'off'}) + if tier != 'off': + params[EFFORT_KEY] = tier + elif family in ('modelscope', 'anthropic'): + # Boolean switch. On the Anthropic transport this is read back out and + # turned into the Messages-API `thinking` block. + _merge_extra_body(params, {'enable_thinking': tier != 'off'}) + elif family == 'minimax': + # `adaptive` rather than `enabled`: M3 decides per request whether the + # reasoning is worth it, which is what "on" should mean for an agent. + _merge_extra_body( + params, + {'thinking': { + 'type': 'disabled' if tier == 'off' else 'adaptive' + }}) + elif family in ('deepseek', 'zhipu'): + if tier == 'off': + # NOT `reasoning_effort: none`, even though the newer models accept + # it: glm-5.1 and glm-5 do not validate the field and simply IGNORE + # it (probed 2026-08-17 — 896 and 986 characters of reasoning with + # `none` set). The `thinking` object is the only spelling every + # generation honours. If a model rejects it outright (GLM-5.3 no + # longer allows thinking to be disabled), the mandatory-thinking + # repair below strips the request rather than failing the turn. + _merge_extra_body(params, {'thinking': {'type': 'disabled'}}) + else: + params[EFFORT_KEY] = tier + elif family == 'moonshot': + # Kimi is the other way round: it honours `none` on both k3 and k2.6 + # (0 characters of reasoning), so the effort field alone covers the + # whole range and no second shape is needed. + params[EFFORT_KEY] = 'none' if tier == 'off' else tier + elif family == 'openrouter': + # OpenRouter's own unified object; `enabled: false` is how it says off. + _merge_extra_body( + params, {'reasoning': { + 'enabled': False + } if tier == 'off' else { + 'effort': tier + }}) + else: # openai, unknown + params[EFFORT_KEY] = 'none' if tier == 'off' else tier + return params + + +def output_format_params(family: str) -> Dict[str, Any]: + """Params about WHERE the reasoning is delivered, not how much of it to do. + + Separate from the effort ladder on purpose: this asks nothing about how hard + to think, so it applies even under ``auto``, where we deliberately say + nothing about depth. + + MiniMax is the only family that needs it. Its OpenAI-compatible endpoint + inlines the reasoning into the answer as ``…`` (its docs call + that the native format) and offers ``reasoning_split`` to deliver it in + ``reasoning_content`` instead — which its docs "strongly recommend", and + which is the only shape it reads back: probed 2026-08-18, replaying a + separate ``reasoning_content`` in NATIVE mode behaves exactly like + discarding the thinking, because the field is not part of that format. + Verified on M3, M2.7 and M2.5: no error, reasoning moves out of ``content``. + + Host-gated by construction — third-party hosts of the same weights reject + the parameter outright (NVIDIA NIM: "Unsupported parameter(s): + 'reasoning_split'"), and they are a different family here. + """ + if family == 'minimax': + return {'extra_body': {'reasoning_split': True}} + return {} + + +def auto_params(family: str) -> Dict[str, Any]: + """What ``auto`` sends. Almost always nothing — see the module docstring. + + Two endpoints get an explicit ``true`` anyway: + + * ``anthropic`` — our Messages transport has no way to say "no opinion": it + always writes a ``thinking`` block, and absent means ``disabled``. Claude + would then never think. + * ``dashscope`` — its older commercial hybrids (qwen-plus, qwen-turbo, + qwen-flash, qwen3-max) default thinking OFF, and those are exactly the + cheap models people leave selected. Newer qwen3.5+ default it on, where + the flag is a redundant no-op (probed: 258 vs 276 characters of reasoning + with and without it). + """ + if family in ('anthropic', 'dashscope'): + return {'extra_body': {'enable_thinking': True}} + return {} + + +def _drop_conflicts(params: Dict[str, Any], + existing: Optional[Dict[str, Any]]) -> Dict[str, Any]: + """Yield to whatever the caller wrote by hand. + + Two degrees of yielding, because the raw keys mean different things: + + * A **switch** key (``enable_thinking``, ``thinking``, ``reasoning``) means + the caller is driving thinking themselves, so we contribute NOTHING — + adding a tier next to their ``enable_thinking: false`` would ask for a + depth and a shutdown in the same request. + * ``thinking_budget`` is only a depth, so the switch may still go out; but + our effort must not, because DashScope rejects that exact pair + ("'reasoning_effort' and 'thinking_budget' cannot be set simultaneously") + — and ``thinking_budget`` is precisely what the settings hint invites + people to add by hand, so the two suggestions would collide. + """ + if not params or not isinstance(existing, dict): + return params + extra = existing.get('extra_body') + present = set(existing) | (set(extra) if isinstance(extra, dict) else set()) + switches = set(THINKING_PARAM_KEYS) - {EFFORT_KEY} - set(_EFFORT_CONFLICTS) + if present & switches: + return {} + if present.isdisjoint(_EFFORT_CONFLICTS): + return params + return {k: v for k, v in params.items() if k != EFFORT_KEY} + + +def plan(effort: Any, + *, + base_url: str = '', + protocol: str = '', + existing: Optional[Dict[str, Any]] = None) -> dict: + """Resolve a canonical effort into a wire plan, without sending anything. + + Returns ``{'family', 'requested', 'effective', 'params', 'extra_hint'}``. + ``effective`` is the clamped tier, or ``'auto'``. ``existing`` is the + request (or stored params) the plan will be merged into, so conflicting raw + keys are honoured here rather than discovered on the wire. Shared by the + transports and by the WebUI, so what the settings page shows is what + actually ships. + """ + family = endpoint_family(base_url, protocol) + requested = normalize_effort(effort) + if requested is None: + requested = 'auto' + if requested == 'auto': + effective, wire = 'auto', auto_params(family) + else: + effective = clamp_effort( + requested, _FAMILY_TIERS.get(family, _FAMILY_TIERS['unknown'])) + wire = lower_effort(effective, family) + # Where the reasoning is delivered is a separate question from how much of + # it to do, so it survives `auto` and rides along with every tier. + for key, value in output_format_params(family).items(): + if key == 'extra_body': + _merge_extra_body(wire, value) + else: + wire.setdefault(key, value) + return { + 'family': family, + 'requested': requested, + 'effective': effective, + 'params': _drop_conflicts(wire, existing), + 'extra_hint': FAMILY_EXTRA_HINTS.get(family, ''), + } + + +def apply_effort(kwargs: Dict[str, Any], *, base_url: str, + protocol: str = '') -> Dict[str, Any]: + """Replace the canonical knob in ``kwargs`` with this endpoint's wire shape. + + Runs even when no knob was set, because "unset" IS ``auto`` and on two + endpoints auto has something to say (see :func:`auto_params`) — an absent + key must not mean a different thing from an explicit ``auto``. + + The canonical key is always removed, so a value like ``auto`` or ``off`` + never reaches a vendor that would reject it. Anything the caller set by hand + wins: raw wire keys already present are left exactly as they are, because + the raw form is the escape hatch and a user who reached for it means it. + """ + effort = kwargs.get(EFFORT_KEY, 'auto') + resolved = plan(effort, + base_url=base_url, + protocol=protocol, + existing={k: v + for k, v in kwargs.items() if k != EFFORT_KEY}) + if EFFORT_KEY not in kwargs and not resolved['params']: + return kwargs # nothing to say and nothing to strip + out = dict(kwargs) + out.pop(EFFORT_KEY, None) + existing_extra = out.get('extra_body') or {} + for key, value in resolved['params'].items(): + if key == 'extra_body': + for sub_key, sub_value in value.items(): + if sub_key not in existing_extra: + _merge_extra_body(out, {sub_key: sub_value}) + elif key not in out: + out[key] = value + return out + + +# --------------------------------------------------------------------------- # +# Refusal fallback +# --------------------------------------------------------------------------- # + + +def model_key(client: Any, model: str) -> tuple: + return (str(getattr(client, 'base_url', '')), model) + + +def _is_bad_request(exc: Exception) -> bool: + status = getattr(exc, 'status_code', None) + if status is not None and status != 400: + return False + return status == 400 or '400' in str(exc) + + +#: Phrases an endpoint uses to say thinking is not optional here. +_MANDATORY_MARKERS = ('mandatory', 'cannot be disabled', 'can not be disabled', + 'must be enabled', 'cannot be turned off') + + +def is_thinking_refusal(exc: Exception) -> bool: + """A 400 that names the thinking parameters — not any other bad request.""" + if not _is_bad_request(exc): + return False + text = str(exc).lower() + return any(k in text for k in THINKING_PARAM_KEYS) + + +def is_thinking_mandatory(exc: Exception) -> bool: + """A 400 complaining that thinking may not be switched OFF. + + Checked before :func:`is_thinking_refusal`, which it would otherwise match + (the message names ``reasoning``) and be repaired backwards. + """ + if not _is_bad_request(exc): + return False + text = str(exc).lower() + if not any(k in text for k in THINKING_PARAM_KEYS): + return False + return any(marker in text for marker in _MANDATORY_MARKERS) + + +def asks_to_disable(kwargs: Dict[str, Any]) -> bool: + """Whether this request is telling the model NOT to think. + + Every family spells "off" differently (see :func:`lower_effort`), and a + model that merely refuses to be switched off must still be allowed to + receive a positive tier — so the memo has to know which kind of request it + is looking at rather than blanking them all. + """ + extra = kwargs.get('extra_body') + extra = extra if isinstance(extra, dict) else {} + if extra.get('enable_thinking') is False or kwargs.get( + 'enable_thinking') is False: + return True + for source in (extra, kwargs): + thinking = source.get('thinking') + if isinstance(thinking, dict) and thinking.get('type') == 'disabled': + return True + reasoning = source.get('reasoning') + if isinstance(reasoning, dict) and reasoning.get('enabled') is False: + return True + return kwargs.get(EFFORT_KEY) in ('none', 'off') + + +def strip_thinking(kwargs: Dict[str, Any]) -> Dict[str, Any]: + """``kwargs`` with every thinking parameter REMOVED, saying nothing at all. + + The repair for an endpoint that insists on thinking: stop asking it to + stop. Returns the SAME object when there was nothing to strip. + """ + extra = kwargs.get('extra_body') + in_extra = isinstance(extra, dict) and any(k in extra + for k in THINKING_PARAM_KEYS) + if not in_extra and not any(k in kwargs for k in THINKING_PARAM_KEYS): + return kwargs + cleaned = {k: v for k, v in kwargs.items() if k not in THINKING_PARAM_KEYS} + if in_extra: + pruned = { + k: v + for k, v in extra.items() if k not in THINKING_PARAM_KEYS + } + if pruned: + cleaned['extra_body'] = pruned + else: + cleaned.pop('extra_body', None) + return cleaned + + +def without_thinking(kwargs: Dict[str, Any]) -> Dict[str, Any]: + """``kwargs`` with thinking turned OFF explicitly, not merely removed. + + Dropping the flag is not enough: some models default it ON and then refuse + the call ("parameter.enable_thinking must be set to false for non-stream + call" — qwen3-8b). So every other thinking spelling goes away and + ``enable_thinking`` is pinned to False, inside ``extra_body`` when that is + where it came from. + + Returns the SAME object when the request carried no thinking parameter at + all, so callers can tell "we never asked for thinking" (a 400 that is + somebody else's problem) from "we just turned it off" (worth a retry). + """ + extra = kwargs.get('extra_body') + in_extra = isinstance(extra, dict) and any(k in extra + for k in THINKING_PARAM_KEYS) + in_top = any(k in kwargs for k in THINKING_PARAM_KEYS) + if not in_extra and not in_top: + return kwargs + cleaned = dict(kwargs) + for key in THINKING_PARAM_KEYS: + cleaned.pop(key, None) + if in_extra: + new_extra = { + k: v + for k, v in extra.items() if k not in THINKING_PARAM_KEYS + } + new_extra['enable_thinking'] = False + cleaned['extra_body'] = new_extra + else: + cleaned['enable_thinking'] = False + return cleaned + + +def create_with_thinking_fallback(create, client, model: str, logger, + **kwargs) -> Any: + """Call ``create(**kwargs)``, retrying once with thinking off on a refusal. + + ``create`` must be the completions factory itself; it is called with the + (possibly cleaned) kwargs. + + Streaming is covered in BOTH shapes: clients that issue the request eagerly + raise out of ``create`` itself, and gateways that answer 200 before + rejecting the parameters raise on the first chunk — see + ``llm/stream_retry.py`` for why only that first chunk is guarded. + """ + from ms_agent.llm.stream_retry import retry_on_first_chunk + + key = model_key(client, model) + if key in MODELS_REFUSING_THINKING: + kwargs = without_thinking(kwargs) + elif key in MODELS_REQUIRING_THINKING and asks_to_disable(kwargs): + # Only the "off" request is doomed here; a positive tier still goes out + # normally, so this model is not blacklisted the way a refuser is. + kwargs = strip_thinking(kwargs) + + def _repair(e: BaseException) -> Any: + """Repair-and-retry, shared by the eager and first-chunk paths.""" + # Order matters: "reasoning is mandatory" also names a thinking + # parameter, so refusal would claim it and repair it backwards. + if is_thinking_mandatory(e): + retry_kwargs = strip_thinking(kwargs) + if retry_kwargs is kwargs: + raise e + MODELS_REQUIRING_THINKING.add(key) + logger.warning( + f'{model} does not allow thinking to be switched off; ' + f'retrying without any thinking parameter (it stays that way ' + f'for this model): {e}') + return create(**retry_kwargs) + if not is_thinking_refusal(e): + raise e + retry_kwargs = without_thinking(kwargs) + if retry_kwargs is kwargs: # we asked for no thinking; not our 400 + raise e + MODELS_REFUSING_THINKING.add(key) + logger.warning( + f'{model} rejected the thinking parameters; retrying with ' + f'thinking off (it stays off for this model): {e}') + return create(**retry_kwargs) + + try: + result = create(**kwargs) + except Exception as e: + return _repair(e) + return retry_on_first_chunk(result, _repair) diff --git a/ms_agent/llm/transport/anthropic_messages.py b/ms_agent/llm/transport/anthropic_messages.py index 6af1b2848..66231c4f3 100644 --- a/ms_agent/llm/transport/anthropic_messages.py +++ b/ms_agent/llm/transport/anthropic_messages.py @@ -15,9 +15,15 @@ import json from typing import Any, Dict, Generator, Iterator, List, Optional, Union +from ms_agent.llm import multimodal +from ms_agent.llm.thinking import apply_effort, create_with_thinking_fallback from ms_agent.llm.transport.base import Transport from ms_agent.llm.utils import Message, Tool, ToolCall -from ms_agent.utils import assert_package_exist +from ms_agent.llm.vision import create_with_vision_fallback +from ms_agent.llm.vision import disabled_reason as vision_disabled_reason +from ms_agent.utils import assert_package_exist, get_logger + +logger = get_logger() class AnthropicMessagesTransport(Transport): @@ -28,6 +34,8 @@ def __init__( api_key: Optional[str], base_url: str, generation_config: Optional[Dict] = None, + vision: Optional['multimodal.VisionOptions'] = None, + vision_supported: bool = True, ): assert_package_exist('anthropic', 'anthropic') import anthropic @@ -35,6 +43,11 @@ def __init__( if not api_key: raise ValueError('Anthropic API key is required.') + # See OpenAICompatTransport for why these are explicit params rather + # than generation_config keys. + self._vision = vision or multimodal.VisionOptions() + self._vision_supported = bool(vision_supported) + self.model = model self.client = anthropic.Anthropic(api_key=api_key, base_url=base_url) self.args: Dict = dict(generation_config or {}) @@ -83,6 +96,55 @@ def _as_tool_input(value: Any) -> Any: return {} return value if value is not None else {} + @staticmethod + def _blocks_from_structured(content: List[Any]) -> List[Dict[str, Any]]: + """Translate an OpenAI-shaped content array to Anthropic blocks. + + Only reachable when a caller hands us structured content directly; our + own attachment path builds Anthropic blocks natively. Unknown block + kinds degrade to their text payload rather than being dropped silently. + """ + out: List[Dict[str, Any]] = [] + for item in content: + if not isinstance(item, dict): + out.append({'type': 'text', 'text': str(item)}) + continue + kind = item.get('type') + if kind == 'text': + text = str(item.get('text') or '') + if text: + out.append({'type': 'text', 'text': text}) + elif kind == 'image': # already Anthropic-shaped + out.append(item) + elif kind in ('image_url', 'input_image'): + url = item.get('image_url') + url = url.get('url') if isinstance(url, dict) else url + url = str(url or '') + if url.startswith('data:') and ',' in url: + header, data = url.split(',', 1) + media_type = header[5:].split(';')[0] or 'image/png' + out.append({ + 'type': 'image', + 'source': { + 'type': 'base64', + 'media_type': media_type, + 'data': data, + }, + }) + elif url: + out.append({ + 'type': 'image', + 'source': { + 'type': 'url', + 'url': url + }, + }) + else: + text = str(item.get('text') or '') + if text: + out.append({'type': 'text', 'text': text}) + return out + def _format_input_message(self, messages: List[Message]) -> List[Dict[str, Any]]: formatted_messages = [] @@ -106,7 +168,28 @@ def _format_input_message(self, if signature: thinking_block['signature'] = signature content.append(thinking_block) - if msg.content: + attachments = getattr(msg, 'attachments', None) or [] + if attachments and msg.role == 'user': + # Image refs -> native image blocks (image-then-text, each + # introduced by its own "Image N: " label). + built = multimodal.anthropic_content( + msg.content if isinstance(msg.content, str) else '', + attachments, + self._vision, + vision_supported=self._vision_supported, + disabled_reason=vision_disabled_reason( + getattr(self.client, 'base_url', ''), self.model)) + if isinstance(built, list): + content.extend(built) + elif built: + content.append({'type': 'text', 'text': str(built)}) + elif isinstance(msg.content, list): + # Already-structured content (an image_url list handed in by an + # SDK caller, or our own blocks on a replayed turn). Passing it + # to _as_text would JSON-serialize the whole array into ONE text + # block, silently destroying every image; convert instead. + content.extend(self._blocks_from_structured(msg.content)) + elif msg.content: content.append({ 'type': 'text', 'text': self._as_text(msg.content) @@ -129,10 +212,24 @@ def _format_input_message(self, if msg.role == 'tool': tool_use_id = msg.tool_call_id or (pending_tool_ids.pop(0) if pending_tool_ids else '') + # This protocol DOES allow image blocks inside tool_result, so a + # tool's images stay attached to the call that produced them — + # better than the hoist the OpenAI transports are forced into, + # because the association survives regardless of message order. + result_content: Any = self._as_text(msg.content) + image_blocks = multimodal.anthropic_tool_result_blocks( + attachments, + self._vision, + vision_supported=self._vision_supported) + if image_blocks: + text = result_content + result_content = [*image_blocks] + if text: + result_content.append({'type': 'text', 'text': text}) result_block = { 'type': 'tool_result', 'tool_use_id': tool_use_id, - 'content': self._as_text(msg.content), + 'content': result_content, } # Anthropic requires ALL tool_results for one assistant turn's # tool_use blocks in the SINGLE user message immediately after it. @@ -169,6 +266,9 @@ def _call_llm(self, system = formatted_messages[0]['content'] formatted_messages = formatted_messages[1:] + # Already lowered in `generate()` — it has to happen before the + # signature filter there, and doing it twice is destructive (see the + # note in transport/openai_compat.py). max_tokens = kwargs.pop('max_tokens', 16000) extra_body = kwargs.get('extra_body', {}) enable_thinking = extra_body.get('enable_thinking', False) @@ -189,9 +289,54 @@ def _call_llm(self, params['tools'] = tools params.update(kwargs) - if stream: - return self.client.messages.stream(**params) - return self.client.messages.create(**params) + # Same per-model hard-400 hazard as the OpenAI-family transports: a model + # that cannot accept images rejects the whole request rather than + # ignoring the blocks. Retry once with the images folded into text and + # remember the model. Symmetric with OpenAICompatTransport so behaviour + # does not depend on which protocol a gateway happens to speak. + sent_images = any( + multimodal.has_image_blocks(m.get('content')) + for m in formatted_messages if isinstance(m, dict)) + + def _send(messages, **kw): + call = dict(kw) + call['messages'] = messages + # `model` is a named parameter of create_with_vision_fallback (it + # keys the per-model refusal memo), so it is consumed there rather + # than forwarded — the API call has to name it again itself. + call['model'] = self.model + if stream: + return self.client.messages.stream(**call) + return self.client.messages.create(**call) + + # Thinking is the OTHER per-model hard-400, and this transport used to + # be the one family without the repair: a Messages-API gateway fronting + # a model that cannot think rejected the `thinking` block outright and + # the error went straight to the user. Nested INSIDE the vision + # fallback, exactly as in the OpenAI-family transports, so the two + # retries compose instead of masking each other. + def _create(messages, **kw): + return create_with_thinking_fallback( + lambda **kw2: _send(messages, **kw2), + self.client, + self.model, + logger, + **kw) + + # Everything except `model` and `messages`, which the wrapper takes as + # named arguments; leaving either in `params` would collide with them. + rest = { + k: v + for k, v in params.items() if k not in ('model', 'messages') + } + return create_with_vision_fallback( + _create, + base_url=getattr(self.client, 'base_url', ''), + model=self.model, + messages=params['messages'], + sent_images=sent_images, + logger_=logger, + **rest) def generate( self, @@ -204,6 +349,11 @@ def generate( args.update(kwargs) stream = args.pop('stream', False) + # Before the signature filter: `reasoning_effort` is not a Messages API + # parameter, so filtering first would drop the knob instead of lowering + # it into this protocol's `thinking` block. + args = apply_effort(args, base_url='', protocol='anthropic') + sig_params = inspect.signature(self.client.messages.create).parameters filtered_args = {k: v for k, v in args.items() if k in sig_params} diff --git a/ms_agent/llm/transport/openai_compat.py b/ms_agent/llm/transport/openai_compat.py index 20bdf25ce..6cfe62f9b 100644 --- a/ms_agent/llm/transport/openai_compat.py +++ b/ms_agent/llm/transport/openai_compat.py @@ -22,12 +22,31 @@ from copy import deepcopy from typing import Any, Dict, Generator, Iterable, List, Optional, Union +from ms_agent.llm import multimodal +from ms_agent.llm.thinking import apply_effort, create_with_thinking_fallback from ms_agent.llm.transport.base import Transport from ms_agent.llm.utils import Message, Tool, ToolCall +from ms_agent.llm.vision import create_with_vision_fallback +from ms_agent.llm.vision import disabled_reason as vision_disabled_reason from ms_agent.utils import MAX_CONTINUE_RUNS, assert_package_exist, get_logger logger = get_logger() +#: Field names carrying the model's reasoning, in preference order. Most +#: OpenAI-compatible vendors use ``reasoning_content`` (DashScope, ModelScope, +#: Zhipu, DeepSeek); OpenRouter normalizes everything it proxies into +#: ``reasoning`` instead, so reading only the first name made every model +#: routed through it look like it never thought. +_REASONING_FIELDS = ('reasoning_content', 'reasoning') + + +def _reasoning_of(delta_or_message: Any) -> str: + for field in _REASONING_FIELDS: + value = getattr(delta_or_message, field, None) + if value: + return value + return '' + class OpenAICompatTransport(Transport): # Fields forwarded to the API. Includes continue-gen flags (partial/prefix) @@ -49,10 +68,21 @@ def __init__( continue_gen_stop: Optional[List[str]] = None, max_continue_runs: Optional[int] = None, strip_reasoning_tags: bool = False, + vision: Optional['multimodal.VisionOptions'] = None, + vision_supported: bool = True, ): assert_package_exist('openai') import openai + # Image-attachment handling. Passed explicitly rather than via + # generation_config because that dict is forwarded wholesale as API + # kwargs (`self._call_llm(..., **args)`), so a private key in it would + # be sent to the endpoint and rejected. + self._vision = vision or multimodal.VisionOptions() + # Whether THIS model accepts images. Resolved by the caller from the + # per-model capability flag; False degrades attachments to text. + self._vision_supported = bool(vision_supported) + self.model = model self.base_url = self._normalize_base_url(base_url) self.client = openai.OpenAI(api_key=api_key, base_url=self.base_url) @@ -180,17 +210,40 @@ def _format_input_message(self, # disappears from the dict entirely rather than arriving as None. pending_tool_ids: List[str] = [] for idx, message in enumerate(messages): + # Image refs must be read BEFORE to_dict_clean(), which strips them + # (they are this method's input, never wire output). + attachments = (message.attachments if isinstance(message, Message) + else message.get('attachments')) or [] if isinstance(message, Message): if isinstance(message.content, str): message.content = message.content.strip() message = message.to_dict_clean() else: message = dict(message) + message.pop('attachments', None) content = message.get('content', '') if isinstance(content, str): content = content.strip() + # Expand image refs into native blocks. Text-only turns come back + # as the same plain string, so nothing changes for them (prefix + # caching included). + # Not for a tool message: the Chat Completions SCHEMA allows + # only text parts in `role: "tool"`, so its images go to the + # synthetic user turn appended after it (below). Five compatible + # providers were measured to accept inline image parts here anyway, + # but hoisting is valid under the schema AND under all of them — see + # multimodal.TOOL_MEDIA_PROMPT for the full measurement. + if attachments and message.get('role') != 'tool': + content = multimodal.openai_content( + content, + attachments, + self._vision, + vision_supported=self._vision_supported, + disabled_reason=vision_disabled_reason( + getattr(self.client, 'base_url', ''), self.model)) + if cache_indice is not None and idx == cache_indice: content = self._to_structured_content( content, @@ -231,6 +284,17 @@ def _format_input_message(self, 'will likely reject this request') openai_messages.append(formatted_message) + + # A tool result's images ride on a synthetic user turn right + # after it, because the Chat Completions schema restricts a tool + # message to text parts (see multimodal.TOOL_MEDIA_PROMPT). + if attachments and role == 'tool': + media = multimodal.openai_tool_media_message( + attachments, + self._vision, + vision_supported=self._vision_supported) + if media is not None: + openai_messages.append(media) return openai_messages # ------------------------------------------------------------------ # @@ -248,6 +312,14 @@ def generate( args = self.args.copy() args.update(kwargs) stream = args.get('stream', False) + # NOT lowered here — `_call_llm` does it, exactly once per request. + # Lowering twice is destructive rather than idempotent: the canonical + # key and DashScope's wire key are both spelled `reasoning_effort`, so a + # second pass reads the `enable_thinking` the first pass just added as + # "the caller is driving thinking by hand" and stands down, deleting our + # own tier. `reasoning_effort` is a real OpenAI parameter, so it survives + # the filter below and reaches `_call_llm` intact; continuation calls + # re-lower from the same canonical value. args = {key: value for key, value in args.items() if key in parameters} # Format tools once and thread the formatted list through the @@ -339,8 +411,33 @@ def _call_llm(self, stream_options_config = self.args.get('stream_options', {}) if is_streaming and stream_options_config.get('include_usage', True): kwargs.setdefault('stream_options', {})['include_usage'] = True - return self.client.chat.completions.create( - model=self.model, messages=messages, tools=tools, **kwargs) + # `reasoning_effort` is the one knob callers set; each endpoint spells it + # differently, so lower it here — as late as possible, when the base_url + # that decides the spelling is known. Thinking is also per-model and a + # refusal is a hard 400. Both live in llm/thinking.py. + kwargs = apply_effort( + kwargs, base_url=str(getattr(self.client, 'base_url', ''))) + + # Image content is the other per-model hard-400: a text-only model + # rejects the whole request rather than ignoring the image blocks, which + # would make it unusable the moment a user attaches a file. Retry once + # with the images folded into text, and remember the model. Wrapped + # OUTSIDE the thinking fallback so the two compose: a request can be + # retried for thinking and, independently, for images. + sent_images = any( + multimodal.has_image_blocks(m.get('content')) + for m in messages if isinstance(m, dict)) + return create_with_vision_fallback( + lambda messages, **kw: create_with_thinking_fallback( + lambda **kw2: self.client.chat.completions.create( + model=self.model, messages=messages, tools=tools, **kw2), + self.client, self.model, logger, **kw), + base_url=getattr(self.client, 'base_url', ''), + model=self.model, + messages=messages, + sent_images=sent_images, + logger_=logger, + **kwargs) # ------------------------------------------------------------------ # # usage @@ -507,8 +604,7 @@ def _stream_format_output_message(completion_chunk) -> Message: content = '' if completion_chunk.choices and completion_chunk.choices[0].delta: content = completion_chunk.choices[0].delta.content - reasoning_content = getattr(completion_chunk.choices[0].delta, - 'reasoning_content', '') + reasoning_content = _reasoning_of(completion_chunk.choices[0].delta) if completion_chunk.choices[0].delta.tool_calls: func = completion_chunk.choices[0].delta.tool_calls tool_calls = [ @@ -538,11 +634,7 @@ def _stream_format_output_message(completion_chunk) -> Message: @staticmethod def _format_output_message(completion) -> Message: content = completion.choices[0].message.content or '' - if hasattr(completion.choices[0].message, 'reasoning_content'): - reasoning_content = completion.choices[ - 0].message.reasoning_content or '' - else: - reasoning_content = '' + reasoning_content = _reasoning_of(completion.choices[0].message) tool_calls = None if completion.choices[0].message.tool_calls: tool_calls = [ diff --git a/ms_agent/llm/utils.py b/ms_agent/llm/utils.py index ec568d976..09a04a444 100644 --- a/ms_agent/llm/utils.py +++ b/ms_agent/llm/utils.py @@ -107,6 +107,30 @@ class Message: # the model provider (the model still sees the failure via ``content``). is_error: bool = False + # Non-text parts riding alongside ``content`` — today only images. Ordered: + # entry i is "Image i+1" to the model, and the UI must show its chips in the + # same order or "the second image" points at the wrong one. + # + # Each entry is a REFERENCE, not bytes: + # {'type': 'image', 'path': 'user_files/a.png', + # 'media_type': 'image/png', 'label': 'Image 1: a.png'} + # + # Deliberately NOT folded into ``content``: this framework has ~40 call + # sites that read a user message's ``content`` expecting a string (memory + # extraction, full-text indexing, session auto-naming, summary compaction, + # snapshot labels, hook prompt extraction). None of them wants to know how + # many images there are, and none of them would crash loudly if handed a + # block list — they would silently store a Python repr or skip the turn. + # Keeping ``content`` a str keeps all of them correct for free; the + # transports expand these refs into provider-native blocks at the wire. + # + # Storing a reference rather than base64 is what lets the same log be + # re-encoded per provider (Anthropic 1568px/2576px tiers, DashScope not + # accepting GIF) and lets a session survive a model switch: swap to a + # text-only model and the refs degrade to text placeholders; swap back and + # the images are visible again. + attachments: List[Dict[str, Any]] = field(default_factory=list) + def to_dict(self): return asdict(self) @@ -139,6 +163,13 @@ def to_dict_clean(self): 'searching_detail', 'search_result', '_responses_output_items', + # Image refs are the transports' input, never wire output: each + # provider adapter reads ``message.attachments`` BEFORE calling this + # and folds them into provider-native content blocks. Leaving them + # in would ship a bare {'type':'image','path':...} to the endpoint. + # This entry is load-bearing: to_dict_clean() keeps every truthy + # field not listed here, so omitting it leaks the refs. + 'attachments', ] return { key: value @@ -161,6 +192,12 @@ class ToolResult: tool_detail: Optional[str] = None hook_attachments: List[Any] = field(default_factory=list) is_error: bool = False + #: Non-text parts the tool produced (images), same reference shape as + #: ``Message.attachments``. ``text`` still carries a short human/model + #: readable status; these carry the pixels. Splitting them is the point: a + #: tool that put base64 into ``text`` was writing into the one channel a + #: model cannot decode. + attachments: List[Dict[str, Any]] = field(default_factory=list) @staticmethod def from_raw(raw): @@ -177,6 +214,7 @@ def from_raw(raw): tool_detail=None if td is None else str(td), hook_attachments=raw.get('hook_attachments', []), is_error=bool(raw.get('is_error', False)), + attachments=raw.get('attachments', []) or [], extra={ k: v for k, v in raw.items() if k not in [ @@ -186,6 +224,7 @@ def from_raw(raw): 'tool_detail', 'hook_attachments', 'is_error', + 'attachments', ] }) raise TypeError('tool_call_result must be str or dict') diff --git a/ms_agent/llm/vision.py b/ms_agent/llm/vision.py new file mode 100644 index 000000000..6a1f4985d --- /dev/null +++ b/ms_agent/llm/vision.py @@ -0,0 +1,294 @@ +# Copyright (c) ModelScope Contributors. All rights reserved. +"""Which models can be shown an image, and what to do when we guess wrong. + +Two halves: + +**Resolution** — whether to attach pixels at all. Two states only, and the +default is OFF: + +1. an explicit per-model setting (the "image understanding" switch in the model + form) — the user's own statement, and the only thing that turns images ON; +2. a model observed to REFUSE images earlier in this process vetoes it, because + a refusal is ground truth. + +Deliberately NOT consulted: the provider's declared ``vision`` capability. +Vision is a property of the MODEL, not of the endpoint — ModelScope serves +``Qwen3-VL-8B-Instruct`` and the text-only ``Qwen3-235B-A22B`` through one +provider entry, so a provider-level flag says yes to both. It used to be the +middle tier here, and because nine of ten registry entries declare ``vision`` +it made "nobody has said" mean "send images", i.e. the switch's OFF position +described a state the runtime never actually used. ``ProviderCapability.VISION`` +still exists and is still correct about what the *protocol* accepts; it is just +not evidence about a particular model's eyesight. + +Whether a model can really see is therefore the user's call. There is no +probing: a model that accepts image blocks with HTTP 200 and cannot read them +(measured: zhipu glm-5.x, MiniMax-M2.7, and ModelScope's Qwen3-235B-A22B, which +answered with an invented string) is indistinguishable at runtime from one that +can. + +**Self-healing** — a provider that cannot see images rejects the whole request +with a hard 400, which would otherwise make such a model unusable the moment a +user attaches a file. So the request is retried once with the images replaced by +text, and the model is remembered so a session pays that round-trip at most once. + +The refusal detector deliberately does **no keyword matching**. Measured against +DashScope (2026-08), a text-only model given an ``image_url`` block answers:: + + <400> InternalError.Algo.InvalidParameter: The provided messages input is + invalid. The error info is [Unexpected item type in content.] + +— which names neither "image" nor "multimodal" nor "vision". Any keyword list +built from a vendor's current phrasing is a guess that goes stale. What we *do* +know for certain is whether the request we just sent carried image blocks; that +fact plus a 400 is the attribution. Mirrors ``llm/thinking.py``, which exists +because a hand-maintained model blocklist was wrong twice before it. +""" +from __future__ import annotations + +from typing import Any, List, Optional, Set, Tuple + +from ms_agent.llm import multimodal +from ms_agent.utils import get_logger + +logger = get_logger() + +#: ``(base_url, model)`` pairs observed to reject image content. +MODELS_REFUSING_IMAGES: Set[Tuple[str, str]] = set() + +#: Callables notified the first time a model is learned to refuse images, so a +#: host (the WebUI) can TELL THE USER. Deliberately not a write-back hook: the +#: switch is the user's statement about their own model, and silently rewriting +#: it would both contradict them and hide the reason. The memo below keeps the +#: session from paying the failed round-trip twice; making it permanent is the +#: user's decision to make in the model form. +_OBSERVERS: List[Any] = [] + + +def register_refusal_observer(fn) -> None: + """Register ``fn(base_url, model)``, called once per newly-learned refusal. + + Idempotent per callable, so repeated setup (a WebUI reload) cannot stack + duplicate notifications. Observer exceptions are swallowed: learning that a + model refuses images must never be able to fail the turn that discovered it. + """ + if fn not in _OBSERVERS: + _OBSERVERS.append(fn) + + +def model_key(base_url: Any, model: str) -> Tuple[str, str]: + return (str(base_url or ''), str(model or '')) + + +def note_refusal(base_url: Any, model: str) -> None: + key = model_key(base_url, model) + first_time = key not in MODELS_REFUSING_IMAGES + MODELS_REFUSING_IMAGES.add(key) + if not first_time: + return + for observer in list(_OBSERVERS): + try: + observer(key[0], key[1]) + except Exception as exc: # never fail the turn over bookkeeping + logger.warning('[vision] refusal observer failed: %s', exc) + + +def known_refuser(base_url: Any, model: str) -> bool: + return model_key(base_url, model) in MODELS_REFUSING_IMAGES + + +def disabled_reason(base_url: Any = '', model: str = '') -> str: + """Why this turn's images are text placeholders, for the model to relay. + + A model whose switch is off should be told to turn it on; a model whose + switch is ON but whose endpoint rejected the images must NOT be, or it + sends the user back to a box they already ticked. + """ + if model and known_refuser(base_url, model): + return multimodal.REASON_REJECTED + return multimodal.REASON_DISABLED + + +def _status_of(exc: Exception) -> Optional[int]: + status = getattr(exc, 'status_code', None) + if status is None: + response = getattr(exc, 'response', None) + status = getattr(response, 'status_code', None) + try: + return int(status) if status is not None else None + except (TypeError, ValueError): + return None + + +def is_image_refusal(exc: Exception, sent_images: bool) -> bool: + """True when a 400 is attributable to the images in THIS request. + + ``sent_images`` is the whole detector: we know what we put on the wire, and + guessing the vendor's wording does not work (see the module docstring). + + This is deliberately a WIDE net — it says "worth one retry", not "definitely + the images". Measured across seven providers, a 400 on an image-carrying + request also covers model-not-found ("Model id ... has no provider + supported" on ModelScope), auth failures and content filters. The + discrimination therefore happens in ``create_with_vision_fallback``, which + only blacklists the model when the image-less retry actually SUCCEEDS; a 400 + that persists without images is re-raised untouched and teaches us nothing. + + So the cost of a false positive is exactly one extra round-trip, and it can + never mask the real error or wrongly disable images on a capable model. + """ + if not sent_images: + return False # a 400 with no images in it is somebody else's problem + status = _status_of(exc) + if status is not None: + return status == 400 + # Some SDK wrappers lose the status; fall back to the textual marker. + return '400' in str(exc) + + +def strip_images_from_messages(messages: Any) -> Tuple[Any, bool]: + """``(messages, changed)`` with every image block folded back into text. + + Operates on the already-formatted provider payload, so it works for both the + OpenAI ``image_url`` shape and the Anthropic ``image``/``source`` shape. + """ + if not isinstance(messages, list): + return messages, False + changed = False + out = [] + for message in messages: + if not isinstance(message, dict): + out.append(message) + continue + content = message.get('content') + if multimodal.has_image_blocks(content): + message = { + **message, 'content': multimodal.strip_image_blocks(content) + } + changed = True + out.append(message) + return out, changed + + +def create_with_vision_fallback(create, + *, + base_url: Any, + model: str, + messages: Any, + sent_images: bool, + logger_=None, + **kwargs) -> Any: + """Call ``create(messages=..., **kwargs)``, retrying once without images. + + ``create`` must accept ``messages`` as a keyword so the retry can hand it a + rewritten list. + + Streaming is covered in BOTH shapes: clients that issue the request eagerly + raise out of ``create`` itself, and gateways that answer 200 before + rejecting the image blocks raise on the first chunk (see + ``llm/stream_retry.py``). + """ + from ms_agent.llm.stream_retry import retry_on_first_chunk + + log = logger_ or logger + if sent_images and known_refuser(base_url, model): + messages, _ = strip_images_from_messages(messages) + sent_images = False + + def _remember() -> None: + note_refusal(base_url, model) + log.warning( + 'images stay off for %s for the rest of this process (the ' + 'image-less retry succeeded)', model) + + def _confirm(result: Any, original: BaseException) -> Any: + """Blacklist only once the image-less attempt actually produces output. + + For a non-streaming call "returned" already means "succeeded". For a + stream it does not: the replacement can still fail on its own first + chunk, and treating that as proof would blacklist a model whose real + problem was something else entirely. + """ + if not hasattr(result, '__next__'): + _remember() + return result + + def _guarded(): + try: + first = next(result) + except StopIteration: + _remember() # empty, but the endpoint accepted it + return + except Exception: + raise original from None # the images were not the cause + _remember() + yield first + yield from result + + return _guarded() + + def _repair(exc: BaseException) -> Any: + if not is_image_refusal(exc, sent_images): + raise exc + retry_messages, changed = strip_images_from_messages(messages) + if not changed: + raise exc + log.warning( + '%s returned 400 on a request carrying images; retrying once with ' + 'the images replaced by text: %s', model, exc) + try: + result = create(messages=retry_messages, **kwargs) + except Exception: + # Removing the images did NOT help, so they were not the cause — + # this was a model-not-found / auth / content-filter 400 that merely + # happened to ride on a turn with an attachment. Re-raise the + # ORIGINAL error (it describes the real problem) and, crucially, do + # not blacklist the model: marking a vision-capable model as + # image-refusing here would silently stop sending it images for the + # rest of the process. Measured on ModelScope, whose "Model id ... + # has no provider supported" is exactly this shape. + raise exc from None + return _confirm(result, exc) + + try: + result = create(messages=messages, **kwargs) + except Exception as exc: + return _repair(exc) + return retry_on_first_chunk(result, _repair) + + +def resolve_supports_vision(config: Any, + spec: Any = None, + model: str = '', + base_url: Any = '') -> bool: + """Whether to attach pixels for this model. See the module docstring. + + ``config.llm.supports_vision`` is the explicit per-model switch and the only + thing that turns images on; unset means OFF. ``spec`` is accepted and ignored + (kept so existing callers need no edit): a provider's declared ``vision`` + capability describes the protocol, not the model behind it. + """ + if model and known_refuser(base_url, model): + return False # observed truth beats the switch + + llm = getattr(config, 'llm', None) if config is not None else None + if llm is not None: + for name in ('supports_vision', 'vision_supported'): + value = getattr(llm, name, None) + if value is not None: + return _as_bool(value) + return False + + +def _as_bool(value: Any) -> bool: + """Tolerate a YAML/JSON boolean written as a string. + + ``supports_vision: "false"`` is a common enough mistake that treating it as + truthy (which bare ``bool()`` does) would silently enable images on a model + the user just tried to turn them off for. + """ + if isinstance(value, bool): + return value + if isinstance(value, str): + return value.strip().lower() in ('1', 'true', 'yes', 'on', 'y') + return bool(value) diff --git a/ms_agent/memory/memory_manager.py b/ms_agent/memory/memory_manager.py index 77b1ce312..80eea4fba 100644 --- a/ms_agent/memory/memory_manager.py +++ b/ms_agent/memory/memory_manager.py @@ -17,7 +17,12 @@ class SharedMemoryManager: @classmethod async def get_shared_memory(cls, config: DictConfig, mem_instance_type: str) -> Memory: - """Get or create a shared memory instance based on configuration.""" + """Get or create the shared memory instance for this config's store. + + An existing instance is reconfigured in place when the incoming config + differs, so the caller always gets an instance that matches what it + asked for. + """ node = getattr(config.memory, mem_instance_type, OmegaConf.create({})) # unified_memory namespaces the user under `namespace.user_id`; # legacy memories keep a top-level `user_id`. Honor both. @@ -31,18 +36,37 @@ async def get_shared_memory(cls, config: DictConfig, path: str = getattr(node, 'path', None) or getattr( config, 'output_dir', None) or DEFAULT_OUTPUT_DIR path = os.path.abspath(os.path.expanduser(str(path))) - llm_str: str = getattr(config.llm, 'model', 'default_model') - key = f'{mem_instance_type}_{user_id}_{llm_str}_{path}' + # One instance per (memory type, user, store) — deliberately NOT + # keyed by the agent's model. Embedded stores (mem0 + local qdrant) + # take an exclusive file lock, so a second instance on the same path + # cannot open the store at all: keying by model meant that switching + # models with a store already open silently disabled memory for the + # new agent. A configuration change is handled by reconfiguring the + # live instance below, not by keeping two of them. + key = f'{mem_instance_type}_{user_id}_{path}' - if key not in cls._instances: + instance = cls._instances.get(key) + if instance is None: logger.info(f'Creating new shared memory instance for key: {key}') - cls._instances[key] = memory_mapping[mem_instance_type](config) - else: - logger.info( - f'Reusing existing shared memory instance for key: {key}') + instance = memory_mapping[mem_instance_type](config) + cls._instances[key] = instance + return instance - return cls._instances[key] + logger.info(f'Reusing existing shared memory instance for key: {key}') + # The cached instance was built from whatever config the FIRST agent + # had. Later agents may carry a changed one (the user edited the + # project's memory models, recall size, ...), and silently serving the + # old config is indistinguishable from "the setting does nothing". + reconfigure = getattr(instance, 'reconfigure', None) + if reconfigure is not None: + try: + await reconfigure(config) + except Exception as e: # noqa: BLE001 - never fail agent startup + logger.warning( + f'Reconfiguring shared memory {key} failed, keeping the ' + f'existing configuration: {e}') + return instance @classmethod async def close_matching(cls, base_dir: str) -> int: diff --git a/ms_agent/memory/unified/backends/mem0_adapter.py b/ms_agent/memory/unified/backends/mem0_adapter.py index 1990ccd41..b1de7d057 100644 --- a/ms_agent/memory/unified/backends/mem0_adapter.py +++ b/ms_agent/memory/unified/backends/mem0_adapter.py @@ -53,6 +53,24 @@ def _result_list(results: Any) -> List[Dict[str, Any]]: return list(results or []) +# Extraction guidance we add to mem0's own, for one reason: retrieval. The +# store is queried with the user's next message, so a memory kept in a language +# the user does not write in has to survive a cross-lingual embedding hop — and +# mem0 2.x's hybrid search also runs a BM25 leg, where cross-language lexical +# overlap is simply zero. (Observed: Chinese questions returning nothing at all +# against English-worded memories.) +# +# mem0 2.x treats `custom_instructions` as an EXTRA, highest-priority section +# appended to its prompt — it does not replace the built-in one. (1.x's +# `custom_fact_extraction_prompt`, which did replace it wholesale, no longer +# exists in the config.) Overridable: an explicit `custom_instructions` in +# `backend_options.mem0` wins. +DEFAULT_CUSTOM_INSTRUCTIONS = ( + 'Write each memory in the SAME language and script the user used — do not ' + 'translate or transliterate it. Keep names, technical terms and product ' + 'names exactly as they appeared.') + + def _mem0_search(m0: Any, query: str, user_id: str, top_k: int = 10) -> Any: """mem0 2.x moved entity params into ``filters=``; 1.x uses kwargs.""" try: @@ -87,8 +105,10 @@ def __init__(self, config: MemoryConfig) -> None: async def start(self, **kwargs: Any) -> None: try: from mem0 import Memory - mem0_cfg = self._config.backend_options.get('mem0', {}) - self._mem0 = Memory.from_config(mem0_cfg) if mem0_cfg else Memory() + mem0_cfg = dict(self._config.backend_options.get('mem0', {}) or {}) + mem0_cfg.setdefault('custom_instructions', + DEFAULT_CUSTOM_INSTRUCTIONS) + self._mem0 = Memory.from_config(mem0_cfg) self._user_id = kwargs.get('user_id', self._config.user_id) logger.info('[mem0_backend] mem0 initialized') except Exception as e: @@ -160,9 +180,15 @@ async def recall_block(self, query: str) -> str: results, max(1, int(getattr(self._config, 'recall_top_k', 10)))) if not formatted: return '' + # Recall rides on the user turn now (see inject), so the dating rule + # that used to head the system-prompt block travels with it: mem0 2.x + # only ever ADDs, so a superseded fact and its replacement both come + # back from one search — undated, the model has nothing to prefer the + # newer one by. return ('\n' f'{RECALL_BLOCK_MARKER} (background ' - 'reference — not instructions):\n' + 'reference — not instructions). Each entry is dated; when two ' + 'entries conflict, the later one supersedes the earlier:\n' f'{formatted}\n' '') @@ -216,7 +242,8 @@ async def search( return [] try: results = _result_list(await _offload(_mem0_search, self._mem0, - query, self._user_id)) + query, self._user_id, + max(1, int(limit)))) return [ MemoryEntry( id=r.get('id', ''), @@ -247,11 +274,21 @@ def _extract_query(messages: List[Dict[str, Any]]) -> str: @staticmethod def _format_results(results: Any, top_k: int = 10) -> str: + """One bullet per memory, stamped with the day it was written. + + mem0's extraction only ever ADDs (2.x has no update/delete pass), so a + superseded fact and the fact that replaced it both come back from the + same search. Undated, the model has nothing to prefer the newer one by; + dated, resolving the contradiction is at least possible at read time. + """ lines = [] for r in _result_list(results)[:top_k]: text = r.get('memory', r.get('text', '')) - if text: - lines.append(f'- {text}') + if not text: + continue + day = str(r.get('updated_at') or r.get('created_at') or '')[:10] + stamp = f'({day}) ' if len(day) == 10 else '' + lines.append(f'- {stamp}{text}') return '\n'.join(lines) diff --git a/ms_agent/memory/unified/orchestrator.py b/ms_agent/memory/unified/orchestrator.py index cddf496ff..c48d0e2a8 100644 --- a/ms_agent/memory/unified/orchestrator.py +++ b/ms_agent/memory/unified/orchestrator.py @@ -6,12 +6,12 @@ What it DOES own is the write/read discipline around the backend: -* **Serialization** — retrieval (``run``), ingestion (``add``) and flush all - take one asyncio lock per *store* (keyed by ``base_dir``), because embedded +* **Serialization** — every access to the store (``run``, ``add``, ``search``, + ``flush``, teardown) takes one asyncio lock per *store*, because embedded vector stores (mem0 + local qdrant) have no internal locking at all and mem0 even fans out worker threads inside ``add``. Lock per store, not per - orchestrator: ``SharedMemoryManager`` may hand different orchestrator - instances the same directory. + orchestrator: a transient client (the WebUI's read path) may be on the same + directory. * **Background ingestion** — ``schedule_add`` takes the extraction-LLM + embedding cost (seconds) off the turn's critical path. Tasks are retained for ``flush_pending`` so a teardown cannot silently drop the last write. @@ -36,6 +36,7 @@ import os import tempfile import time +from dataclasses import fields from typing import Any, Dict, List, Optional, Set from ms_agent.llm.utils import Message @@ -45,23 +46,41 @@ from ms_agent.session.context_assembler import _dicts_to_messages from ms_agent.utils.logger import get_logger from .config import MemoryConfig -from .protocols import (RECALL_BLOCK_MARKER, MemoryBackend, MemoryEntry) +from .protocols import RECALL_BLOCK_MARKER, MemoryBackend, MemoryEntry from .registry import backend_registry logger = get_logger() -# One lock per storage directory. Never per orchestrator instance: two -# orchestrators over the same path (possible through SharedMemoryManager's -# llm-dependent cache key) must still serialize against each other. -_STORE_LOCKS: Dict[str, asyncio.Lock] = {} +# One lock per storage directory. Never per orchestrator instance: the HTTP +# read path and any other transient client over the same path must serialize +# against this orchestrator's writes. +# +# Keyed by (loop, path), not path alone: an asyncio.Lock binds itself to the +# loop that first has to *wait* on it, and raises for good once a second loop +# contends. A process that runs more than one loop over its lifetime -- the +# inline `asyncio.run` ingest path below, a test suite calling asyncio.run per +# case, a notebook -- would otherwise wedge on a lock belonging to a loop that +# is already closed. Same shape as PermissionEnforcer._ask_lock_for_loop. +# (Two loops alive at once over one store still get one lock each and would not +# exclude each other; that is inherent to any per-loop scheme, and no such +# caller exists -- embedded stores are single-client by construction.) +_STORE_LOCKS: Dict[str, Any] = {} # path -> (loop, lock) def _store_lock(base_dir: str) -> asyncio.Lock: - key = os.path.abspath(str(base_dir or '.')) - lock = _STORE_LOCKS.get(key) - if lock is None: - lock = _STORE_LOCKS.setdefault(key, asyncio.Lock()) - return lock + path = os.path.abspath(str(base_dir or '.')) + try: + loop = asyncio.get_running_loop() + except RuntimeError: # sync caller: nothing to serialize against yet + loop = None + entry = _STORE_LOCKS.get(path) + # Identity of the loop object, not its id(): an id is reused after the loop + # is collected, which would hand back a lock bound to the dead one. + if entry is None or entry[0] is not loop: + lock = asyncio.Lock() + _STORE_LOCKS[path] = (loop, lock) + return lock + return entry[1] # Only conversational text is ingested (mirrors what backends extract from); @@ -71,12 +90,36 @@ def _store_lock(base_dir: str) -> asyncio.Lock: _LEDGER_FILE = 'ingest_state.json' _LEDGER_MAX = 4096 +# Config fields the backend re-reads from ``mem_config`` on every call, so a +# change to them can be applied by mutating the live config object. Everything +# else decides which backend exists or which store it points at, and needs a +# teardown (see ``MemoryOrchestrator.reconfigure``). +_SOFT_FIELDS = ('recall_top_k', 'ingest_interval') + def _content_hash(msg: Dict[str, Any]) -> str: raw = f"{msg.get('role', '')}\x1f{msg.get('content', '')}" return hashlib.sha256(raw.encode('utf-8')).hexdigest()[:16] +def _ingestable_hashes(msg_dicts: List[Dict[str, Any]]) -> Set[str]: + return { + _content_hash(m) + for m in msg_dicts + if m.get('role') in _INGEST_ROLES and m.get('content') + } + + +def _changed_fields(old: MemoryConfig, new: MemoryConfig) -> Set[str]: + # Field-by-field rather than ``asdict``: that deep-copies every value, + # and ``llm_config`` / ``backend_options`` can hold anything. + return { + f.name + for f in fields(old) + if getattr(old, f.name, None) != getattr(new, f.name, None) + } + + class MemoryOrchestrator(Memory): """Thin adapter between the ms-agent ``Memory`` ABC and a ``MemoryBackend`` implementation. @@ -96,8 +139,13 @@ def __init__(self, config: Any) -> None: self.mem_config = self._parse_config(config) self._backend: Optional[MemoryBackend] = None self._started = False + # Retired by close(): a closed orchestrator never reopens its store. + self._closed = False # Background ingestion bookkeeping (see module docstring). self._pending: Set[asyncio.Task] = set() + # Hashes a scheduled ingest has claimed but not yet written. The ledger + # may not advance past them from anywhere else (see mark_ingested). + self._inflight: Set[str] = set() self._turns_since_ingest = 0 self._ledger: Optional[Set[str]] = None # lazy-loaded from disk self._ledger_order: List[str] = [] @@ -133,7 +181,7 @@ async def _ensure_started(self, **kwargs: Any) -> MemoryBackend: # ------------------------------------------------------------------ async def run(self, messages: List[Message]) -> List[Message]: - if not self.mem_config.enabled: + if not self.mem_config.enabled or self._closed: return messages # Retrieval must not overlap a write: the embedded stores underneath @@ -157,8 +205,13 @@ async def run(self, messages: List[Message]) -> List[Message]: async def recall_block(self, query: str) -> str: """Formatted recall for a NEW user turn; '' when the backend has no per-query recall (file backend) or memory is disabled. Same store - lock as run() — retrieval must not overlap a write.""" - if not self.mem_config.enabled: + lock as run() — retrieval must not overlap a write. + + Closed is terminal here too: this is a store access like any other, + and going through ``_ensure_started`` after the owner released the + store would retake the embedded store's file lock. + """ + if not self.mem_config.enabled or self._closed: return '' async with _store_lock(self.mem_config.base_dir): backend = await self._ensure_started() @@ -195,9 +248,16 @@ def schedule_add(self, messages: List[Message], asyncio.run(self._ingest(msg_dicts, **kwargs)) return None self._status.update(state='scheduled', error=None) + # Claim the messages NOW, synchronously. A task does not start until + # the loop gets around to it, and an interrupt landing in that gap + # would otherwise mark them as ingested before the write ever looked. + claimed = _ingestable_hashes(msg_dicts) + self._inflight |= claimed task = loop.create_task(self._ingest(msg_dicts, **kwargs)) self._pending.add(task) task.add_done_callback(self._pending.discard) + task.add_done_callback( + lambda _t: self._inflight.difference_update(claimed)) return task def _should_ingest(self) -> bool: @@ -217,6 +277,16 @@ async def _ingest(self, msg_dicts: List[Dict[str, Any]], Never raises — a memory write must not break anything above it; the outcome lands in ``ingest_status`` instead. """ + if self._closed: + # Scheduled before the teardown, woken after it. Writing now would + # reopen a store its owner has already released. + logger.debug('[orchestrator] ingest dropped: orchestrator closed') + return 0 + # Also claimed here, not only in schedule_add: `add()` reaches this + # directly. Claiming twice is harmless — both releases drop the same + # hashes. + claimed = _ingestable_hashes(msg_dicts) + self._inflight |= claimed try: async with _store_lock(self.mem_config.base_dir): backend = await self._ensure_started() @@ -241,17 +311,25 @@ async def _ingest(self, msg_dicts: List[Dict[str, Any]], f'persisted for this turn: {type(e).__name__}: {e}') self._set_status('error', error=f'{type(e).__name__}: {e}') return 0 + finally: + self._inflight -= claimed def mark_ingested(self, messages: List[Message]) -> None: """Advance the ledger WITHOUT ingesting (interrupted turns): a - half-finished answer must not be swept into the next turn's delta.""" - self._ledger_mark( - [ - m for m in _messages_to_dicts(messages) - if m.get('role') in _INGEST_ROLES and m.get('content') - ], - persist=True, - ) + half-finished answer must not be swept into the next turn's delta. + + Pass ONLY the interrupted round's own messages. Anything a scheduled + ingest has already claimed is skipped regardless: marking a message + that is still being written would either make that write a no-op (the + delta comes out empty) or, if it fails, deny it the retry the ledger + exists to guarantee — either way the memory is silently lost. + """ + dicts = [ + m for m in _messages_to_dicts(messages) + if m.get('role') in _INGEST_ROLES and m.get('content') + and _content_hash(m) not in self._inflight + ] + self._ledger_mark(dicts, persist=True) async def flush_pending(self, timeout: float = 15.0) -> None: """Barrier: wait for scheduled ingests (teardown must not drop the @@ -349,7 +427,7 @@ def _ledger_mark(self, msg_dicts: List[Dict[str, Any]], # ------------------------------------------------------------------ async def flush(self, messages: List[Message]) -> None: - if not self.mem_config.enabled: + if not self.mem_config.enabled or self._closed: return async with _store_lock(self.mem_config.base_dir): backend = await self._ensure_started() @@ -361,8 +439,14 @@ async def flush(self, messages: List[Message]) -> None: # ------------------------------------------------------------------ async def search(self, query: str, limit: int = 10) -> List[MemoryEntry]: - backend = await self._ensure_started() - return await backend.search(query, limit) + if self._closed: + return [] + # Under the store lock like every other access: an embedded store has + # no locking of its own, and this one used to be the single reader that + # could land in the middle of a background write. + async with _store_lock(self.mem_config.base_dir): + backend = await self._ensure_started() + return await backend.search(query, limit) # ------------------------------------------------------------------ # Tool interface (called by the agent's ToolManager) @@ -405,15 +489,77 @@ def init_update_queue(self) -> None: # Shutdown # ------------------------------------------------------------------ - async def close(self) -> None: - # Drain scheduled writes first — closing under a pending ingest would - # either lose the write or race the backend teardown. + async def _shutdown_backend(self, retire: bool = False) -> None: + """Drain scheduled writes, then release the backend (and with it the + store's file lock). ``retire`` additionally makes the orchestrator + refuse to ever reopen the store; ``reconfigure`` leaves it False, + because everyone holding this instance must keep working with it. + + Order matters. Draining comes FIRST: a write that was already + scheduled is part of what a teardown promises to persist. Retiring + comes right after, so a straggler past ``flush_pending``'s timeout + finds the door closed instead of going through ``_ensure_started``, + which would restart the backend and take the embedded store's file + lock again — after its owner believed it released. + + Stragglers are deliberately NOT cancelled: a backend write runs in a + worker thread (mem0 extraction + embedding), so cancelling only + abandons the await while the thread keeps writing — and we would then + close the client under it. Waiting on the store lock below is what + actually makes the teardown safe. + """ await self.flush_pending() + if retire: + self._closed = True if self._backend is not None and self._started: async with _store_lock(self.mem_config.base_dir): await self._backend.close() self._started = False + async def close(self) -> None: + await self._shutdown_backend(retire=True) + + # ------------------------------------------------------------------ + # Reconfiguration + # ------------------------------------------------------------------ + + async def reconfigure(self, config: Any) -> bool: + """Adopt ``config`` on this live instance. Returns True when the + backend had to be torn down, False for a no-op or an in-place update. + + Applied to the instance instead of by replacing it, because + ``SharedMemoryManager`` hands ONE instance per store to every agent + that asks: a replacement would leave earlier holders pointing at an + orchestrator whose store was closed under them, and two live + orchestrators over one embedded store is exactly the exclusive-lock + conflict that sharing exists to prevent. Mutating the shared object + updates every holder at once. + """ + new_cfg = self._parse_config(config) + changed = _changed_fields(self.mem_config, new_cfg) + if not changed: + return False + if not changed - set(_SOFT_FIELDS): + # The live backend holds a reference to this very MemoryConfig, so + # writing the fields through reaches it without a teardown. + for field in _SOFT_FIELDS: + setattr(self.mem_config, field, getattr(new_cfg, field)) + logger.info(f'[orchestrator] memory config updated in place: ' + f'{sorted(changed)}') + return False + logger.info(f'[orchestrator] memory config changed ' + f'({sorted(changed)}) -> rebuilding backend') + # Release the old store, but keep the instance usable — everyone + # holding it must keep working, now against the new configuration. + await self._shutdown_backend() + self._backend = None + if 'base_dir' in changed: + # A different store keeps a different ledger. + self._ledger = None + self._ledger_order = [] + self.mem_config = new_cfg + return True + # ------------------------------------------------------------------ # Config parsing # ------------------------------------------------------------------ @@ -471,6 +617,12 @@ def _messages_to_dicts(messages: List[Message]) -> List[Dict[str, Any]]: d['reasoning_content'] = m.reasoning_content if m.reasoning_signature: d['reasoning_signature'] = m.reasoning_signature + # Image refs: this round-trip is the live LLM context, so a field + # dropped here is dropped from what the model sees this turn — not + # merely from storage (see the docstring above). Must mirror + # ``_dicts_to_messages``. + if getattr(m, 'attachments', None): + d['attachments'] = m.attachments result.append(d) else: result.append({'role': 'user', 'content': str(m)}) diff --git a/ms_agent/permission/config.py b/ms_agent/permission/config.py index 251647f8c..dde6a17aa 100644 --- a/ms_agent/permission/config.py +++ b/ms_agent/permission/config.py @@ -88,7 +88,18 @@ def _expand_dirs(raw: list[str]) -> tuple[str, ...]: ) -_DEFAULT_BLACKLIST: tuple[str, ...] = ( +#: Nothing by default. A blacklist entry can never be overridden — not by the +#: mode, not by a whitelist, not by the user answering a prompt — so it is the +#: wrong tool for "risky, ask first". The network-egress commands below used to +#: live here and were simply unusable: an agent asked to run ``curl`` reported +#: that it had been blocked and there was no way for the user to permit it. +_DEFAULT_BLACKLIST: tuple[str, ...] = () + +#: Commands that must be CONFIRMED rather than refused. Unlike the mode-level +#: default these hold in every mode, including full-access: reaching the network +#: or another host is worth one deliberate click even from a user who has +#: otherwise waved the agent through. ``allow_network: true`` drops them. +_DEFAULT_ASK_RULES: tuple[str, ...] = ( 'code_executor---shell_executor:curl *', 'code_executor---shell_executor:wget *', 'code_executor---shell_executor:ssh *', @@ -105,7 +116,10 @@ class PermissionConfig: mode: Literal['auto', 'strict', 'interactive'] = 'auto' whitelist: tuple[str, ...] = () blacklist: tuple[str, ...] = _DEFAULT_BLACKLIST - ask_rules: tuple[str, ...] = () + # Defaulted here as well as in from_dict: a config with no ``permission`` + # section at all takes the early return below, and the network commands + # must still be confirmed there. + ask_rules: tuple[str, ...] = _DEFAULT_ASK_RULES safety: SafetyConfig = SafetyConfig() @classmethod @@ -119,18 +133,19 @@ def from_dict(cls, _MODE_ALIASES = {'restricted': 'interactive'} mode = _MODE_ALIASES.get(raw_mode, raw_mode) whitelist = tuple(d.get('whitelist', ())) - ask_rules = tuple(d.get('ask_rules', ())) + user_ask_rules = tuple(d.get('ask_rules', ())) user_blacklist = tuple(d.get('blacklist', ())) - # The default blacklist blocks network-egress shell commands - # (curl/wget/ssh/...). ``allow_network: true`` (or legacy - # ``no_default_blacklist``) opts out of that secure default; the - # user's own blacklist entries still apply. + # Network-egress shell commands (curl/wget/ssh/...) are confirmed, not + # refused. ``allow_network: true`` (or legacy ``no_default_blacklist``) + # opts out of that confirmation; the user's own rules still apply. allow_network = bool( d.get('allow_network', False) or d.get('no_default_blacklist', False)) - base_blacklist = () if allow_network else _DEFAULT_BLACKLIST - blacklist = base_blacklist + tuple( - p for p in user_blacklist if p not in base_blacklist) + base_ask = () if allow_network else _DEFAULT_ASK_RULES + ask_rules = base_ask + tuple( + p for p in user_ask_rules if p not in base_ask) + blacklist = _DEFAULT_BLACKLIST + tuple( + p for p in user_blacklist if p not in _DEFAULT_BLACKLIST) safety_raw = d.get('safety_rules', {}) # Merge directory configs from top level into safety config diff --git a/ms_agent/permission/enforcer.py b/ms_agent/permission/enforcer.py index ca10b020b..c8d59880e 100644 --- a/ms_agent/permission/enforcer.py +++ b/ms_agent/permission/enforcer.py @@ -14,7 +14,7 @@ from .config import PermissionConfig from .handler import (AutoPermissionHandler, PermissionAction, PermissionHandler, PermissionResponse) -from .matcher import PermissionMatcher +from .matcher import CONTENT_SEP, PermissionMatcher from .memory import PermissionMemory from .suggestions import generate_suggestions @@ -84,6 +84,15 @@ async def _ask_user(self, return None return await self._handler.ask(**kwargs) + def _can_ask_human(self) -> bool: + """Whether a real person can actually answer a prompt right now. + + ``AutoPermissionHandler`` is the stand-in used headlessly and it always + answers "allow", so treating it as an asker would turn every ask rule + into a no-op. + """ + return not isinstance(self._handler, AutoPermissionHandler) + def _handler_accepts(self, param: str) -> bool: try: sig = inspect.signature(self._handler.ask) @@ -127,19 +136,41 @@ async def check( ) return self._process_response(response, tool_name, tool_args) + # 1b. Ask rules → confirm, in EVERY mode. Until now this config existed + # but only the hook path consulted it, so an ask rule was silently + # inert on the ordinary route. It outranks the mode and the whitelist — + # that is the whole point of "ask even under full access" — but not the + # user's own remembered answer below, so consenting once still sticks. + ask_rule = next( + (p for p in self._config.ask_rules + if self._matcher.match_with_content(p, tool_name, tool_args)), + None, + ) + if ask_rule and not self._can_ask_human(): + # Headless (AutoPermissionHandler allows everything): there is + # nobody to confirm, and silently running the thing an ask rule was + # written to gate would be worse than refusing. + return PermissionDecision( + action='deny', + reason=(f'Ask rule matched: {ask_rule}; no interactive ' + 'handler is attached to confirm it'), + ) + # 2. Auto / strict mode → allow (safety handled by SafetyGuard + ask_resolver) - if self._config.mode in ('auto', 'strict'): + if self._config.mode in ('auto', 'strict') and not ask_rule: return PermissionDecision( action='allow', reason=f'{self._config.mode.capitalize()} mode') # 3. Whitelist → allow - for pattern in self._config.whitelist: - if self._matcher.match_with_content(pattern, tool_name, tool_args): - return PermissionDecision( - action='allow', - reason=f'Allowed by whitelist rule: {pattern}', - ) + if not ask_rule: + for pattern in self._config.whitelist: + if self._matcher.match_with_content(pattern, tool_name, + tool_args): + return PermissionDecision( + action='allow', + reason=f'Allowed by whitelist rule: {pattern}', + ) # 4. Memory (session + persistent) → allow if self._memory.matches(tool_name, tool_args): @@ -160,6 +191,24 @@ async def check( return self._process_response(response, tool_name, tool_args) + def _remember_pattern(self, response: PermissionResponse, tool_name: str, + tool_args: dict[str, Any]) -> str: + """What to remember when the caller named no pattern of its own. + + The bare tool name means "allow this TOOL" — for the shell that is + every future command, so approving ``ls -la`` once silently handed over + unrestricted shell access. Prefer instead the most specific generated + suggestion that is no broader than the tool itself (``:ls *``); + a suggestion that WIDENS the scope (``---*``) is not a fallback + anyone asked for. + """ + if response.pattern: + return response.pattern + for s in generate_suggestions(tool_name, tool_args): + if s == tool_name or s.startswith(f'{tool_name}{CONTENT_SEP}'): + return s + return tool_name + def _process_response( self, response: PermissionResponse | None, @@ -179,7 +228,7 @@ def _process_response( action='allow', reason='User allowed once') if response.action == PermissionAction.ALLOW_SESSION: - pattern = response.pattern or tool_name + pattern = self._remember_pattern(response, tool_name, tool_args) self._memory.add_session(pattern) return PermissionDecision( action='allow', @@ -187,7 +236,7 @@ def _process_response( ) if response.action == PermissionAction.ALLOW_ALWAYS: - pattern = response.pattern or tool_name + pattern = self._remember_pattern(response, tool_name, tool_args) self._memory.add(pattern, scope='project', source='user') return PermissionDecision( action='allow', diff --git a/ms_agent/permission/matcher.py b/ms_agent/permission/matcher.py index a1422591c..10b1d57b5 100644 --- a/ms_agent/permission/matcher.py +++ b/ms_agent/permission/matcher.py @@ -37,6 +37,27 @@ def _extract_content(tool_name: str, tool_args: dict[str, Any]) -> str | None: return str(val) if val is not None else None +def _with_bare_command_variants(content_pattern: str) -> str: + """Add an argument-less variant for every `` *`` alternative. + + ``curl *`` means "the curl command with any arguments" — and running it with + NONE is a case of that. fnmatch disagrees: it wants the space and at least + one character after it, so bare ``curl`` slipped past a rule written to gate + exactly that, and a remembered ``whoami *`` failed to match the very + ``whoami`` it was generated from. + + Only the space-star idiom of shell commands is extended. Path patterns end + in ``/*`` (``~/.ssh/*``) or ``=*`` (``dd if=*``) and are left alone — there + the trailing component is meaningful, not an optional argument list. + """ + alts = [a.strip() for a in content_pattern.split('|')] + out = list(alts) + for alt in alts: + if alt.endswith(' *'): + out.append(alt[:-2].rstrip()) + return '|'.join(p for p in out if p) + + class PermissionMatcher: """Wildcard matcher for permission rules, shared by both SafetyGuard and PermissionEnforcer.""" @@ -77,4 +98,4 @@ def match_with_content( if content is None: return False - return self.match(content_pattern, content) + return self.match(_with_bare_command_variants(content_pattern), content) diff --git a/ms_agent/permission/path_validator.py b/ms_agent/permission/path_validator.py index 52b2791ad..74f465f8c 100644 --- a/ms_agent/permission/path_validator.py +++ b/ms_agent/permission/path_validator.py @@ -14,6 +14,10 @@ _WINDOWS_DRIVE_ROOT = re.compile(r'^[A-Za-z]:/?$') _WINDOWS_DRIVE_CHILD = re.compile(r'^[A-Za-z]:/[^/]+$') _ROOT_CHILD = re.compile(r'^/[^/]+$') +#: A dangerous-removal entry that is nothing but separators and stars (``*``, +#: ``/*``). Meaningful as a literal argument, useless as a glob: fnmatch's ``*`` +#: crosses ``/``, so such an entry matches every path in existence. +_WILDCARD_ONLY = re.compile(r'^[/\\*]+$') @dataclass(frozen=True) @@ -182,35 +186,44 @@ def is_dangerous_removal_path( ``extra_patterns`` are configurable (``safety_rules.dangerous_removal_paths``): each is expanded (``~``) and matched against the normalized path both - literally and as an fnmatch glob (REVIEW P1-8).""" + literally and as an fnmatch glob (REVIEW P1-8). + + A wildcard-ONLY pattern (``*``, ``/*``) is compared literally and never as a + glob. Those entries exist to catch the user typing ``rm *`` — the argument + itself — but ``fnmatch(anything, '*')`` is unconditionally true, so as globs + they made EVERY path dangerous and no ``rm`` could run at all, not even + ``rm build/out.txt``. The literal comparison (plus the fixed checks below) + still catches what they were written for.""" import fnmatch - normalized = _CONSECUTIVE_SLASHES.sub('/', path) - if normalized.endswith('/') and len(normalized) > 1: - normalized = normalized.rstrip('/') + + def _norm(raw: str) -> str: + out = _CONSECUTIVE_SLASHES.sub('/', raw) + return out.rstrip('/') if out.endswith('/') and len(out) > 1 else out + + # Judge the argument both AS WRITTEN (`~`, `*` — what the user typed, and + # what the pattern list is phrased in) and EXPANDED (`/Users/me` — what rm + # would actually delete). Checking only the written form let `rm ~` through. + candidates = {_norm(path), _norm(os.path.expanduser(path))} for pat in extra_patterns or (): - pat_expanded = _CONSECUTIVE_SLASHES.sub('/', - os.path.expanduser(str(pat))) - if (normalized == pat_expanded.rstrip('/') - or fnmatch.fnmatch(normalized, pat_expanded)): + raw = _norm(str(pat)) + expanded = _norm(os.path.expanduser(str(pat))) + if candidates & {raw, expanded}: + return True + if _WILDCARD_ONLY.match(expanded): + continue + if any(fnmatch.fnmatch(c, expanded) for c in candidates): return True - - if normalized == '*': - return True - if normalized.endswith('/*') or normalized.endswith('\\*'): - return True - if normalized == '/': - return True home = os.path.expanduser('~').replace('\\', '/') - if normalized == home: - return True - - if _ROOT_CHILD.match(normalized): - return True - if _WINDOWS_DRIVE_ROOT.match(normalized): - return True - if _WINDOWS_DRIVE_CHILD.match(normalized): - return True + for normalized in candidates: + if normalized == '*' or normalized.endswith(('/*', '\\*')): + return True + if normalized == '/' or normalized == home: + return True + if (_ROOT_CHILD.match(normalized) + or _WINDOWS_DRIVE_ROOT.match(normalized) + or _WINDOWS_DRIVE_CHILD.match(normalized)): + return True return False diff --git a/ms_agent/session/context_assembler.py b/ms_agent/session/context_assembler.py index 2f813777e..347f5dd1d 100644 --- a/ms_agent/session/context_assembler.py +++ b/ms_agent/session/context_assembler.py @@ -214,6 +214,11 @@ def _dicts_to_messages(dicts: List[Dict[str, Any]]) -> List[Message]: # replays them — required for its thinking-mode tool follow-ups. reasoning_content=d.get('reasoning_content', '') or '', reasoning_signature=d.get('reasoning_signature', '') or '', + # Image refs attached to a user turn. This runs on EVERY + # round (the live context is reassembled from the log), so + # omitting them here would make images visible on the turn + # they were sent and invisible from the next round on. + attachments=d.get('attachments') or [], )) else: result.append(Message(role='user', content=str(d))) diff --git a/ms_agent/session/strategies/summary_compactor.py b/ms_agent/session/strategies/summary_compactor.py index 622f46f79..ab9a61190 100644 --- a/ms_agent/session/strategies/summary_compactor.py +++ b/ms_agent/session/strategies/summary_compactor.py @@ -9,6 +9,8 @@ import json from typing import Any, Dict, List, Optional, Tuple +from ms_agent.llm import multimodal +from ms_agent.llm.message_text import flatten_message_text from ms_agent.utils.logger import get_logger logger = get_logger() @@ -41,13 +43,16 @@ def _estimate_tokens(text: str) -> int: def _estimate_message_tokens(msg: Dict[str, Any]) -> int: - """Heuristic token count from message body (no API usage fields).""" + """Heuristic token count from message body (no API usage fields). + + Image blocks are charged a flat per-image cost rather than having their + base64 measured as text — see the twin in ``tool_pruner`` for the numbers and + for why over-counting here does not self-correct. + """ total = 0 content = msg.get('content', '') if content: - if not isinstance(content, str): - content = json.dumps(content, ensure_ascii=False) - total += _estimate_tokens(content) + total += multimodal.estimate_content_tokens(content, _estimate_tokens) tc = msg.get('tool_calls') if tc: total += _estimate_tokens(json.dumps(tc)) @@ -158,8 +163,11 @@ def _generate_summary(self, messages: List[Dict[str, Any]], conv_parts: List[str] = [] for msg in messages: role = msg.get('role', '?').upper() - content = msg.get('content', '') - if isinstance(content, str) and content: + # Reduce a block list to its text instead of skipping the message: + # a turn dropped here is invisible to the summary that decides what + # survives compaction. + content = flatten_message_text(msg.get('content', '')) + if content: conv_parts.append(f'{role}: {content[:char_limit]}') conversation = '\n'.join(conv_parts) diff --git a/ms_agent/session/strategies/tool_pruner.py b/ms_agent/session/strategies/tool_pruner.py index e1b48ca92..b4abe1cc9 100644 --- a/ms_agent/session/strategies/tool_pruner.py +++ b/ms_agent/session/strategies/tool_pruner.py @@ -13,6 +13,7 @@ import json from typing import Any, Dict, List, Optional, Tuple +from ms_agent.llm import multimodal from ms_agent.utils.logger import get_logger logger = get_logger() @@ -25,13 +26,25 @@ def _estimate_tokens(text: str) -> int: def _estimate_message_tokens(msg: Dict[str, Any]) -> int: - """Heuristic token count from message body (no API usage fields).""" + """Heuristic token count from message body (no API usage fields). + + Structured content is walked block-by-block, and an image block is charged a + flat per-image cost instead of having its payload measured as text. The + providers bill images by pixel dimensions (Anthropic ``⌈w/28⌉ × ⌈h/28⌉``; + DashScope returns the real figure as ``prompt_tokens_details.image_tokens``), + which has nothing to do with how long the base64 happens to be. + + Measured on DashScope with a 900x320 PNG: 282 real image tokens versus 7,293 + from counting base64 characters — a 26x over-count on a 22 KB image. A 2 MiB + PNG estimates at ~699k tokens against a ~108k usable budget, which does not + merely mis-trigger compaction once: the offending message is the last visible + one, so the compactor re-appends it, the estimate never drops, and every + subsequent round pays for another summary LLM call. + """ total = 0 content = msg.get('content', '') if content: - if not isinstance(content, str): - content = json.dumps(content, ensure_ascii=False) - total += _estimate_tokens(content) + total += multimodal.estimate_content_tokens(content, _estimate_tokens) tool_calls = msg.get('tool_calls') if tool_calls: total += _estimate_tokens(json.dumps(tool_calls)) diff --git a/ms_agent/tools/filesystem_tool.py b/ms_agent/tools/filesystem_tool.py index 4848618e6..f83719888 100644 --- a/ms_agent/tools/filesystem_tool.py +++ b/ms_agent/tools/filesystem_tool.py @@ -194,7 +194,10 @@ async def _get_tools_inner(self): ('Read the content of one or more files.\n\n' '- `paths`: list of relative file paths to read (preferred).\n' '- `path`: single relative file path (alias when the model passes one file).\n' - '- For image files (png/jpg/jpeg/gif/webp), returns base64-encoded content.\n' + '- Image files (png/jpg/jpeg/gif/webp) are returned AS IMAGES, attached to\n' + ' the result — look at them directly. They are not readable as text, and\n' + ' `offset`/`limit` do not apply. If you cannot see an attached image, the\n' + ' current model has image understanding disabled.\n' '- `offset`: line number to start reading from (1-based). ' 'Only effective when paths has exactly one element. Omit to read from the beginning.\n' '- `limit`: number of lines to read. ' @@ -820,6 +823,10 @@ async def read_file(self, return await self._read_files_abbreviated(paths) results = {} + # Structured image references collected while walking the paths. Returned + # alongside the text so the transports can put the pixels in the image + # channel instead of stringifying them into the text one. + image_refs: list = [] use_line_range = len(paths) == 1 and (offset is not None or limit is not None) @@ -836,13 +843,36 @@ async def read_file(self, # --- Image files --- if ext in self.IMAGE_EXTENSIONS: - with open(target_path_real, 'rb') as f: - raw = f.read() + # Two channels, and the bytes belong in the other one. This + # dict is JSON-serialized into the tool message's TEXT + # content, so returning base64 here put the image where a + # model cannot decode it: tens of thousands of tokens of + # literal characters, zero comprehension, and an inflated + # context estimate that re-fires compaction every round + # (see session/strategies/tool_pruner). + # + # So: a short status in the text, and a structured reference + # collected below into ``attachments``, which the transports + # expand into a real image block. The model genuinely sees + # the file it asked to read. media_type = f'image/{ext}' if ext != 'jpg' else 'image/jpeg' + size = os.path.getsize(target_path_real) + image_refs.append({ + 'type': 'image', + 'path': path, + 'media_type': media_type, + 'label': f'Image: {os.path.basename(path)}', + }) results[path] = { 'type': 'image', 'media_type': media_type, - 'base64': base64.b64encode(raw).decode('ascii'), + 'bytes': size, + 'shown_as_image': True, + 'message': + (f'This {media_type} image ({size} bytes) is attached to ' + 'this result as an image, so look at it directly; it ' + 'cannot be read as text. If you cannot see it, the ' + 'current model has image understanding disabled.'), } continue @@ -909,7 +939,13 @@ async def read_file(self, results[path] = f'Read file <{path}> failed: FileNotFound' except Exception as e: results[path] = f'Read file <{path}> failed, error: ' + str(e) - return json.dumps(results, indent=2, ensure_ascii=False) + text = json.dumps(results, indent=2, ensure_ascii=False) + if image_refs: + # Dict form so the agent's ToolResult picks up ``attachments`` and + # carries the pixels to the image channel. A read with no images + # returns the same plain string as before, so nothing else moves. + return {'result': text, 'attachments': image_refs} + return text async def _read_files_abbreviated(self, paths: list[str]) -> str: results = {} diff --git a/ms_agent/tools/image_reader_tool.py b/ms_agent/tools/image_reader_tool.py new file mode 100644 index 000000000..e6cda7a6d --- /dev/null +++ b/ms_agent/tools/image_reader_tool.py @@ -0,0 +1,243 @@ +# Copyright (c) ModelScope Contributors. All rights reserved. +"""``image_reader``: let a text-only model ask a vision model about an image. + +The main conversation model may have no image understanding at all — measured +across seven providers, roughly a third of configured models are in that class, +and four of them accept an image block with HTTP 200 and simply cannot see it. +For those, an attached image degrades to a path and the answer is "I can't view +images", which is honest but useless. + +This tool closes that gap without changing the main model: it sends the image to +a SEPARATELY configured vision model and returns that model's description as +text. The main model then reasons over the text. Lossy by construction — a +description is not the pixels — so it is the fallback, never the preferred path: +when the main model can see images, the transports show it the real thing and +this tool should not be needed. + +Configuration (all under ``llm.vision.auxiliary``; absent ⇒ the tool is not +registered, so nothing changes for anyone who has not opted in):: + + llm: + vision: + auxiliary: + service: dashscope # provider id / SDK service name + model: qwen3.8-max # a model that CAN see images + api_key: ... # optional; falls back to the env/spec + base_url: ... # optional; same + protocol: openai # optional; 'anthropic' for that wire format + +Modelled on hermes-agent's ``vision_analyze``. +""" +from __future__ import annotations + +import json +import os +from typing import Any, Dict, List, Optional + +from ms_agent.llm.utils import Tool +from ms_agent.tools.base import ToolBase +from ms_agent.utils import get_logger + +logger = get_logger() + +#: Asked of the auxiliary model when the caller has no specific question. Aims at +#: a description another model can reason over rather than prose for a human: +#: verbatim text first, because that is what callers most often actually need. +DEFAULT_PROMPT = ( + 'Describe this image for another AI model that cannot see it. Start with ' + 'every piece of text in the image, transcribed verbatim. Then describe the ' + 'layout, the objects, their colours and any relationships that matter. Be ' + 'specific and factual; do not speculate about intent.') + + +def _fail(error: str) -> str: + """A failed ``image_reader`` result, in the same JSON shape as a success.""" + return json.dumps({'ok': False, 'error': error}, ensure_ascii=False) + + +def auxiliary_config(config: Any) -> Optional[Dict[str, Any]]: + """The ``llm.vision.auxiliary`` block as a plain dict, or None if unset. + + Returning None is the opt-in switch: no auxiliary model configured means the + tool is never registered, so a user who has not asked for this pays nothing — + not a tool definition in the prompt, not a stray dependency. + """ + llm = getattr(config, 'llm', None) + vision = getattr(llm, 'vision', None) if llm is not None else None + aux = getattr(vision, 'auxiliary', None) if vision is not None else None + if aux is None: + return None + model = getattr(aux, 'model', None) + if not model: + return None + out: Dict[str, Any] = {'model': str(model)} + for key in ('service', 'api_key', 'base_url', 'protocol'): + value = getattr(aux, key, None) + if value: + out[key] = str(value) + return out + + +class ImageReaderTool(ToolBase): + """One tool: ``image_reader(path, question=None)``.""" + + server_name = 'image_reader' + + def __init__(self, config, **kwargs): + super().__init__(config) + self.exclude_func(getattr(config.tools, 'image_reader', None)) + self._aux = auxiliary_config(config) or {} + self._llm = None # built lazily: no cost unless the tool is called + + async def connect(self) -> None: + if not self._aux: + logger.warning( + '[image_reader] no llm.vision.auxiliary.model configured; the ' + 'tool will report that it is unavailable') + + async def cleanup(self) -> None: + self._llm = None + + async def _get_tools_inner(self): + return { + 'image_reader': [ + Tool( + tool_name='image_reader', + server_name='image_reader', + description= + ('Look at an image file and get a text description of it, ' + 'produced by a vision model.\n\n' + 'Use this ONLY when you cannot see an image yourself. ' + 'Images the user attached to the conversation are shown ' + 'to you directly when this model supports it — asking ' + 'this tool about them instead would give you a lossy ' + 'second-hand description.\n\n' + 'Typical use: the user attached an image but you cannot ' + 'see it, or you need to inspect an image file in the ' + 'workspace that was never attached.'), + parameters={ + 'type': 'object', + 'properties': { + 'path': { + 'type': + 'string', + 'description': + ('Workspace-relative path of the image ' + '(png/jpg/jpeg/gif/webp).'), + }, + 'question': { + 'type': + 'string', + 'description': + ('What you need to know about the image. Omit ' + 'for a full general description.'), + }, + }, + 'required': ['path'], + 'additionalProperties': False, + }) + ] + } + + def _build_llm(self): + """Construct the auxiliary vision LLM (once).""" + if self._llm is not None: + return self._llm + from omegaconf import OmegaConf + + from ms_agent.llm import LLM + + aux = self._aux + service = aux.get('service') or 'openai' + llm_cfg: Dict[str, Any] = { + 'service': service, + 'model': aux['model'], + # The auxiliary model is chosen BECAUSE it can see images, so state + # that outright rather than letting the resolver guess. + 'supports_vision': True, + 'use_provider_router': True, + } + if aux.get('protocol'): + llm_cfg['protocol'] = aux['protocol'] + if aux.get('api_key'): + llm_cfg[f'{service}_api_key'] = aux['api_key'] + if aux.get('base_url'): + llm_cfg[f'{service}_base_url'] = aux['base_url'] + cfg = OmegaConf.create({ + 'llm': llm_cfg, + 'generation_config': { + 'stream': False + }, + # So the attachment's relative path resolves against the same + # workspace the caller is talking about. + 'output_dir': self.output_dir, + }) + self._llm = LLM.from_config(cfg) + return self._llm + + async def call_tool(self, server_name: str, *, tool_name: str, + tool_args: dict) -> str: + # Same dispatch shape as FileSystemTool: the tool name IS the method. + return await getattr(self, tool_name)(**(tool_args or {})) + + async def image_reader(self, + path: str = '', + question: Optional[str] = None) -> str: + """Describe the image at ``path`` using the auxiliary vision model.""" + if not self._aux: + return _fail( + 'No auxiliary vision model is configured ' + '(llm.vision.auxiliary.model), so this image cannot be ' + 'described. Tell the user to configure one, enable image ' + 'understanding for the current model, or switch to a model ' + 'that supports images.') + if not path: + return _fail('path is required') + + from ms_agent.llm import multimodal + from ms_agent.llm.utils import Message, collect_response + + opts = multimodal.VisionOptions.from_config( + self.config, workspace_root=self.output_dir) + refs = multimodal.image_refs([{'type': 'image', 'path': path}], opts) + if not refs: + return _fail(f'{path!r} is not a readable image type (expected ' + 'png/jpg/jpeg/gif/webp).') + # Resolve to bytes here rather than trusting the path to exist later, so + # a missing file is one clear error instead of a provider-side failure. + if multimodal.load_image(refs[0], opts) is None: + return _fail(f'cannot read or decode the image at {path!r}') + + prompt = (question or '').strip() or DEFAULT_PROMPT + attachment = { + 'type': 'image', + 'path': path, + 'media_type': refs[0].media_type, + 'label': f'Image: {os.path.basename(path)}', + } + try: + llm = self._build_llm() + response = collect_response( + llm.generate([ + Message( + role='user', content=prompt, attachments=[attachment]) + ])) + description = (getattr(response, 'content', '') or '').strip() + except Exception as exc: + logger.warning('[image_reader] %s failed: %s', + self._aux.get('model'), exc) + return _fail(f'{type(exc).__name__}: {exc}') + + if not description: + return _fail('the vision model returned nothing') + return json.dumps( + { + 'ok': True, + 'path': path, + 'model': self._aux.get('model'), + # Named so the reader cannot mistake it for having seen the + # image: it is one model's account of another's pixels. + 'description_from_vision_model': description, + }, + ensure_ascii=False, + indent=2) diff --git a/ms_agent/tools/tool_manager.py b/ms_agent/tools/tool_manager.py index dac7dcf6e..39a0d942a 100644 --- a/ms_agent/tools/tool_manager.py +++ b/ms_agent/tools/tool_manager.py @@ -20,6 +20,7 @@ from ms_agent.tools.code import CodeExecutionTool, LocalCodeExecutionTool from ms_agent.tools.filesystem_tool import FileSystemTool from ms_agent.tools.image_generator import ImageGenerator +from ms_agent.tools.image_reader_tool import ImageReaderTool try: from ms_agent.tools.mcp_client import MCPClient @@ -144,6 +145,18 @@ def __init__( if hasattr(config, 'tools') and hasattr(config.tools, 'video_generator'): self.extra_tools.append(VideoGenerator(config)) + # image_reader is registered ONLY when an auxiliary vision model is + # configured: without one the tool can do nothing, and an always-present + # tool the model may call and always fail is worse than no tool at all. + if _tool_on(config, 'image_reader'): + from ms_agent.tools.image_reader_tool import auxiliary_config + + if auxiliary_config(config): + self.extra_tools.append(ImageReaderTool(config)) + else: + logger.info( + 'tools.image_reader is enabled but ' + 'llm.vision.auxiliary.model is unset; not registering it') if _tool_on(config, 'file_system'): self.extra_tools.append( FileSystemTool( diff --git a/ms_agent/tui/managed_config.py b/ms_agent/tui/managed_config.py index 35de5c6fb..112759801 100644 --- a/ms_agent/tui/managed_config.py +++ b/ms_agent/tui/managed_config.py @@ -82,7 +82,18 @@ def resolve_mcp_config( for k, v in entry.items() if k not in _MCP_META } except Exception: - pass + # Everything above is one try, so a single malformed mcp.json silently + # became "this agent has no MCP servers at all" — indistinguishable + # from "none configured", and the hardest possible shape to diagnose + # from the outside. Still non-fatal (a broken file must not stop the + # agent), but no longer invisible. + from ms_agent.utils import get_logger + get_logger().warning( + 'could not read the managed MCP configuration (global=%s, ' + 'project=%s); continuing with no MCP servers from it', + global_home, + work_dir, + exc_info=True) # Explicit --mcp-server-file wins last (same-name replace). if explicit_file and os.path.isfile(explicit_file): try: diff --git a/ms_agent/ui/events.py b/ms_agent/ui/events.py index 5aeff78e5..79845143c 100644 --- a/ms_agent/ui/events.py +++ b/ms_agent/ui/events.py @@ -135,6 +135,28 @@ class ReasoningEnded(AgentEvent): # ── tools ───────────────────────────────────────────────────────────────── +@dataclass(frozen=True) +class ToolCallComposing(AgentEvent): + """The model is still WRITING a tool call; nothing runs yet. + + Between the last ``content_delta`` and the first ``tool_call_started`` the + model streams the call's arguments, and until this event existed that window + produced no events at all. It is imperceptible for a small call and very + visible for a large one: measured at ~67 s of blank UI for one round that + wrote five long files, because every file's whole body travels inside the + arguments. + + The tool NAME arrives before its arguments do, so this can say what is being + prepared. ``arguments_len`` is the bytes accumulated so far — enough to show + progress, and deliberately not the payload itself, which is often huge and + is delivered in full by ``tool_call_started`` anyway. + """ + EVENT_TYPE: ClassVar[str] = 'tool_call_composing' + index: int = 0 + name: str = '' + arguments_len: int = 0 + + @dataclass(frozen=True) class ToolCallStarted(AgentEvent): """A tool call is about to execute.""" diff --git a/ms_agent/ui/input.py b/ms_agent/ui/input.py index a44b0c653..a9a0ae9ea 100644 --- a/ms_agent/ui/input.py +++ b/ms_agent/ui/input.py @@ -34,6 +34,16 @@ class InputSource(Protocol): async def read_prompt(self, prompt: str = '>>> ') -> str: ... + # Optional: sources that can carry non-text parts (a WebUI composer with + # image attachments) also implement + # + # def take_attachments(self) -> list[dict]: ... + # + # returning the parts belonging to the prompt just read and clearing them. + # It is intentionally NOT part of this Protocol's required surface so every + # existing text-only source stays conformant; ``InteractiveSession`` + # feature-detects it. + class StdinInputSource: """Default input source: blocking ``input()`` off the event loop. diff --git a/tests/llm/test_message_text.py b/tests/llm/test_message_text.py new file mode 100644 index 000000000..ec4d1e357 --- /dev/null +++ b/tests/llm/test_message_text.py @@ -0,0 +1,102 @@ +# Copyright (c) ModelScope Contributors. All rights reserved. +"""flatten_message_text and the shape-preserving mutators. + +Exists because a block list reaching a str-assuming call site never crashes — it +stores a Python repr or a guard skips the message. Both are silent, and two of +those sites (WebUI session auto-naming, TUI session naming) PERSIST what they +compute, so the garbage becomes permanent and user-visible. +""" +import unittest + +from ms_agent.llm.message_text import (append_text, flatten_message_text, + prepend_text) + +BLOCKS = [ + {'type': 'text', 'text': 'Image 1: a.png'}, + {'type': 'image_url', 'image_url': {'url': 'data:image/png;base64,AAAA'}}, + {'type': 'text', 'text': 'what is in it'}, +] + + +class TestFlatten(unittest.TestCase): + + def test_string_passes_through_unchanged(self): + # The common case must be byte-identical, so no existing behaviour moves. + for value in ('hello', '', ' spaced ', 'multi\nline'): + self.assertEqual(flatten_message_text(value), value) + + def test_none_is_empty_not_the_word_none(self): + self.assertEqual(flatten_message_text(None), '') + + def test_text_blocks_joined_images_dropped(self): + out = flatten_message_text(BLOCKS) + self.assertEqual(out, 'Image 1: a.png\nwhat is in it') + self.assertNotIn('base64', out) + self.assertNotIn('AAAA', out) + + def test_every_non_text_modality_contributes_nothing(self): + for kind in ('image', 'image_url', 'input_image', 'audio', + 'input_audio', 'video', 'input_video', 'file', 'document'): + self.assertEqual( + flatten_message_text([{'type': kind, 'data': 'x' * 100}]), '', + f'{kind} must not leak its payload') + + def test_anthropic_shaped_image_block(self): + anthropic = [ + {'type': 'text', 'text': 'look'}, + {'type': 'image', 'source': {'type': 'base64', 'data': 'ZZZZ'}}, + ] + self.assertEqual(flatten_message_text(anthropic), 'look') + + def test_bare_strings_inside_a_list(self): + self.assertEqual(flatten_message_text(['a', 'b']), 'a\nb') + + def test_unknown_block_falls_back_to_its_text_field(self): + self.assertEqual( + flatten_message_text([{'type': 'weird', 'text': 'still text'}]), + 'still text') + + def test_custom_separator(self): + self.assertEqual(flatten_message_text(BLOCKS, sep=' | '), + 'Image 1: a.png | what is in it') + + def test_never_raises_on_junk(self): + for junk in (123, 4.5, True, object(), {'no': 'type'}): + self.assertIsInstance(flatten_message_text(junk), str) + + +class TestShapePreservingMutators(unittest.TestCase): + """The framework augments a user turn in place (memory recall, update + notices). Concatenating a string onto a list raises; replacing the list with + a string silently drops the images.""" + + def test_append_to_string(self): + self.assertEqual(append_text('base', 'extra'), 'base\n\nextra') + self.assertEqual(append_text('', 'extra'), 'extra') + + def test_append_to_blocks_adds_a_trailing_text_block(self): + out = append_text(BLOCKS, 'recalled memory') + self.assertIsInstance(out, list) + self.assertEqual(len(out), len(BLOCKS) + 1) + self.assertEqual(out[-1], {'type': 'text', 'text': 'recalled memory'}) + # The image block survives untouched — the whole point. + self.assertEqual(out[1], BLOCKS[1]) + + def test_prepend_to_blocks_puts_text_first(self): + out = prepend_text(BLOCKS, 'NOTICE') + self.assertEqual(out[0], {'type': 'text', 'text': 'NOTICE'}) + self.assertEqual(out[1:], BLOCKS) + + def test_empty_extra_is_a_no_op_on_both(self): + self.assertEqual(append_text(BLOCKS, ''), BLOCKS) + self.assertEqual(prepend_text('x', ''), 'x') + + def test_originals_are_not_mutated(self): + original = list(BLOCKS) + append_text(BLOCKS, 'a') + prepend_text(BLOCKS, 'b') + self.assertEqual(BLOCKS, original) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/llm/test_thinking_effort.py b/tests/llm/test_thinking_effort.py new file mode 100644 index 000000000..db105bf1d --- /dev/null +++ b/tests/llm/test_thinking_effort.py @@ -0,0 +1,638 @@ +# Copyright (c) ModelScope Contributors. All rights reserved. +"""One knob (`reasoning_effort`) lowered onto each endpoint's own spelling. + +The tiers and wire shapes asserted here come from the vendors' own docs, checked +2026-08-17; the module docstring of ``ms_agent/llm/thinking.py`` has the table. +""" +import pytest + +from ms_agent.llm import thinking as T + + +# --------------------------------------------------------------------------- # +# Which dialect an endpoint speaks +# --------------------------------------------------------------------------- # +@pytest.mark.parametrize( + 'base_url,expected', + [ + ('https://dashscope.aliyuncs.com/compatible-mode/v1', 'dashscope'), + # ATokenPlan and friends are Aliyun MaaS endpoints speaking DashScope. + ('https://token-plan.cn-beijing.maas.aliyuncs.com/compatible-mode/v1', + 'dashscope'), + ('https://api-inference.modelscope.cn/v1', 'modelscope'), + ('https://api.deepseek.com', 'deepseek'), + ('https://open.bigmodel.cn/api/paas/v4', 'zhipu'), + ('https://api.moonshot.cn/v1', 'moonshot'), + ('https://api.minimaxi.com/v1', 'minimax'), + ('https://openrouter.ai/api/v1', 'openrouter'), + ('https://api.openai.com/v1', 'openai'), + ('https://vllm.internal.corp:8000/v1', 'unknown'), + ], +) +def test_family_comes_from_the_host(base_url, expected): + assert T.endpoint_family(base_url) == expected + + +def test_anthropic_protocol_beats_the_host(): + """DeepSeek serves an Anthropic-compatible gateway on its own domain. The + body shape follows the protocol, not the vendor.""" + assert T.endpoint_family('https://api.deepseek.com/anthropic', + 'anthropic') == 'anthropic' + + +# --------------------------------------------------------------------------- # +# Canonical input +# --------------------------------------------------------------------------- # +@pytest.mark.parametrize('raw,expected', [ + (None, 'auto'), + ('', 'auto'), + ('auto', 'auto'), + (' HIGH ', 'high'), + ('x-high', 'xhigh'), + ('none', 'off'), + ('disabled', 'off'), + (True, 'high'), + (False, 'off'), + ('turbo', None), +]) +def test_effort_is_normalized_leniently(raw, expected): + assert T.normalize_effort(raw) == expected + + +def test_clamp_prefers_the_next_weaker_tier(): + """An effort tier is a quality FLOOR, not a ceiling, so a clamp must never + land above the request — see zlxlabs/llm-compat#11, where clamping UP into a + support set's interior gap jumped three tiers and billed at the top one.""" + assert T.clamp_effort('medium', ('low', 'high', 'max')) == 'low' + assert T.clamp_effort('max', ('off', 'low', 'medium', 'high')) == 'high' + assert T.clamp_effort('low', ('low', 'high', 'max')) == 'low' + + +def test_a_thinking_tier_never_collapses_into_off(): + """`off` is a different request, not the bottom rung. Clamping downward + must not turn "think a little" into "do not think".""" + assert T.clamp_effort('minimal', ('off', 'high')) == 'high' + assert T.clamp_effort('low', ('off', 'high')) == 'high' + # ...and asking to switch off where that is impossible lands on the weakest + # thinking tier, which is also the remedy Zhipu prescribes for GLM-5.3. + assert T.clamp_effort('off', ('low', 'high', 'max')) == 'low' + + +def test_kimi_can_be_switched_off_after_all(): + """The docs say Kimi K3 always thinks; the endpoint disagrees. Probed + 2026-08-17, `reasoning_effort: none` yields ZERO characters of reasoning on + both k3 and k2.6, so "off" goes out as a real value rather than being + clamped up to the floor tier.""" + got = T.plan('off', base_url='https://api.moonshot.cn/v1') + assert got['effective'] == 'off' + assert got['params'] == {'reasoning_effort': 'none'} + + +# --------------------------------------------------------------------------- # +# Lowering +# --------------------------------------------------------------------------- # +def test_boolean_endpoints_get_a_boolean(): + got = T.plan('max', base_url='https://api-inference.modelscope.cn/v1') + assert got['params'] == {'extra_body': {'enable_thinking': True}} + assert got['effective'] == 'high' # the ladder collapses to on/off here + + +def test_dashscope_gets_both_knobs_because_they_do_different_jobs(): + """They cover disjoint sets of models on this host. `reasoning_effort: high` + ALONE leaves qwen-plus at zero reasoning — it does not support the effort + field, so only `enable_thinking` reaches it (probed 2026-08-17: 0 characters + vs 543 with the flag) — while qwen3.8-max is the one Qwen model that does + take an effort tier. Sending one without the other loses half the models.""" + got = T.plan('low', base_url='https://dashscope.aliyuncs.com/v1') + assert got['params'] == { + 'extra_body': { + 'enable_thinking': True + }, + 'reasoning_effort': 'low', + } + + +def test_dashscope_drops_only_the_value_it_measurably_rejects(): + """`max` is the one 400 on this host (qwen3.7-plus rejects it, qwen3.8-max + accepts all seven — each value probed individually 2026-08-18), and it is + documented as an alias of `xhigh`, which is where the downward clamp lands. + Every other rung reaches the endpoint untouched.""" + got = T.plan('max', base_url='https://dashscope.aliyuncs.com/v1') + assert got['effective'] == 'xhigh' + assert got['params']['reasoning_effort'] == 'xhigh' + for rung in ('minimal', 'low', 'medium', 'high', 'xhigh'): + sent = T.plan(rung, base_url='https://dashscope.aliyuncs.com/v1') + assert sent['effective'] == rung + assert sent['params']['reasoning_effort'] == rung + + +def test_dashscope_off_stays_a_plain_boolean(): + """The models that refuse every effort value (qwen-vl-*) still accept + `enable_thinking: false`, so "off" must not go out as an effort tier.""" + got = T.plan('off', base_url='https://dashscope.aliyuncs.com/v1') + assert got['params'] == {'extra_body': {'enable_thinking': False}} + + +def test_a_hand_written_thinking_budget_suppresses_our_effort(): + """DashScope 400s on the pair ("'reasoning_effort' and 'thinking_budget' + cannot be set simultaneously") — and thinking_budget is exactly what the + settings hint invites people to add, so the two suggestions would collide. + The hand-written key wins; only our effort is dropped, not the switch.""" + out = T.apply_effort( + { + 'reasoning_effort': 'high', + 'extra_body': { + 'thinking_budget': 2048 + } + }, + base_url='https://dashscope.aliyuncs.com/v1') + assert out == { + 'extra_body': { + 'thinking_budget': 2048, + 'enable_thinking': True + } + } + # The preview the settings page renders has to agree with that. + shown = T.plan('high', + base_url='https://dashscope.aliyuncs.com/v1', + existing={'extra_body': { + 'thinking_budget': 2048 + }}) + assert 'reasoning_effort' not in shown['params'] + + +def test_ladder_endpoints_get_a_tier(): + got = T.plan('max', base_url='https://open.bigmodel.cn/api/paas/v4') + assert got['params'] == {'reasoning_effort': 'max'} + + +def test_deepseek_off_uses_the_thinking_object_not_an_effort(): + """DeepSeek's reasoning_effort has no "none" rung — low/high/max only — so + the only way to switch thinking off is the `thinking` object.""" + got = T.plan('off', base_url='https://api.deepseek.com') + assert got['params'] == {'extra_body': {'thinking': {'type': 'disabled'}}} + + +def test_openai_off_is_the_none_tier(): + got = T.plan('off', base_url='https://api.openai.com/v1') + assert got['params'] == {'reasoning_effort': 'none'} + + +def test_minimax_on_means_adaptive(): + """`adaptive`, not `enabled` — MiniMax's enum is exactly + ``["disabled", "adaptive"]`` and `enabled` appears nowhere in its docs.""" + got = T.plan('high', base_url='https://api.minimaxi.com/v1') + assert got['params']['extra_body']['thinking'] == {'type': 'adaptive'} + + +def test_minimax_always_asks_for_the_reasoning_to_be_split_out(): + """Its native format inlines reasoning into the answer as `…` + and does not read a separate `reasoning_content` back — so replaying one, as + we do, is a no-op there (probed 2026-08-18: identical to discarding it). + `reasoning_split` is the format its docs recommend and the only one where + our replay shape means anything. It says nothing about depth, so it rides + along with `auto` and with every tier — including `off`, where there is + simply no reasoning to split.""" + for effort in ('auto', 'off', 'low', 'max'): + got = T.plan(effort, base_url='https://api.minimaxi.com/v1') + assert got['params']['extra_body']['reasoning_split'] is True + + # Nobody else gets it: third-party hosts of the same weights reject the + # parameter outright, and they are a different family here. + for base in ('https://api-inference.modelscope.cn/v1', + 'https://openrouter.ai/api/v1'): + assert 'reasoning_split' not in str(T.plan('auto', base_url=base)) + + +def test_openrouter_uses_its_own_unified_object(): + assert T.plan('low', base_url='https://openrouter.ai/api/v1')['params'] \ + == {'extra_body': {'reasoning': {'effort': 'low'}}} + assert T.plan('off', base_url='https://openrouter.ai/api/v1')['params'] \ + == {'extra_body': {'reasoning': {'enabled': False}}} + + +# --------------------------------------------------------------------------- # +# `auto` — the default, and the whole point +# --------------------------------------------------------------------------- # +def test_auto_sends_nothing_almost_everywhere(): + """Vendor defaults are per-model and they move (DashScope alone has qwen3.5+ + defaulting ON and qwen-plus defaulting OFF). Sending nothing inherits + whatever they tuned, which is the only thing that stays correct for free.""" + for base_url in ('https://api-inference.modelscope.cn/v1', + 'https://api.deepseek.com', + 'https://open.bigmodel.cn/api/paas/v4', + 'https://api.openai.com/v1', + 'https://vllm.internal.corp:8000/v1'): + assert T.plan('auto', base_url=base_url)['params'] == {} + + +def test_auto_is_explicit_only_where_silence_would_mean_off(): + # Anthropic: our Messages transport always writes a `thinking` block and + # absent means disabled, so Claude would never think. + assert T.plan(None, base_url='', protocol='anthropic')['params'] == { + 'extra_body': { + 'enable_thinking': True + } + } + # DashScope: qwen-plus/turbo/flash and qwen3-max default thinking OFF. + assert T.plan(None, base_url='https://dashscope.aliyuncs.com/v1')[ + 'params'] == { + 'extra_body': { + 'enable_thinking': True + } + } + + +# --------------------------------------------------------------------------- # +# apply_effort: what actually reaches the client +# --------------------------------------------------------------------------- # +def test_the_canonical_key_never_reaches_the_wire(): + """`auto` and `off` are ours, not any vendor's. Leaving the key in place + would send `reasoning_effort: "auto"` to an endpoint that validates it.""" + out = T.apply_effort({'reasoning_effort': 'auto', 'temperature': 0.3}, + base_url='https://api-inference.modelscope.cn/v1') + assert out == {'temperature': 0.3} + + +def test_a_hand_written_wire_value_wins_over_the_knob(): + """extra_body is the escape hatch; someone who reached for it meant it.""" + out = T.apply_effort( + { + 'reasoning_effort': 'high', + 'extra_body': { + 'enable_thinking': False + } + }, + base_url='https://dashscope.aliyuncs.com/v1') + assert out == {'extra_body': {'enable_thinking': False}} + + +def test_unrelated_extra_body_keys_survive_lowering(): + out = T.apply_effort( + { + 'reasoning_effort': 'high', + 'extra_body': { + 'thinking_budget': 2048 + } + }, + base_url='https://dashscope.aliyuncs.com/v1') + assert out == { + 'extra_body': { + 'thinking_budget': 2048, + 'enable_thinking': True + } + } + + +def test_requests_without_the_knob_are_untouched_where_auto_is_silent(): + kwargs = {'temperature': 0.3, 'extra_body': {'enable_thinking': True}} + assert T.apply_effort(kwargs, base_url='https://x/v1') is kwargs + + +def test_an_absent_knob_means_auto_not_nothing(): + """Unset has to behave exactly like an explicit `auto`, or the two endpoints + where auto speaks up would depend on whether a caller bothered to write the + key.""" + out = T.apply_effort({'temperature': 0.3}, + base_url='https://dashscope.aliyuncs.com/v1') + assert out == { + 'temperature': 0.3, + 'extra_body': { + 'enable_thinking': True + } + } + # ...and it still yields to a hand-written wire value. + kwargs = {'extra_body': {'enable_thinking': False}} + assert T.apply_effort( + kwargs, base_url='https://dashscope.aliyuncs.com/v1') == kwargs + + +def test_a_reasoning_effort_refusal_is_recognized(): + """The fallback used to look only for qwen-style names, so a 400 on the + modern field would not have been healed.""" + assert T.is_thinking_refusal( + RuntimeError('Error code: 400 - unknown parameter reasoning_effort')) + + +# --------------------------------------------------------------------------- # +# Through the real transports +# --------------------------------------------------------------------------- # +class _Recorder: + + def __init__(self): + self.calls = [] + + def create(self, **kwargs): + self.calls.append(kwargs) + return 'completion' + + +def _fake_client(recorder, base_url): + ns = type('NS', (), {}) + client = ns() + client.chat = ns() + client.chat.completions = recorder + client.base_url = base_url + return client + + +def test_transport_lowers_the_knob_onto_the_endpoint(): + from ms_agent.llm.transport import openai_compat as TC + + rec = _Recorder() + tr = TC.OpenAICompatTransport.__new__(TC.OpenAICompatTransport) + tr.client = _fake_client(rec, 'https://api.deepseek.com') + tr.model = 'deepseek-v4-pro' + tr.args = {} + tr._format_input_message = lambda m: m + + tr._call_llm([], None, reasoning_effort='medium') + # DeepSeek reports the full vocabulary when handed a bogus value, `medium` + # included, so the tier reaches it untouched. + assert rec.calls[0]['reasoning_effort'] == 'medium' + assert 'extra_body' not in rec.calls[0] + + +def test_the_knob_is_not_an_anthropic_parameter(): + """Why the Anthropic transport has to lower BEFORE its signature filter: + `reasoning_effort` is not a Messages API argument, so filtering first would + silently drop the knob instead of turning it into a `thinking` block.""" + import inspect + + anthropic = pytest.importorskip('anthropic') + params = inspect.signature( + anthropic.Anthropic(api_key='x').messages.create).parameters + assert 'reasoning_effort' not in params + assert 'extra_body' in params # ...but the shape we lower into survives + + +def test_a_refused_tier_falls_all_the_way_back(): + """qwen-vl-max rejects every effort value (DashScope converts the tier into + a thinking_budget, which that model has no room for). The fallback has to + strip the tier as well as the switch, or the retry repeats the 400.""" + from ms_agent.llm.transport import openai_compat as TC + + class _Refuser(_Recorder): + + def create(self, **kwargs): + self.calls.append(kwargs) + asked = (kwargs.get('reasoning_effort') + or (kwargs.get('extra_body') or {}).get('enable_thinking')) + if asked: + raise RuntimeError( + 'Error code: 400 - The thinking_budget parameter must be a ' + 'positive integer and not greater than 0') + return 'completion' + + rec = _Refuser() + tr = TC.OpenAICompatTransport.__new__(TC.OpenAICompatTransport) + tr.client = _fake_client(rec, 'https://dashscope.aliyuncs.com/v1') + tr.model = 'qwen-vl-max' + tr.args = {} + tr._format_input_message = lambda m: m + + T.MODELS_REFUSING_THINKING.clear() + try: + out = tr._call_llm([], None, reasoning_effort='high') + finally: + T.MODELS_REFUSING_THINKING.clear() + + assert out == 'completion' + assert rec.calls[0]['reasoning_effort'] == 'high' + assert 'reasoning_effort' not in rec.calls[1] + assert rec.calls[1]['extra_body'] == {'enable_thinking': False} + + +# --------------------------------------------------------------------------- # +# Bugs the unit tests missed and a live matrix caught +# --------------------------------------------------------------------------- # +def test_lowering_twice_would_destroy_the_tier(): + """Documents WHY each transport lowers exactly once. + + The canonical key and DashScope's wire key are both `reasoning_effort`, so + the operation is not idempotent: a second pass reads the `enable_thinking` + the first pass added as "the caller is driving thinking by hand" and stands + down — deleting the tier we ourselves just set. Two call sites were doing + this, which silently reduced every DashScope request back to a bare switch. + """ + base = 'https://dashscope.aliyuncs.com/compatible-mode/v1' + once = T.apply_effort({'reasoning_effort': 'low'}, base_url=base) + assert once['reasoning_effort'] == 'low' + assert 'reasoning_effort' not in T.apply_effort(once, base_url=base) + + +def test_generate_sends_the_tier_all_the_way_to_the_client(): + """The end-to-end shape, through `generate()` and its signature filter — + the level the double-lowering bug lived at and a `_call_llm` test could not + see.""" + from ms_agent.llm.transport import openai_compat as TC + + class _SigRecorder(_Recorder): + """`generate()` filters kwargs against create()'s SIGNATURE, so a stub + taking bare **kwargs would drop every argument — including `stream` — + and quietly test nothing.""" + + def create(self, + *, + model=None, + messages=None, + tools=None, + stream=None, + max_tokens=None, + extra_body=None, + reasoning_effort=None, + **kw): + self.calls.append({ + 'extra_body': extra_body, + 'reasoning_effort': reasoning_effort, + 'stream': stream, + }) + return 'completion' + + rec = _SigRecorder() + tr = TC.OpenAICompatTransport.__new__(TC.OpenAICompatTransport) + tr.client = _fake_client( + rec, 'https://dashscope.aliyuncs.com/compatible-mode/v1') + tr.model = 'qwen3.8-max' + tr.args = {'reasoning_effort': 'max'} + tr.max_continue_runs = 1 + tr._strip_reasoning_tags = False + tr._format_input_message = lambda m: m + tr.format_tools = lambda t: None + + # Only what reached the client matters here; the stub cannot satisfy the + # response-shaping that follows. + try: + tr.generate([]) + except Exception: + pass + assert rec.calls[0]['reasoning_effort'] == 'xhigh' + assert rec.calls[0]['extra_body'] == {'enable_thinking': True} + + +def test_mandatory_thinking_is_repaired_forwards_not_backwards(): + """OpenRouter answers "Reasoning is mandatory for this endpoint and cannot + be disabled" for x-ai/grok-4.5. That names a thinking parameter, so the + refusal path used to claim it and "repair" it by forcing thinking OFF — + the exact opposite — and then remembered the model, degrading every later + request in the session.""" + from ms_agent.llm.transport import openai_compat as TC + + class _Mandatory(_Recorder): + + def create(self, **kwargs): + self.calls.append(kwargs) + reasoning = (kwargs.get('extra_body') or {}).get('reasoning') or {} + if reasoning.get('enabled') is False: + raise RuntimeError( + 'Error code: 400 - Reasoning is mandatory for this ' + 'endpoint and cannot be disabled.') + return 'completion' + + rec = _Mandatory() + tr = TC.OpenAICompatTransport.__new__(TC.OpenAICompatTransport) + tr.client = _fake_client(rec, 'https://openrouter.ai/api/v1') + tr.model = 'x-ai/grok-4.5' + tr.args = {} + tr._format_input_message = lambda m: m + + T.MODELS_REFUSING_THINKING.clear() + T.MODELS_REQUIRING_THINKING.clear() + try: + assert tr._call_llm([], None, reasoning_effort='off') == 'completion' + # Repaired by saying nothing, not by forcing the switch the other way. + assert 'extra_body' not in rec.calls[1] + assert 'reasoning_effort' not in rec.calls[1] + # ...and the model is not blacklisted, so a later tier still works. + assert T.model_key(tr.client, tr.model) not in T.MODELS_REFUSING_THINKING + tr._call_llm([], None, reasoning_effort='high') + assert rec.calls[2]['extra_body'] == {'reasoning': {'effort': 'high'}} + finally: + T.MODELS_REFUSING_THINKING.clear() + T.MODELS_REQUIRING_THINKING.clear() + + +def test_openrouter_style_reasoning_field_is_read(): + """OpenRouter normalizes every upstream's reasoning into `reasoning`, not + `reasoning_content`. Reading only the latter made every model proxied + through it look like it never thought.""" + from ms_agent.llm.transport.openai_compat import _reasoning_of + + ns = type('NS', (), {}) + delta = ns() + delta.reasoning = 'thought about it' + assert _reasoning_of(delta) == 'thought about it' + + both = ns() + both.reasoning_content = 'native' + both.reasoning = 'proxied' + assert _reasoning_of(both) == 'native' # the native field wins + assert _reasoning_of(ns()) == '' + + +def test_a_switch_only_endpoint_is_not_offered_a_ladder(): + """What we ACCEPT and what we OFFER are different questions. ModelScope's + gateway and MiniMax have a boolean, so listing eight rungs in the settings + dialog would promise control the model does not have.""" + assert T.offered_tiers('modelscope') == ('auto', 'off', 'on') + assert T.offered_tiers('minimax') == ('auto', 'off', 'on') + assert T.offered_tiers('anthropic') == ('auto', 'off', 'on') + # `on` is an alias of the single thinking tier, so it round-trips. + assert T.normalize_effort('on') == 'high' + assert T.plan('on', base_url='https://api-inference.modelscope.cn/v1')[ + 'params'] == {'extra_body': {'enable_thinking': True}} + + +def test_a_real_ladder_is_offered_in_full(): + assert T.offered_tiers('zhipu') == ('auto', 'off', 'minimal', 'low', + 'medium', 'high', 'xhigh', 'max') + # ...minus the rung DashScope rejects. + assert 'max' not in T.offered_tiers('dashscope') + assert 'xhigh' in T.offered_tiers('dashscope') + + +# --------------------------------------------------------------------------- # +# Rejections that arrive on the first chunk, not out of create() +# --------------------------------------------------------------------------- # +class _Boom400(Exception): + """An Aliyun-family gateway answering 200 and then rejecting the params.""" + + MSG = ('<400> InternalError.Algo.InvalidParameter: The thinking_budget ' + 'parameter must be a positive integer and not greater than 0') + + def __init__(self, msg=MSG): + super().__init__(msg) + self.status_code = 400 + + +class _Log: + + def warning(self, *a, **k): + pass + + +def _streaming_factory(reject_thinking=True): + """Returns fine, fails only while the first chunk is read.""" + seen = [] + + def create(**kw): + extra = kw.get('extra_body') or {} + asking = bool(extra.get('thinking_budget')) and not T.asks_to_disable(kw) + seen.append(asking) + + def gen(): + if asking and reject_thinking: + raise _Boom400() + yield 'chunk-1' + yield 'chunk-2' + + return gen() + + return create, seen + + +def test_stream_time_thinking_refusal_is_repaired(): + """Regression: `thinking_budget` rejected mid-stream used to bypass the + fallback completely — no retry, no memo, raw 400 shown to the user.""" + T.MODELS_REFUSING_THINKING.clear() + create, seen = _streaming_factory() + stream = T.create_with_thinking_fallback( + create, client=None, model='stream-model', logger=_Log(), + extra_body={'thinking_budget': 4096}) + assert list(stream) == ['chunk-1', 'chunk-2'] + assert seen == [True, False] # asked, then repaired + assert any(k[1] == 'stream-model' for k in T.MODELS_REFUSING_THINKING) + T.MODELS_REFUSING_THINKING.clear() + + +def test_stream_failure_after_first_chunk_is_not_retried(): + calls = [] + + def create(**kw): + calls.append(kw) + + def gen(): + yield 'chunk-1' + raise _Boom400() + + return gen() + + T.MODELS_REFUSING_THINKING.clear() + stream = T.create_with_thinking_fallback( + create, client=None, model='late-model', logger=_Log(), + extra_body={'thinking_budget': 4096}) + got = [] + with pytest.raises(_Boom400): + for item in stream: + got.append(item) + assert got == ['chunk-1'] + assert len(calls) == 1 # already rendered; no restart + T.MODELS_REFUSING_THINKING.clear() + + +def test_non_stream_result_is_passed_through_untouched(): + def create(**kw): + return 'completion' + + assert T.create_with_thinking_fallback( + create, client=None, model='plain', logger=_Log()) == 'completion' diff --git a/tests/llm/test_thinking_fallback.py b/tests/llm/test_thinking_fallback.py new file mode 100644 index 000000000..36fe3f4c5 --- /dev/null +++ b/tests/llm/test_thinking_fallback.py @@ -0,0 +1,153 @@ +# Copyright (c) ModelScope Contributors. All rights reserved. +"""Thinking parameters are dropped and retried when a model refuses them. + +Support for "thinking" is per-model and unpredictable from the name — probing +one provider's 122 chat models turned up refusals across vision, OCR, omni, +open-weight and even non-Qwen families — and a refusal is a hard 400, not an +ignored flag. So the client does not try to know: it asks, and on a refusal +retries once with thinking off, remembering the model. +""" +import pytest + +from ms_agent.llm import openai_llm as O +from ms_agent.llm import thinking as T +from ms_agent.llm.transport import openai_compat as TC + + +class _Recorder: + """Stands in for ``client.chat.completions``; records every call and fails + the ones that ask for thinking.""" + + def __init__(self, refuse: str = 'thinking_budget must be positive'): + self.calls = [] + self.refuse = refuse + + def create(self, **kwargs): + self.calls.append(kwargs) + extra = kwargs.get('extra_body') or {} + asked = extra.get('enable_thinking') or kwargs.get('enable_thinking') + if asked and self.refuse: + raise RuntimeError(f'Error code: 400 - {self.refuse}') + return f'completion-{len(self.calls)}' + + +def _client(recorder): + ns = type('NS', (), {}) + client = ns() + client.chat = ns() + client.chat.completions = recorder + client.base_url = 'https://example.test/v1' + return client + + +def _llm(recorder, model='some-model'): + llm = O.OpenAI.__new__(O.OpenAI) # bypass __init__ (config/network) + llm.client = _client(recorder) + llm.model = model + llm.args = {} + llm._format_input_message = lambda m: m + return llm + + +@pytest.fixture(autouse=True) +def _clear_memo(): + T.MODELS_REFUSING_THINKING.clear() + yield + T.MODELS_REFUSING_THINKING.clear() + + +def test_refusal_is_retried_with_thinking_explicitly_off(): + rec = _Recorder() + out = _llm(rec)._call_llm([], None, extra_body={'enable_thinking': True}) + + assert out == 'completion-2' # the retry's result, not an exception + assert len(rec.calls) == 2 + # Explicitly OFF, not merely absent: some models default it on and then + # refuse the call ("must be set to false for non-stream call"). + assert rec.calls[1]['extra_body'] == {'enable_thinking': False} + + +def test_budget_keys_are_dropped_and_other_extras_kept(): + rec = _Recorder() + _llm(rec)._call_llm([], + None, + extra_body={ + 'enable_thinking': True, + 'thinking_budget': 512, + 'unrelated': 'keep me' + }) + retried = rec.calls[1]['extra_body'] + assert retried == {'enable_thinking': False, 'unrelated': 'keep me'} + + +def test_the_refusal_is_remembered_so_later_turns_cost_one_call(): + rec = _Recorder() + llm = _llm(rec) + llm._call_llm([], None, extra_body={'enable_thinking': True}) + assert len(rec.calls) == 2 + + llm._call_llm([], None, extra_body={'enable_thinking': True}) + assert len(rec.calls) == 3 # no failed attempt this time + assert rec.calls[2]['extra_body'] == {'enable_thinking': False} + + +def test_memo_is_per_model(): + rec = _Recorder() + _llm(rec, model='refuser')._call_llm([], + None, + extra_body={'enable_thinking': True}) + assert len(rec.calls) == 2 + # A different model on the same endpoint must still get its chance to think. + _llm(rec, model='thinker')._call_llm([], + None, + extra_body={'enable_thinking': True}) + assert len(rec.calls) == 4 + assert rec.calls[2]['extra_body'] == {'enable_thinking': True} + + +def test_unrelated_400_is_not_retried(): + rec = _Recorder(refuse='context length exceeded') + + class _Always(_Recorder): + + def create(self, **kwargs): + self.calls.append(kwargs) + raise RuntimeError('Error code: 400 - context length exceeded') + + rec = _Always() + with pytest.raises(RuntimeError, match='context length'): + _llm(rec)._call_llm([], None, extra_body={'enable_thinking': True}) + assert len(rec.calls) == 1 + + +def test_a_request_without_thinking_is_never_retried(): + """A 400 that merely mentions thinking, on a call that asked for none, is + somebody else's problem — retrying would hide it.""" + + class _Always(_Recorder): + + def create(self, **kwargs): + self.calls.append(kwargs) + raise RuntimeError('Error code: 400 - thinking is unsupported') + + rec = _Always() + with pytest.raises(RuntimeError): + _llm(rec)._call_llm([], None, temperature=0.5) + assert len(rec.calls) == 1 + assert not T.MODELS_REFUSING_THINKING + + +def test_the_router_transport_heals_too(): + """The WebUI does not go through llm/openai_llm.py at all — its provider + router uses transport/openai_compat.py. A fallback that only covered one of + them looked fine in unit tests and still failed in the browser.""" + rec = _Recorder() + tr = TC.OpenAICompatTransport.__new__(TC.OpenAICompatTransport) + tr.client = _client(rec) + tr.model = 'refuser' + tr.args = {} + tr._format_input_message = lambda m: m + + out = tr._call_llm([], None, extra_body={'enable_thinking': True}) + assert out == 'completion-2' + assert rec.calls[1]['extra_body'] == {'enable_thinking': False} diff --git a/tests/llm/test_vision_fallback.py b/tests/llm/test_vision_fallback.py new file mode 100644 index 000000000..b487ce4a9 --- /dev/null +++ b/tests/llm/test_vision_fallback.py @@ -0,0 +1,435 @@ +# Copyright (c) ModelScope Contributors. All rights reserved. +"""Image-refusal attribution and the one-shot fallback. + +The behaviour under test was shaped by a seven-provider sweep (2026-08): +DashScope is the ONLY provider that hard-400s on image content, and its message +("Unexpected item type in content") names neither image nor multimodal nor +vision — so keyword matching cannot work. Meanwhile a 400 on an image-carrying +request also covers model-not-found and auth, so the status code alone cannot +decide either. Hence: retry wide, blacklist only on a retry that SUCCEEDS. +""" +import unittest + +from ms_agent.llm import vision as V + + +class _Boom(Exception): + + def __init__(self, status=400, msg='bad request'): + super().__init__(msg) + self.status_code = status + + +IMG_MESSAGES = [{ + 'role': 'user', + 'content': [ + {'type': 'text', 'text': 'Image 1: a.png'}, + {'type': 'image_url', 'image_url': {'url': 'data:image/png;base64,AA'}}, + {'type': 'text', 'text': 'what is this'}, + ], +}] + + +class TestIsImageRefusal(unittest.TestCase): + + def test_requires_images_on_the_wire(self): + # A 400 with no images in the request is somebody else's problem. + self.assertFalse(V.is_image_refusal(_Boom(400), sent_images=False)) + self.assertTrue(V.is_image_refusal(_Boom(400), sent_images=True)) + + def test_only_400(self): + for status in (401, 404, 429, 500, 503): + self.assertFalse( + V.is_image_refusal(_Boom(status), sent_images=True), + f'{status} must not be attributed to images') + + def test_status_from_nested_response(self): + + class Wrapped(Exception): + + class response: # noqa: N801 + status_code = 400 + + self.assertTrue(V.is_image_refusal(Wrapped(), sent_images=True)) + + def test_falls_back_to_text_when_status_is_lost(self): + self.assertTrue( + V.is_image_refusal(Exception('Error code: 400 - oops'), + sent_images=True)) + self.assertFalse( + V.is_image_refusal(Exception('some transport hiccup'), + sent_images=True)) + + +class TestStripImages(unittest.TestCase): + + def test_replaces_image_blocks_and_keeps_labels(self): + out, changed = V.strip_images_from_messages(IMG_MESSAGES) + self.assertTrue(changed) + body = out[0]['content'] + self.assertIsInstance(body, str) + self.assertIn('Image 1: a.png', body) # the label survives + self.assertIn('what is this', body) # so does the question + self.assertNotIn('base64', body) # the pixels do not + # The reason and the remedy are both present, so the model can explain + # itself instead of answering "please upload the image". + self.assertIn('Settings', body) + + def test_text_only_is_untouched(self): + msgs = [{'role': 'user', 'content': 'plain'}] + out, changed = V.strip_images_from_messages(msgs) + self.assertFalse(changed) + self.assertEqual(out, msgs) + + +class TestCreateWithVisionFallback(unittest.TestCase): + + def setUp(self): + V.MODELS_REFUSING_IMAGES.clear() + + def test_happy_path_is_a_passthrough(self): + calls = [] + + def create(messages, **kw): + calls.append(messages) + return 'ok' + + got = V.create_with_vision_fallback( + create, base_url='u', model='m', messages=IMG_MESSAGES, + sent_images=True) + self.assertEqual(got, 'ok') + self.assertEqual(len(calls), 1) + self.assertFalse(V.MODELS_REFUSING_IMAGES) + + def test_image_refusal_retries_without_images_and_remembers(self): + seen = [] + + def create(messages, **kw): + seen.append(messages) + if len(seen) == 1: + raise _Boom(400, 'Unexpected item type in content.') + return 'recovered' + + got = V.create_with_vision_fallback( + create, base_url='u', model='m', messages=IMG_MESSAGES, + sent_images=True) + self.assertEqual(got, 'recovered') + self.assertEqual(len(seen), 2) + self.assertIsInstance(seen[1][0]['content'], str) + self.assertIn(('u', 'm'), V.MODELS_REFUSING_IMAGES) + + def test_unrelated_400_does_not_blacklist_and_reraises_the_original(self): + """Regression: ModelScope answers "Model id ... has no provider + supported" with a 400. Attributing that to images wasted a round-trip + AND permanently stopped sending images to a model whose real problem was + that it did not exist.""" + original = _Boom(400, 'Model id : X , has no provider supported') + + def create(messages, **kw): + raise original + + with self.assertRaises(_Boom) as ctx: + V.create_with_vision_fallback( + create, base_url='u', model='m', messages=IMG_MESSAGES, + sent_images=True) + self.assertIs(ctx.exception, original) # the real error, not the retry's + self.assertFalse(V.MODELS_REFUSING_IMAGES) + + def test_known_refuser_skips_the_doomed_first_attempt(self): + V.note_refusal('u', 'm') + seen = [] + + def create(messages, **kw): + seen.append(messages) + return 'ok' + + V.create_with_vision_fallback( + create, base_url='u', model='m', messages=IMG_MESSAGES, + sent_images=True) + self.assertEqual(len(seen), 1) + self.assertIsInstance(seen[0][0]['content'], str) + + def test_non_image_error_propagates_untouched(self): + + def create(messages, **kw): + raise _Boom(429, 'rate limited') + + with self.assertRaises(_Boom): + V.create_with_vision_fallback( + create, base_url='u', model='m', messages=IMG_MESSAGES, + sent_images=True) + self.assertFalse(V.MODELS_REFUSING_IMAGES) + + +class TestResolveSupportsVision(unittest.TestCase): + + def setUp(self): + V.MODELS_REFUSING_IMAGES.clear() + + def test_explicit_switch_wins(self): + from omegaconf import OmegaConf + on = OmegaConf.create({'llm': {'supports_vision': True}}) + off = OmegaConf.create({'llm': {'supports_vision': False}}) + self.assertTrue(V.resolve_supports_vision(on)) + self.assertFalse(V.resolve_supports_vision(off)) + + def test_quoted_false_is_honoured(self): + """`supports_vision: "false"` is a common YAML slip; bare bool() would + read it as ON, i.e. exactly the opposite of what was asked.""" + from omegaconf import OmegaConf + cfg = OmegaConf.create({'llm': {'supports_vision': 'false'}}) + self.assertFalse(V.resolve_supports_vision(cfg)) + cfg = OmegaConf.create({'llm': {'supports_vision': 'yes'}}) + self.assertTrue(V.resolve_supports_vision(cfg)) + + def test_observed_refusal_overrides_an_explicit_yes(self): + from omegaconf import OmegaConf + cfg = OmegaConf.create({'llm': {'supports_vision': True}}) + V.note_refusal('u', 'm') + self.assertFalse( + V.resolve_supports_vision(cfg, model='m', base_url='u')) + + def test_unset_is_off_even_when_the_provider_declares_vision(self): + """Two states, default OFF — the provider's capability is NOT evidence. + + Nine of ten registry entries declare ``vision``, so consulting the spec + made "nobody has said" mean "send images" and the switch's OFF position + describe a state the runtime never used. Vision is a property of the + model (ModelScope serves Qwen3-VL and the text-only Qwen3-235B through + one provider entry), so only the per-model switch turns it on. + """ + from omegaconf import OmegaConf + from ms_agent.llm.spec import get_registry + cfg = OmegaConf.create({'llm': {'model': 'x'}}) + for provider in ('dashscope', 'modelscope', 'kimi', 'openai'): + spec = get_registry().get(provider) + self.assertFalse( + V.resolve_supports_vision(cfg, spec=spec), + f'{provider}: unset must stay OFF regardless of its caps') + self.assertFalse(V.resolve_supports_vision(cfg, spec=None)) + + def test_only_the_switch_turns_images_on(self): + from omegaconf import OmegaConf + from ms_agent.llm.spec import get_registry + spec = get_registry().get('dashscope') + on = OmegaConf.create({'llm': {'supports_vision': True}}) + self.assertTrue(V.resolve_supports_vision(on, spec=spec)) + + +class TestDisabledReason(unittest.TestCase): + """Which explanation the model is handed when the pixels are absent.""" + + def setUp(self): + V.MODELS_REFUSING_IMAGES.clear() + + def tearDown(self): + V.MODELS_REFUSING_IMAGES.clear() + + def test_switch_off_points_at_the_switch(self): + reason = V.disabled_reason('u', 'm') + self.assertIn('Settings', reason) + self.assertNotIn('rejected image input', reason) + + def test_endpoint_refusal_does_not_point_at_the_switch(self): + """Regression: telling a user who already enabled the switch to enable + it is the single most confusing thing this feature can say.""" + V.note_refusal('u', 'm') + reason = V.disabled_reason('u', 'm') + self.assertIn('rejected image input', reason) + self.assertNotIn('Settings → Models', reason) + + +class TestStreamTimeRefusal(unittest.TestCase): + """A 400 that arrives on the FIRST CHUNK, not out of ``create()``. + + Aliyun-family gateways answer 200 and then put the rejection in the stream. + Guarding only ``create()`` let that error bypass the retry entirely: no + repair, no blacklist, raw provider error to the user. + """ + + def setUp(self): + V.MODELS_REFUSING_IMAGES.clear() + + def tearDown(self): + V.MODELS_REFUSING_IMAGES.clear() + + @staticmethod + def _streaming_create(reject_images: bool = True): + """A client that returns fine and only fails while being consumed.""" + seen = [] + + def create(messages, **kw): + has_img = any( + V.multimodal.has_image_blocks(m.get('content')) + for m in messages if isinstance(m, dict)) + seen.append(has_img) + + def gen(): + if has_img and reject_images: + raise _Boom(400, 'Unexpected item type in content.') + yield 'chunk-1' + yield 'chunk-2' + + return gen() + + return create, seen + + def test_first_chunk_refusal_is_repaired_and_remembered(self): + create, seen = self._streaming_create() + stream = V.create_with_vision_fallback( + create, base_url='u', model='m', messages=IMG_MESSAGES, + sent_images=True) + self.assertEqual(list(stream), ['chunk-1', 'chunk-2']) + self.assertEqual(seen, [True, False]) # with images, then without + self.assertIn(('u', 'm'), V.MODELS_REFUSING_IMAGES) + + def test_unrelated_stream_error_reraises_and_does_not_blacklist(self): + """The retry fails too -> the images were not the cause.""" + original = _Boom(400, 'Model id : X , has no provider supported') + + def create(messages, **kw): + def gen(): + raise original + yield # pragma: no cover + return gen() + + stream = V.create_with_vision_fallback( + create, base_url='u', model='m', messages=IMG_MESSAGES, + sent_images=True) + with self.assertRaises(_Boom) as ctx: + list(stream) + self.assertIs(ctx.exception, original) + self.assertFalse(V.MODELS_REFUSING_IMAGES) + + def test_failure_after_the_first_chunk_is_not_retried(self): + """Output already reached the user; restarting would duplicate it.""" + calls = [] + + def create(messages, **kw): + calls.append(1) + + def gen(): + yield 'chunk-1' + raise _Boom(400, 'Unexpected item type in content.') + + return gen() + + stream = V.create_with_vision_fallback( + create, base_url='u', model='m', messages=IMG_MESSAGES, + sent_images=True) + got = [] + with self.assertRaises(_Boom): + for item in stream: + got.append(item) + self.assertEqual(got, ['chunk-1']) + self.assertEqual(len(calls), 1) # no retry + self.assertFalse(V.MODELS_REFUSING_IMAGES) + + def test_non_streaming_result_is_untouched(self): + def create(messages, **kw): + return 'plain-response' + + self.assertEqual( + V.create_with_vision_fallback( + create, base_url='u', model='m', messages=IMG_MESSAGES, + sent_images=True), 'plain-response') + + +class TestTransportWiring(unittest.TestCase): + """The wrapper's named arguments must not collide with the API params. + + ``create_with_vision_fallback`` takes ``model`` and ``messages`` as named + arguments and forwards everything else to the factory. A transport that also + leaves those keys in the dict it splats raises + ``TypeError: got multiple values for keyword argument 'model'`` on EVERY + call — a total outage of that transport, not a vision-only edge case. It + reached a real endpoint before it was caught, so it is pinned here for both + transport families. + """ + + def setUp(self): + V.MODELS_REFUSING_IMAGES.clear() + + def _call(self, transport_params): + """Drive the wrapper the way a transport does and return the API kwargs.""" + seen = {} + + def factory(messages, **kw): + seen.update(kw) + seen['messages'] = messages + # Mimic a real client: it needs `model` named in the call. + assert 'model' in seen, 'the API call was made without a model' + return 'ok' + + params = dict(transport_params) + rest = { + k: v + for k, v in params.items() if k not in ('model', 'messages') + } + out = V.create_with_vision_fallback( + lambda messages, **kw: factory( + messages, model=params['model'], **kw), + base_url='https://example/v1', + model=params['model'], + messages=params['messages'], + sent_images=False, + **rest) + return out, seen + + def test_anthropic_shaped_params_do_not_collide(self): + out, seen = self._call({ + 'model': 'claude-x', + 'messages': IMG_MESSAGES, + 'max_tokens': 1024, + 'thinking': { + 'type': 'disabled', + 'budget_tokens': 1024 + }, + 'system': 'be brief', + }) + self.assertEqual(out, 'ok') + # model survives to the API call, and the other params are untouched. + self.assertEqual(seen['model'], 'claude-x') + self.assertEqual(seen['max_tokens'], 1024) + self.assertEqual(seen['system'], 'be brief') + self.assertEqual(seen['messages'], IMG_MESSAGES) + + def test_real_anthropic_transport_builds_a_valid_call(self): + """End-to-end through AnthropicMessagesTransport._call_llm itself.""" + from ms_agent.llm.transport import anthropic_messages as AM + from ms_agent.llm.utils import Message + + calls = [] + + class _Messages: + + def create(self, **kw): + calls.append(kw) + return 'created' + + def stream(self, **kw): + calls.append(kw) + return 'streamed' + + class _Client: + base_url = 'https://api.deepseek.com/anthropic' + messages = _Messages() + + transport = AM.AnthropicMessagesTransport.__new__( + AM.AnthropicMessagesTransport) + transport.client = _Client() + transport.model = 'deepseek-v4-pro' + transport.vision = None + transport.vision_supported = False + + out = transport._call_llm( + [Message(role='user', content='hi')], tools=None, stream=False) + + self.assertEqual(out, 'created') + self.assertEqual(len(calls), 1) + self.assertEqual(calls[0]['model'], 'deepseek-v4-pro') + self.assertEqual(calls[0]['messages'][0]['role'], 'user') + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/memory/test_orchestrator_teardown.py b/tests/memory/test_orchestrator_teardown.py new file mode 100644 index 000000000..d77b84fad --- /dev/null +++ b/tests/memory/test_orchestrator_teardown.py @@ -0,0 +1,231 @@ +# Copyright (c) ModelScope Contributors. All rights reserved. +"""Interrupt / teardown hazards around the ingest ledger and the store lock. + +All four guard the same property from different sides: a memory that the +conversation produced is either written or still owed — never quietly dropped, +and never written into a store whose owner has let go of it. + +* an interrupt must not advance the ledger past a write that is still running + (the write then finds an empty delta, or fails and is denied its retry); +* the per-store lock must survive a process that runs more than one event loop; +* retrieval takes that lock too, so it cannot read a store mid-write; +* a closed orchestrator must not be reopened by a straggling ingest. +""" +import asyncio + +import pytest + +from ms_agent.llm.utils import Message +from ms_agent.memory.unified.config import MemoryConfig +from ms_agent.memory.unified.orchestrator import (MemoryOrchestrator, + _store_lock) + + +class SlowBackend: + """Records what it was actually asked to write, slowly enough to overlap.""" + + def __init__(self, delay: float = 0.05): + self.delay = delay + self.batches = [] + self.searches = [] + self.starts = 0 + self.closes = 0 + self.wrote_after_close = False + + async def start(self, **kwargs): + self.starts += 1 + + async def on_messages(self, messages, **kwargs): + await asyncio.sleep(self.delay) + if self.closes: + self.wrote_after_close = True + self.batches.append([m['content'] for m in messages]) + return len(messages) + + async def inject(self, messages): + return messages + + async def search(self, query, limit=10): + # Recorded on ENTRY: the question is whether retrieval reaches the + # store while a write holds it, not when it finishes. + self.searches.append(query) + await asyncio.sleep(self.delay) + return [] + + async def on_pre_compress(self, messages): + pass + + async def close(self): + self.closes += 1 + + def invalidate(self): + pass + + +def _orch(tmp_path, backend, **cfg): + orch = MemoryOrchestrator( + MemoryConfig(base_dir=str(tmp_path), storage_backend='file', **cfg)) + orch._backend = backend + orch._started = True + return orch + + +def _round(user, assistant): + return [ + Message(role='user', content=user), + Message(role='assistant', content=assistant) + ] + + +def test_interrupt_does_not_swallow_an_ingest_in_flight(tmp_path): + """The reported data loss. + + A round is being written in the background (extraction takes seconds) when + the user hits stop. The interrupt advanced the ledger over that round too, + so the write found an empty delta — the memory was neither stored nor still + owed. Reproduced here by handing ``mark_ingested`` the WHOLE history, which + is what the interrupt path used to pass: whatever a caller hands over, a + write already in flight owns its own messages until it finishes. + """ + backend = SlowBackend() + orch = _orch(tmp_path, backend) + history = _round('u1', 'a1') + + async def main(): + lock = _store_lock(str(tmp_path)) + await lock.acquire() # the scheduled ingest cannot reach its delta yet + try: + task = orch.schedule_add(history) + await asyncio.sleep(0) + orch.mark_ingested(history + _round('u2', 'half an answ')) + finally: + lock.release() + await task + + asyncio.run(main()) + assert backend.batches == [['u1', 'a1']] # the round still got written + + +def test_interrupt_marks_only_its_own_round(tmp_path): + """`mark_ingested` is fed one round, not the whole history: an earlier + round that was never ingested (a failed write, an interval skip) must stay + owed, not be written off by an unrelated interrupt.""" + backend = SlowBackend(delay=0) + orch = _orch(tmp_path, backend) + earlier = _round('u1', 'a1') + + async def main(): + orch.mark_ingested(_round('u2', 'half an answ')) + await orch.add(earlier) + + asyncio.run(main()) + assert backend.batches == [['u1', 'a1']] + + +def test_interrupt_still_seals_its_own_partial_round(tmp_path): + """...while the partial answer itself never reaches the store.""" + backend = SlowBackend(delay=0) + orch = _orch(tmp_path, backend) + partial = _round('u1', 'half an answ') + + async def main(): + orch.mark_ingested(partial) + await orch.add(partial) + + asyncio.run(main()) + assert backend.batches == [] + + +def test_store_lock_survives_a_second_event_loop(tmp_path): + """asyncio.Lock binds to the loop that first waits on it and refuses every + other one afterwards. The lock is per (loop, store) so a process that runs + several loops — the inline `asyncio.run` ingest path, a test suite — does + not wedge on a lock belonging to a loop that is already closed.""" + + async def contend(): + lock = _store_lock(str(tmp_path)) + await lock.acquire() + waiter = asyncio.create_task(_take(lock)) + await asyncio.sleep(0) # let it queue: this is what binds the loop + lock.release() + await waiter + + async def _take(lock): + async with lock: + pass + + asyncio.run(contend()) + asyncio.run(contend()) # RuntimeError: bound to a different event loop + + +def test_search_waits_for_a_write_to_finish(tmp_path): + """Retrieval used to be the one store access outside the lock.""" + backend = SlowBackend(delay=0.05) + orch = _orch(tmp_path, backend) + + async def main(): + lock = _store_lock(str(tmp_path)) + await lock.acquire() + task = asyncio.create_task(orch.search('who am i')) + await asyncio.sleep(0.02) + held = list(backend.searches) # must not have run yet + lock.release() + await task + return held, backend.searches + + during, after = asyncio.run(main()) + assert during == [] and after == ['who am i'] + + +def test_a_closed_orchestrator_never_reopens_the_store(tmp_path): + """`close()` releases an embedded store's file lock, so anything that + reopens it afterwards takes that lock behind the owner's back.""" + backend = SlowBackend(delay=0) + orch = _orch(tmp_path, backend) + + async def main(): + await orch.close() + await orch.add(_round('u1', 'a1')) + await orch.run(_round('u2', 'a2')) + return await orch.search('anything') + + found = asyncio.run(main()) + assert backend.starts == 0 and backend.closes == 1 + assert backend.batches == [] and found == [] + + +def test_close_still_drains_what_was_already_scheduled(tmp_path): + """Retiring must not cost the writes close() promised to persist — the + order is drain, then retire.""" + backend = SlowBackend(delay=0.02) + orch = _orch(tmp_path, backend) + + async def main(): + orch.schedule_add(_round('u1', 'a1')) + await orch.close() + + asyncio.run(main()) + assert backend.batches == [['u1', 'a1']] + assert backend.wrote_after_close is False + + +def test_reconfigure_keeps_the_instance_usable(tmp_path): + """The teardown a config change performs is not a retirement: every agent + sharing this instance must keep working, now on the new configuration.""" + backend = SlowBackend(delay=0) + orch = _orch(tmp_path, backend) + + async def main(): + await orch.reconfigure( + MemoryConfig( + base_dir=str(tmp_path), + storage_backend='file', + memory_path='OTHER.md')) + assert orch._closed is False + # A fresh backend is built on demand from the new config. + orch._backend, orch._started = backend, True + await orch.add(_round('u1', 'a1')) + + asyncio.run(main()) + assert backend.closes == 1 # old backend released + assert backend.batches == [['u1', 'a1']] # instance still writes diff --git a/tests/memory/test_shared_memory_reconfigure.py b/tests/memory/test_shared_memory_reconfigure.py new file mode 100644 index 000000000..1d8034966 --- /dev/null +++ b/tests/memory/test_shared_memory_reconfigure.py @@ -0,0 +1,198 @@ +# Copyright (c) ModelScope Contributors. All rights reserved. +"""Sharing and reconfiguration of memory instances. + +``SharedMemoryManager`` hands one instance per store to every agent that asks +for it, which raises two questions these tests pin down: + +* what happens when a later agent's config differs from the one the instance + was built with — it must be adopted, otherwise editing a memory setting is + indistinguishable from the setting doing nothing; +* what happens when only the agent's model differs — the instance must still + be shared, because embedded vector stores take an exclusive file lock and a + second instance on the same path cannot open the store at all. +""" +import asyncio + +import pytest +from omegaconf import OmegaConf + +from ms_agent.memory.memory_manager import SharedMemoryManager +from ms_agent.memory.unified.config import MemoryConfig +from ms_agent.memory.unified.orchestrator import MemoryOrchestrator + + +class FakeBackend: + """Backend that keeps the config object it was constructed with, the way + every real backend does.""" + + def __init__(self, config): + self._config = config + self.closed = False + + async def start(self, **kwargs): + pass + + async def inject(self, messages): + return messages + + async def on_messages(self, messages, **kwargs): + return len(messages) + + async def on_pre_compress(self, messages): + pass + + async def close(self): + self.closed = True + + def invalidate(self): + pass + + +@pytest.fixture(autouse=True) +def _clean_instances(): + SharedMemoryManager._instances.clear() + yield + SharedMemoryManager._instances.clear() + + +def _cfg(tmp_path, *, model='m1', recall=10, backend='file', options=None): + node = { + 'storage': { + 'backend': backend + }, + 'namespace': { + 'user_id': 'p1' + }, + 'user_id': 'p1', + 'base_dir': str(tmp_path), + 'recall_top_k': recall, + } + if options is not None: + node['mem0'] = options + return OmegaConf.create({ + 'output_dir': str(tmp_path), + 'llm': { + 'model': model + }, + 'memory': { + 'unified_memory': node + }, + }) + + +def _orch(tmp_path, **cfg_kwargs): + """A live orchestrator, built the way an agent builds one.""" + orch = MemoryOrchestrator(_cfg(tmp_path, **cfg_kwargs)) + orch._backend = FakeBackend(orch.mem_config) + orch._started = True + return orch + + +def test_identical_config_is_a_noop(tmp_path): + orch = _orch(tmp_path) + backend = orch._backend + assert asyncio.run(orch.reconfigure(_cfg(tmp_path))) is False + assert backend.closed is False + assert orch._backend is backend + + +def test_recall_size_applies_without_tearing_the_store_down(tmp_path): + """The reported bug: a changed recall size must reach the LIVE backend. + + It is applied by writing through the shared MemoryConfig object rather + than rebinding it, because the backend holds a reference to that object — + rebinding would leave the backend reading the old numbers. + """ + orch = _orch(tmp_path, recall=10) + backend = orch._backend + + torn_down = asyncio.run(orch.reconfigure(_cfg(tmp_path, recall=3))) + + assert torn_down is False # no reason to close a store for a number + assert backend.closed is False + assert orch.mem_config.recall_top_k == 3 + assert backend._config.recall_top_k == 3 # what inject() actually reads + + +def test_store_affecting_change_rebuilds_the_backend(tmp_path): + orch = _orch(tmp_path, backend='mem0') + backend = orch._backend + + torn_down = asyncio.run( + orch.reconfigure( + _cfg( + tmp_path, + backend='mem0', + options={'embedder': { + 'provider': 'fastembed' + }}))) + + assert torn_down is True + assert backend.closed is True # store released, so its lock is too + assert orch._backend is None # next use builds from the new config + + +def test_switching_models_shares_one_instance(tmp_path): + """Two agents on one store, different models: one instance. + + Keying the cache by model used to hand the second agent its own instance, + which then could not open the (exclusively locked) store at all — memory + silently stopped working for whoever switched models. + """ + + async def main(): + first = await SharedMemoryManager.get_shared_memory( + _cfg(tmp_path, model='m1'), 'unified_memory') + second = await SharedMemoryManager.get_shared_memory( + _cfg(tmp_path, model='m2'), 'unified_memory') + return first, second + + first, second = asyncio.run(main()) + assert first is second + assert len(SharedMemoryManager._instances) == 1 + + +def test_manager_adopts_a_changed_recall_size(tmp_path): + + async def main(): + await SharedMemoryManager.get_shared_memory( + _cfg(tmp_path, recall=10), 'unified_memory') + return await SharedMemoryManager.get_shared_memory( + _cfg(tmp_path, recall=5), 'unified_memory') + + assert asyncio.run(main()).mem_config.recall_top_k == 5 + + +def test_different_stores_stay_separate(tmp_path): + + async def main(): + a = await SharedMemoryManager.get_shared_memory( + _cfg(tmp_path / 'a'), 'unified_memory') + b = await SharedMemoryManager.get_shared_memory( + _cfg(tmp_path / 'b'), 'unified_memory') + return a, b + + a, b = asyncio.run(main()) + assert a is not b + assert len(SharedMemoryManager._instances) == 2 + + +def test_reconfigure_failure_keeps_the_cached_instance(tmp_path): + """A broken incoming config must not take an agent's memory down with it: + the existing instance keeps serving its own configuration.""" + + async def main(): + instance = await SharedMemoryManager.get_shared_memory( + _cfg(tmp_path, recall=10), 'unified_memory') + + async def boom(_config): + raise RuntimeError('bad config') + + instance.reconfigure = boom + again = await SharedMemoryManager.get_shared_memory( + _cfg(tmp_path, recall=5), 'unified_memory') + return instance, again + + instance, again = asyncio.run(main()) + assert instance is again + assert again.mem_config.recall_top_k == 10 diff --git a/tests/memory/test_unified_memory.py b/tests/memory/test_unified_memory.py index 6ce4851bd..d459ca870 100644 --- a/tests/memory/test_unified_memory.py +++ b/tests/memory/test_unified_memory.py @@ -1566,6 +1566,107 @@ def test_format_results_limits_to_10(self): lines = [l for l in formatted.split("\n") if l.strip()] assert len(lines) == 10 + def test_format_results_stamps_the_day(self): + # mem0 2.x only ever ADDs, so contradicting memories coexist; the date + # is the only thing the model can prefer the newer one by. + results = [ + {"memory": "Answers in Chinese", "updated_at": "2026-08-01T10:00:00Z"}, + {"memory": "Answers in English", "created_at": "2026-08-11T09:00:00Z"}, + ] + formatted = Mem0Backend._format_results(results) + assert "- (2026-08-01) Answers in Chinese" in formatted + assert "- (2026-08-11) Answers in English" in formatted + + def test_format_results_omits_unusable_timestamp(self): + formatted = Mem0Backend._format_results( + [{"memory": "Uses ruff", "updated_at": "n/a"}] + ) + assert formatted == "- Uses ruff" + + def _capture_mem0_config(self, monkeypatch): + """Run start() against a stand-in mem0 and return the config it got.""" + import sys + import types + + captured = {} + + class FakeMemory: + + @staticmethod + def from_config(cfg): + captured["cfg"] = cfg + return object() + + module = types.ModuleType("mem0") + module.Memory = FakeMemory + monkeypatch.setitem(sys.modules, "mem0", module) + loop = asyncio.new_event_loop() + try: + loop.run_until_complete(self.backend.start()) + finally: + loop.close() + return captured["cfg"] + + def test_language_instructions_are_configured_by_default(self, monkeypatch): + """Memories are stored in the user's own language, because that is what + the store is later QUERIED in: a differently-worded memory has to + survive a cross-lingual embedding hop, and mem0's BM25 leg contributes + nothing across languages.""" + from ms_agent.memory.unified.backends.mem0_adapter import ( + DEFAULT_CUSTOM_INSTRUCTIONS, + ) + + cfg = self._capture_mem0_config(monkeypatch) + assert cfg["custom_instructions"] == DEFAULT_CUSTOM_INSTRUCTIONS + + def test_configured_instructions_win(self, monkeypatch): + self.backend._config.backend_options["mem0"] = { + "custom_instructions": "mine" + } + assert self._capture_mem0_config(monkeypatch)["custom_instructions"] == "mine" + + def test_mem0_appends_instructions_instead_of_replacing_its_prompt(self): + """The contract our default relies on. mem0 1.x's + `custom_fact_extraction_prompt` REPLACED the extraction prompt (losing + every built-in guideline); 2.x's `custom_instructions` is an extra + section. If that ever flips back, this fails instead of quietly + degrading extraction quality.""" + prompts = pytest.importorskip("mem0.configs.prompts") + from ms_agent.memory.unified.backends.mem0_adapter import ( + DEFAULT_CUSTOM_INSTRUCTIONS, + ) + + built = prompts.generate_additive_extraction_prompt( + existing_memories=[], + new_messages=[{"role": "user", "content": "hi"}], + custom_instructions=DEFAULT_CUSTOM_INSTRUCTIONS, + ) + for section in ("## Summary", "## Last k Messages", + "## Recently Extracted Memories", + "## Existing Memories", "## New Messages", + "## Observation Date", "## Current Date"): + assert section in built, f"built-in section {section} disappeared" + assert DEFAULT_CUSTOM_INSTRUCTIONS in built + # ...and the system prompt is mem0's own, untouched by us. + assert "Memory Extractor" in prompts.ADDITIVE_EXTRACTION_PROMPT + + def test_search_passes_limit_through(self): + # Dropping `limit` here silently capped every caller at mem0's default. + seen = {} + + class FakeMem0: + def search(self, query, filters=None, top_k=None, **kwargs): + seen["top_k"] = top_k + return {"results": [{"id": "1", "memory": "x"}]} + + self.backend._mem0 = FakeMem0() + loop = asyncio.new_event_loop() + try: + loop.run_until_complete(self.backend.search("q", limit=3)) + finally: + loop.close() + assert seen["top_k"] == 3 + def test_inject_without_mem0_passthrough(self): loop = asyncio.new_event_loop() try: @@ -1576,9 +1677,18 @@ def test_inject_without_mem0_passthrough(self): loop.close() def test_start_without_mem0_package(self): + # Simulate the absence for real: `sys.modules['mem0'] = None` makes + # `from mem0 import Memory` raise ImportError regardless of what is + # installed. Without this the test only passed by luck — on a machine + # WITH mem0, the outcome depended on whether `Memory.from_config` + # happened to find working credentials that earlier LLM-backed tests + # had exported into os.environ, which made it order-flaky. + import sys + from unittest.mock import patch loop = asyncio.new_event_loop() try: - loop.run_until_complete(self.backend.start()) + with patch.dict(sys.modules, {'mem0': None}): + loop.run_until_complete(self.backend.start()) assert self.backend._mem0 is None finally: loop.close() diff --git a/tests/permission/test_enforcer.py b/tests/permission/test_enforcer.py index c9aa2805f..4840bea8e 100644 --- a/tests/permission/test_enforcer.py +++ b/tests/permission/test_enforcer.py @@ -67,8 +67,15 @@ async def test_always_allows(self, auto_enforcer): assert 'Auto mode' in r.reason @pytest.mark.asyncio - async def test_blacklist_denies(self, auto_enforcer): - r = await auto_enforcer.check( + async def test_blacklist_denies(self): + # A blacklist entry outranks even auto mode. The list ships EMPTY now + # (network commands are ask rules, not refusals), so this states its own + # rule rather than leaning on a default. + config = PermissionConfig( + mode='auto', + blacklist=('code_executor---shell_executor:curl *', ), + ) + r = await PermissionEnforcer(config=config).check( 'code_executor---shell_executor', {'command': 'curl http://example.com'}, ) @@ -220,3 +227,216 @@ async def ask(self, tool_name, tool_args, context, suggestions=None): r = await enforcer.check('code_executor---shell_executor', {'command': 'rm -rf /'}) assert r.action == 'allow' assert r.updated_args == {'command': 'ls -la'} + + +class TestNetworkCommandsAsk: + """curl/wget/ssh/... used to sit in the DEFAULT BLACKLIST, which nothing can + override — so the agent reported "blocked" and the user had no way to permit + it, in any mode. They are ask rules now: confirmed, never silently refused.""" + + @pytest.mark.asyncio + async def test_curl_asks_in_interactive_mode(self, tmp_path): + class Probe: + asked = 0 + + async def ask(self, tool_name, tool_args, context, suggestions=None): + Probe.asked += 1 + return PermissionResponse(action=PermissionAction.ALLOW_ONCE) + + enforcer = PermissionEnforcer( + config=_interactive_config(), + handler=Probe(), + memory=PermissionMemory(project_path=tmp_path), + ) + r = await enforcer.check('code_executor---shell_executor', + {'command': 'curl --version'}) + assert r.action == 'allow' + assert Probe.asked == 1 + + @pytest.mark.asyncio + async def test_curl_still_asks_under_full_access(self, tmp_path): + """Reaching the network is worth one deliberate click even from a user + who waved the agent through everything else — the ask rule outranks the + mode AND the whitelist.""" + class Probe: + asked = 0 + + async def ask(self, tool_name, tool_args, context, suggestions=None): + Probe.asked += 1 + return PermissionResponse(action=PermissionAction.ALLOW_ONCE) + + config = PermissionConfig.from_dict({ + 'mode': 'auto', + 'whitelist': ['code_executor---shell_executor'], + }) + enforcer = PermissionEnforcer( + config=config, + handler=Probe(), + memory=PermissionMemory(project_path=tmp_path), + ) + r = await enforcer.check('code_executor---shell_executor', + {'command': 'curl https://example.com'}) + assert r.action == 'allow' + assert Probe.asked == 1 + + @pytest.mark.asyncio + async def test_ordinary_command_unaffected_in_auto_mode(self, tmp_path): + class Probe: + asked = 0 + + async def ask(self, tool_name, tool_args, context, suggestions=None): + Probe.asked += 1 + return PermissionResponse(action=PermissionAction.ALLOW_ONCE) + + enforcer = PermissionEnforcer( + config=PermissionConfig.from_dict({'mode': 'auto'}), + handler=Probe(), + memory=PermissionMemory(project_path=tmp_path), + ) + r = await enforcer.check('code_executor---shell_executor', + {'command': 'ls -la'}) + assert r.action == 'allow' + assert Probe.asked == 0 + + @pytest.mark.asyncio + async def test_curl_denied_when_nobody_can_be_asked(self, tmp_path): + """Headless: AutoPermissionHandler answers "allow" to everything, so + running the thing an ask rule exists to gate would be worse than + refusing.""" + enforcer = PermissionEnforcer( + config=PermissionConfig.from_dict({'mode': 'auto'}), + handler=AutoPermissionHandler(), + memory=PermissionMemory(project_path=tmp_path), + ) + r = await enforcer.check('code_executor---shell_executor', + {'command': 'curl https://example.com'}) + assert r.action == 'deny' + assert 'curl' in r.reason + + @pytest.mark.asyncio + async def test_allow_network_opts_out(self, tmp_path): + class Probe: + asked = 0 + + async def ask(self, tool_name, tool_args, context, suggestions=None): + Probe.asked += 1 + return PermissionResponse(action=PermissionAction.ALLOW_ONCE) + + config = PermissionConfig.from_dict({ + 'mode': 'auto', + 'allow_network': True, + }) + enforcer = PermissionEnforcer( + config=config, + handler=Probe(), + memory=PermissionMemory(project_path=tmp_path), + ) + r = await enforcer.check('code_executor---shell_executor', + {'command': 'curl https://example.com'}) + assert r.action == 'allow' + assert Probe.asked == 0 + +class TestRememberedPatternBreadth: + """`allow_always` remembering the approval at PROJECT scope is BY DESIGN — + the user asked for "从此放行". What was wrong is WHAT got remembered: with no + pattern supplied the bare TOOL NAME was stored, and for the shell that means + every future command, so approving `ls -la` once permanently released the + entire shell — which is what made it look like the whole project had been + switched to full access.""" + + class _PatternlessAlways: + """A UI that answers the ask without naming a pattern — what the WebUI + authorization card sends.""" + + async def ask(self, tool_name, tool_args, context, suggestions=None): + return PermissionResponse(action=PermissionAction.ALLOW_ALWAYS) + + @pytest.mark.asyncio + async def test_shell_remembers_the_command_not_the_whole_tool(self, tmp_path): + memory = PermissionMemory(project_path=tmp_path) + enforcer = PermissionEnforcer( + config=_interactive_config(), + handler=self._PatternlessAlways(), + memory=memory, + ) + r = await enforcer.check('code_executor---shell_executor', + {'command': 'ls -la'}) + assert r.action == 'allow' + # The approved command, remembered — including with no arguments at all. + assert memory.matches('code_executor---shell_executor', + {'command': 'ls /tmp'}) + assert memory.matches('code_executor---shell_executor', + {'command': 'ls'}) + # A DIFFERENT command is not covered by having approved `ls`. + assert not memory.matches('code_executor---shell_executor', + {'command': 'rm -rf build'}) + + @pytest.mark.asyncio + async def test_the_narrow_pattern_persists_to_the_project(self, tmp_path): + """The approval outliving the conversation is the FEATURE; only its + reach across commands was ever too wide.""" + enforcer = PermissionEnforcer( + config=_interactive_config(), + handler=self._PatternlessAlways(), + memory=PermissionMemory(project_path=tmp_path), + ) + await enforcer.check('code_executor---shell_executor', + {'command': 'ls -la'}) + # A fresh memory — i.e. a new conversation — reads the same file back. + reloaded = PermissionMemory(project_path=tmp_path) + assert reloaded.matches('code_executor---shell_executor', + {'command': 'ls -la'}) + assert not reloaded.matches('code_executor---shell_executor', + {'command': 'curl https://example.com'}) + + @pytest.mark.asyncio + async def test_argument_less_command_remembers_itself(self, tmp_path): + """Approving bare `whoami` must cover `whoami` — the remembered pattern + used not to match the very command it was generated from.""" + memory = PermissionMemory(project_path=tmp_path) + enforcer = PermissionEnforcer( + config=_interactive_config(), + handler=self._PatternlessAlways(), + memory=memory, + ) + await enforcer.check('code_executor---shell_executor', + {'command': 'whoami'}) + assert memory.matches('code_executor---shell_executor', + {'command': 'whoami'}) + + @pytest.mark.asyncio + async def test_fallback_never_widens_past_the_tool(self, tmp_path): + """`web_search` suggests a server-wide `web_search---*`; a FALLBACK must + not be broader than the tool the user actually approved.""" + memory = PermissionMemory(project_path=tmp_path) + enforcer = PermissionEnforcer( + config=_interactive_config(), + handler=self._PatternlessAlways(), + memory=memory, + ) + await enforcer.check('web_search---search', {'query': 'x'}) + assert memory.matches('web_search---search', {'query': 'y'}) + assert not memory.matches('web_search---fetch_page', {'url': 'z'}) + + @pytest.mark.asyncio + async def test_a_caller_supplied_pattern_still_wins(self, tmp_path): + """A UI that DOES put the suggestion list in front of the user (the TUI) + keeps deciding the breadth itself — the fallback only fills a gap.""" + + class Chooses: + async def ask(self, tool_name, tool_args, context, suggestions=None): + return PermissionResponse( + action=PermissionAction.ALLOW_ALWAYS, + pattern='code_executor---shell_executor', + ) + + memory = PermissionMemory(project_path=tmp_path) + enforcer = PermissionEnforcer( + config=_interactive_config(), + handler=Chooses(), + memory=memory, + ) + await enforcer.check('code_executor---shell_executor', + {'command': 'ls -la'}) + assert memory.matches('code_executor---shell_executor', + {'command': 'rm -rf build'}) diff --git a/tests/permission/test_matcher.py b/tests/permission/test_matcher.py index 6e8e64bf6..c1b3b73dc 100644 --- a/tests/permission/test_matcher.py +++ b/tests/permission/test_matcher.py @@ -82,3 +82,43 @@ def test_non_string_content_is_coerced(self, matcher): {'path': ['/tmp/a', '/tmp/b']}, ) assert isinstance(result, bool) + + +class TestBareCommandVariant: + """`` *`` means "that command with any arguments" — and with NONE is a + case of that. fnmatch wants the space plus a character, so bare ``curl`` + slipped past the very ask rule written to gate it, and a remembered + ``whoami *`` failed to match the ``whoami`` it was generated from.""" + + TOOL = 'code_executor---shell_executor' + + def _m(self, pattern: str, command: str) -> bool: + return PermissionMatcher().match_with_content( + f'{self.TOOL}:{pattern}', self.TOOL, {'command': command}) + + def test_argument_less_command_matches(self): + assert self._m('whoami *', 'whoami') + assert self._m('curl *', 'curl') + + def test_command_with_arguments_still_matches(self): + assert self._m('whoami *', 'whoami --version') + assert self._m('curl *', 'curl https://example.com') + + def test_does_not_match_a_longer_command_name(self): + assert not self._m('ls *', 'lsof') + + def test_applies_per_alternative(self): + assert self._m('ls *|cat *', 'cat') + assert not self._m('ls *|cat *', 'rm') + + def test_leaves_non_space_star_patterns_alone(self): + # `dd if=*` / `rm -rf /*`: the trailing component is meaningful, not an + # optional argument list, so the bare command must NOT match. + assert self._m('dd if=*', 'dd if=/dev/zero') + assert not self._m('dd if=*', 'dd') + assert not self._m('rm -rf /*', 'rm') + + def test_path_patterns_unaffected(self): + assert not PermissionMatcher().match_with_content( + 'file_system---read_file:~/.ssh/*', 'file_system---read_file', + {'path': '~/.ssh'}) diff --git a/tests/permission/test_path_validator.py b/tests/permission/test_path_validator.py index f81760066..bcefebf73 100644 --- a/tests/permission/test_path_validator.py +++ b/tests/permission/test_path_validator.py @@ -195,3 +195,53 @@ def test_relative_glob(self): def test_root_glob(self): assert get_glob_base_directory('/*') == '/' + + +class TestDangerousRemovalWithConfiguredPatterns: + """The same check as above, but WITH the patterns production actually + configures. The default list starts with ``*``, and fnmatch's ``*`` crosses + ``/`` — so every path came back "dangerous" and NO removal was possible: + `rm build/out.txt` was refused by the non-bypassable safety layer, in every + mode, with nothing the user could do about it. The tests above never caught + it because they pass no patterns at all.""" + + @staticmethod + def _patterns(): + from ms_agent.permission.config import SafetyConfig + return SafetyConfig().dangerous_removal_paths + + @pytest.mark.parametrize('path', [ + 'build/out.txt', + 'a.log', + './tmp/x', + 'dist/', + os.path.join(os.path.expanduser('~'), 'proj/build/x.o'), + '~/proj/build/x.o', + ]) + def test_ordinary_removals_are_allowed(self, path): + assert not is_dangerous_removal_path(path, self._patterns()) + + @pytest.mark.parametrize('path', [ + '*', + '/*', + '/', + '/etc', + '/usr', + 'build/*', + ]) + def test_dangerous_removals_still_refused(self, path): + assert is_dangerous_removal_path(path, self._patterns()) + + @pytest.mark.parametrize('path', ['~', '~/']) + def test_literal_home_refused(self, path): + """`rm ~` is judged on both the written form and the expanded one. It + used to be caught only by the match-everything accident above.""" + assert is_dangerous_removal_path(path, self._patterns()) + + def test_a_real_glob_pattern_still_globs(self): + """Only a separators-and-stars entry is literal-only; a pattern with an + actual path in it keeps working as a glob.""" + assert is_dangerous_removal_path('/opt/data/db', + ('/opt/data/*', )) + assert not is_dangerous_removal_path('/opt/other/db', + ('/opt/data/*', )) diff --git a/tests/permission/test_safety.py b/tests/permission/test_safety.py index 4970fa37c..83f0a39c3 100644 --- a/tests/permission/test_safety.py +++ b/tests/permission/test_safety.py @@ -285,21 +285,86 @@ def test_relative_path_outside_workspace_root(self): assert r.action == 'deny' -class TestDefaultBlacklist: - """PermissionConfig includes default network command blacklist.""" +class TestDefaultNetworkRules: + """Network-egress commands are CONFIRMED by default, not refused. - def test_default_blacklist_contains_curl(self): + They used to be default BLACKLIST entries, which nothing can override — so + an agent asked to fetch a URL reported it had been blocked and the user had + no way to permit it, in any mode. They are ask rules now, and the blacklist + ships empty: it is the wrong tool for "risky, ask first".""" + + def test_curl_is_an_ask_rule_not_a_blacklist_entry(self): + from ms_agent.permission.config import PermissionConfig + config = PermissionConfig.from_dict({}) + assert any('curl' in p for p in config.ask_rules) + assert not any('curl' in p for p in config.blacklist) + + def test_wget_is_an_ask_rule(self): from ms_agent.permission.config import PermissionConfig - config = PermissionConfig() - assert any('curl' in p for p in config.blacklist) + config = PermissionConfig.from_dict({}) + assert any('wget' in p for p in config.ask_rules) - def test_default_blacklist_contains_wget(self): + def test_default_blacklist_is_empty(self): from ms_agent.permission.config import PermissionConfig - config = PermissionConfig() - assert any('wget' in p for p in config.blacklist) + assert PermissionConfig().blacklist == () - def test_user_blacklist_merged(self): + def test_user_blacklist_kept_alongside_default_ask_rules(self): from ms_agent.permission.config import PermissionConfig config = PermissionConfig.from_dict({'blacklist': ['custom---tool']}) - assert any('curl' in p for p in config.blacklist) - assert 'custom---tool' in config.blacklist + assert config.blacklist == ('custom---tool', ) + assert any('curl' in p for p in config.ask_rules) + + def test_user_ask_rules_merged_with_defaults(self): + from ms_agent.permission.config import PermissionConfig + config = PermissionConfig.from_dict({'ask_rules': ['custom---tool']}) + assert 'custom---tool' in config.ask_rules + assert any('curl' in p for p in config.ask_rules) + + def test_allow_network_drops_the_default_ask_rules(self): + from ms_agent.permission.config import PermissionConfig + config = PermissionConfig.from_dict({ + 'allow_network': True, + 'ask_rules': ['custom---tool'], + }) + assert config.ask_rules == ('custom---tool', ) + + +class TestOrdinaryRemovalReachesTheAsk: + """`rm` must be CONFIRMED, not refused. SafetyGuard is the non-bypassable + inner layer, so a denial there cannot be overridden by the mode or by the + user answering a prompt — and the default `dangerous_removal_paths` list + made every single path "dangerous", so `rm build/out.txt` was flatly + refused and no removal was possible at all.""" + + @staticmethod + def _guard(tmp_path): + from ms_agent.permission.config import SafetyConfig + from ms_agent.permission.safety import SafetyGuard + return SafetyGuard( + SafetyConfig(), + allowed_dirs=[str(tmp_path)], + workspace_root=str(tmp_path), + ) + + @pytest.mark.parametrize('command', [ + 'rm probe.txt', + 'rm -rf build', + 'rm ./dist/bundle.js', + ]) + def test_ordinary_removal_allowed_through_to_the_ask(self, tmp_path, command): + d = self._guard(tmp_path).check('code_executor---shell_executor', + {'command': command}) + assert d.action == 'allow', d.reason + + @pytest.mark.parametrize('command', [ + 'rm -rf /', + 'rm -rf /*', + 'rm *', + 'rm ~', + 'rm -rf /etc', + 'rm /usr', + ]) + def test_dangerous_removal_still_refused(self, tmp_path, command): + d = self._guard(tmp_path).check('code_executor---shell_executor', + {'command': command}) + assert d.action == 'deny', d.reason