From 1bac93c41ef6dab70601cc2c86cb95786bad196d Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Fri, 10 Jul 2026 18:10:25 +0800 Subject: [PATCH 01/15] fix bakend logic --- ms_agent/agent/llm_agent.py | 50 ++++++++++++++++++++++++++++++++----- ms_agent/llm/openai_llm.py | 8 ++++++ requirements/framework.txt | 1 + 3 files changed, 53 insertions(+), 6 deletions(-) diff --git a/ms_agent/agent/llm_agent.py b/ms_agent/agent/llm_agent.py index c5448c1ea..3028d1287 100644 --- a/ms_agent/agent/llm_agent.py +++ b/ms_agent/agent/llm_agent.py @@ -1468,8 +1468,27 @@ async def step( _response_message = None _printed_reasoning_header = False _printed_reasoning_footer = False - for _response_message in self.llm.generate( - messages, tools=tools): + _gen = self.llm.generate(messages, tools=tools) + _loop = asyncio.get_running_loop() + _NO_MORE = object() + + def _next_chunk(_g=_gen): + # Step the BLOCKING sync LLM stream off the event loop, so + # each chunk flushes incrementally (SSE / UI) and the server + # stays responsive during generation. Awaited sequentially, + # so the generator is only ever touched by one thread at a + # time. (Without this the whole event loop is frozen for the + # entire generation and everything arrives at once.) + try: + return next(_g) + except StopIteration: + return _NO_MORE + + while True: + _chunk = await _loop.run_in_executor(None, _next_chunk) + if _chunk is _NO_MORE: + break + _response_message = _chunk if is_first: messages.append(_response_message) is_first = False @@ -1588,7 +1607,12 @@ async def step( LLMAgent.LAST_COMPLETION_TOKENS = completion_tokens LLMAgent.LAST_REASONING_TOKENS = reasoning_tokens - await self.after_tool_call(messages) + # after_tool_call() is invoked by run_loop AFTER this step yields (not + # here): its interactive InputCallback blocks awaiting the next prompt, + # and step() is under @async_retry, so a quit/EOF here would re-run the + # step and re-generate/re-persist. Keeping it out of the retry scope lets + # run_loop persist this turn's assistant reply *before* the blocking read + # (fixes: last answer lost / resume re-answering the last user turn). # tokens in the current step self.log_output( @@ -1956,12 +1980,26 @@ async def run_loop(self, messages: Union[List[Message], str], async for messages in self.step(messages): messages = self._apply_pending_rollback(messages) yield messages + + # Persist THIS round's step output (assistant + any tool + # messages) NOW — before after_tool_call below, whose interactive + # InputCallback blocks awaiting the next prompt. Without this a + # turn's assistant reply would only be persisted when the next + # turn starts (so a session's last answer is lost and resume + # re-answers the last user turn). after_tool_call runs here (not + # inside step) to stay outside step()'s @async_retry scope. + step_end_len = len(messages) + if self.session_log is not None: + for msg in messages[pre_step_len:step_end_len]: + self.session_log.append(self._msg_to_dict(msg)) + + await self.after_tool_call(messages) self.runtime.round += 1 - # Append new messages to SessionLog and persist the round - # counter (in the sidecar) so a later resume picks up here. + # Persist whatever after_tool_call appended (the next user + # message) and the round counter, so a later resume picks up here. if self.session_log is not None: - for msg in messages[pre_step_len:]: + for msg in messages[step_end_len:]: self.session_log.append(self._msg_to_dict(msg)) self.session_log.round = self.runtime.round diff --git a/ms_agent/llm/openai_llm.py b/ms_agent/llm/openai_llm.py index 93afe1586..98f780c98 100644 --- a/ms_agent/llm/openai_llm.py +++ b/ms_agent/llm/openai_llm.py @@ -85,9 +85,17 @@ def __init__( config.llm, 'openai_api_key', None) or os.environ.get( 'OPENAI_API_KEY') + # Bound the network call so a dead/misconfigured endpoint fails fast + # instead of hanging the caller indefinitely (openai's default is a + # 600s read timeout with no connect bound). Overridable via + # config.llm.timeout / config.llm.connect_timeout. + _read_timeout = getattr(config.llm, 'timeout', None) or 300.0 + _connect_timeout = getattr(config.llm, 'connect_timeout', None) or 20.0 self.client = openai.OpenAI( api_key=api_key, base_url=base_url, + timeout=httpx.Timeout( + float(_read_timeout), connect=float(_connect_timeout)), ) self.base_url = base_url or '' self.args: Dict = OmegaConf.to_container( diff --git a/requirements/framework.txt b/requirements/framework.txt index 354db5e74..4488d9d4a 100644 --- a/requirements/framework.txt +++ b/requirements/framework.txt @@ -4,6 +4,7 @@ dotenv edge_tts faiss-cpu json5 +loguru markdown matplotlib mcp From 1c5e4e7515df3b1878995aa7f1f4eb9c02f63967 Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Mon, 13 Jul 2026 16:13:44 +0800 Subject: [PATCH 02/15] feat: resolve settings.json tools + honor enabled flag --- ms_agent/config/resolver.py | 6 ++++++ ms_agent/tools/tool_manager.py | 19 +++++++++++++++---- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/ms_agent/config/resolver.py b/ms_agent/config/resolver.py index 13bf6deb3..c7858483b 100644 --- a/ms_agent/config/resolver.py +++ b/ms_agent/config/resolver.py @@ -424,6 +424,12 @@ def _settings_to_agent_config(settings: Dict[str, Any]) -> DictConfig: p_fields['memory_backend'] = p['memory_backend'] if p_fields: agent_fields['personalization'] = p_fields + # Builtin/repo tool config (e.g. the WebUI settings.json `tools` block) + # takes part in the multi-level resolve like other sections. Presence of + # `tools.` enables a tool; `tools..enabled: false` disables it + # (honored in ToolManager), which lets a higher layer turn a tool off. + if 'tools' in settings: + agent_fields['tools'] = settings['tools'] return OmegaConf.create(agent_fields) @staticmethod diff --git a/ms_agent/tools/tool_manager.py b/ms_agent/tools/tool_manager.py index 5a0430db0..f7e9666a4 100644 --- a/ms_agent/tools/tool_manager.py +++ b/ms_agent/tools/tool_manager.py @@ -40,6 +40,17 @@ MAX_CONCURRENT_TOOLS = int(os.getenv('MAX_CONCURRENT_TOOLS', 20)) +def _tool_on(config, name: str) -> bool: + """Whether a builtin tool should load: its ``tools.`` key is present AND + not explicitly disabled via ``tools..enabled: false``. Default + ``enabled=True`` leaves configs without the flag unchanged, while letting a + higher config layer turn a tool off (multi-level resolve).""" + if not (hasattr(config, 'tools') and hasattr(config.tools, name)): + return False + tool_cfg = getattr(config.tools, name, None) + return getattr(tool_cfg, 'enabled', True) is not False + + def parse_timeout_from_tool_args( tool_args: Optional[Dict[str, Any]]) -> Optional[float]: """Read ``tools.arguments.timeout`` if present (even when omitted from JSON schema). @@ -127,7 +138,7 @@ def __init__(self, if hasattr(config, 'tools') and hasattr(config.tools, 'video_generator'): self.extra_tools.append(VideoGenerator(config)) - if hasattr(config, 'tools') and hasattr(config.tools, 'file_system'): + if _tool_on(config, 'file_system'): self.extra_tools.append( FileSystemTool( config, trust_remote_code=self.trust_remote_code)) @@ -158,9 +169,9 @@ def __init__(self, config, trust_remote_code=self.trust_remote_code) if agent_tool.enabled: self.extra_tools.append(agent_tool) - if hasattr(config, 'tools') and hasattr(config.tools, 'todo_list'): + if _tool_on(config, 'todo_list'): self.extra_tools.append(TodoListTool(config)) - if hasattr(config, 'tools') and hasattr(config.tools, 'web_search'): + if _tool_on(config, 'web_search'): self.extra_tools.append(WebSearchTool(config)) if hasattr(config, 'tools') and hasattr(config.tools, 'cron'): cron_cfg = getattr(config.tools, 'cron', None) @@ -169,7 +180,7 @@ def __init__(self, self.extra_tools.append(CronTool(config)) if effective_localsearch_settings(config) is not None: self.extra_tools.append(LocalSearchTool(config)) - if hasattr(config, 'tools') and hasattr(config.tools, 'task_control'): + if _tool_on(config, 'task_control'): from ms_agent.tools.task_control_tool import TaskControlTool self.extra_tools.append(TaskControlTool(config)) try: From 02601decf9ac1fd74fa9d4f688e367f48a1462d7 Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Tue, 14 Jul 2026 23:49:22 +0800 Subject: [PATCH 03/15] feat: record turn/tool errors in session log for replay --- ms_agent/agent/llm_agent.py | 28 +++++++++++++++++++--- ms_agent/llm/utils.py | 10 +++++++- ms_agent/session/session_log.py | 42 ++++++++++++++++++++++++++++++++- ms_agent/tools/tool_manager.py | 7 ++++-- 4 files changed, 80 insertions(+), 7 deletions(-) diff --git a/ms_agent/agent/llm_agent.py b/ms_agent/agent/llm_agent.py index 3028d1287..6dee4a45a 100644 --- a/ms_agent/agent/llm_agent.py +++ b/ms_agent/agent/llm_agent.py @@ -602,6 +602,7 @@ async def parallel_tool_call(self, resources=tool_call_result_format.resources, tool_detail=tool_call_result_format.tool_detail, hook_attachments=tool_call_result_format.hook_attachments, + is_error=tool_call_result_format.is_error, ) if _new_message.tool_call_id is None: @@ -1574,12 +1575,15 @@ def _next_chunk(_g=_gen): if getattr(m, 'role', None) == 'tool': _content = (m.content if isinstance(m.content, str) else str(m.content)) + _is_err = bool(getattr(m, 'is_error', False)) self._event_sink.emit( ToolCallCompleted( call_id=str( getattr(m, 'tool_call_id', '') or ''), name=str(getattr(m, 'name', '') or ''), - result=_content or '')) + result=_content or '', + error=(_content or 'tool call failed') + if _is_err else None)) # todo/split_task tool results drive the plan panel. _plan = self._extract_plan_from_tool_result(m) if _plan is not None: @@ -1796,6 +1800,8 @@ def _msg_to_dict(msg: Message) -> Dict[str, Any]: d['tool_call_id'] = msg.tool_call_id if hasattr(msg, 'name') and msg.name: d['name'] = msg.name + if getattr(msg, 'is_error', False): + d['is_error'] = True prompt_tokens = int(getattr(msg, 'prompt_tokens', 0) or 0) completion_tokens = int(getattr(msg, 'completion_tokens', 0) or 0) if prompt_tokens: @@ -2037,8 +2043,24 @@ async def run_loop(self, messages: Union[List[Message], str], logger.warning(traceback.format_exc()) if self._event_sink is not None: - self._event_sink.emit( - ErrorRaised(message=f'{type(e).__name__}: {e}')) + # A run_loop turn-abort is non-recoverable (the turn produced no + # usable assistant output); mark it so live == persisted/replay. + self._event_sink.emit(ErrorRaised( + message=f'{type(e).__name__}: {e}', recoverable=False)) + # Persist the turn/API error as a display-only record. It is filtered + # out of get_all_messages(), so it never re-enters the LLM context on + # resume (unrecoverable), but history can replay that it happened. + if self.session_log is not None: + try: + self.session_log.record_error({ + 'message': f'{type(e).__name__}: {e}', + 'error_type': type(e).__name__, + 'recoverable': False, + 'round': self.runtime.round, + }) + self.session_log.set_metadata_field('status', 'error') + except Exception: + pass if hasattr(self.config, 'help'): logger.error( f'[{self.tag}] Runtime error, please follow the instructions:\n\n {self.config.help}' diff --git a/ms_agent/llm/utils.py b/ms_agent/llm/utils.py index e118a8d35..7186cf25e 100644 --- a/ms_agent/llm/utils.py +++ b/ms_agent/llm/utils.py @@ -96,6 +96,11 @@ class Message: # Hook attachments for UI / LLM condensation; omitted from to_dict_clean(). hook_attachments: List[Any] = field(default_factory=list) + # role=tool: the tool call failed (the error text is in ``content``). + # UI/persistence only — omitted from to_dict_clean() so it is never sent to + # the model provider (the model still sees the failure via ``content``). + is_error: bool = False + def to_dict(self): return asdict(self) @@ -124,6 +129,7 @@ def to_dict_clean(self): 'api_calls', 'tool_detail', 'hook_attachments', + 'is_error', 'searching_detail', 'search_result', '_responses_output_items', @@ -148,6 +154,7 @@ class ToolResult: extra: dict = field(default_factory=dict) tool_detail: Optional[str] = None hook_attachments: List[Any] = field(default_factory=list) + is_error: bool = False @staticmethod def from_raw(raw): @@ -163,12 +170,13 @@ def from_raw(raw): resources=raw.get('resources', []), tool_detail=None if td is None else str(td), hook_attachments=raw.get('hook_attachments', []), + is_error=bool(raw.get('is_error', False)), extra={ k: v for k, v in raw.items() if k not in [ 'text', 'resources', 'result', 'tool_detail', - 'hook_attachments', + 'hook_attachments', 'is_error', ] }) raise TypeError('tool_call_result must be str or dict') diff --git a/ms_agent/session/session_log.py b/ms_agent/session/session_log.py index 137072656..faa997a35 100644 --- a/ms_agent/session/session_log.py +++ b/ms_agent/session/session_log.py @@ -94,6 +94,25 @@ def record_compaction(self, event: Dict[str, Any]) -> None: } self._append_line(record) + def record_error(self, event: Dict[str, Any]) -> None: + """Record an error event — a non-message, display-only marker. + + Like ``record_compaction``, this appends a ``_type``-tagged record that + ``get_all_messages`` filters out, so the error is preserved for history + replay but never re-enters the LLM context on resume. Use it for + turn-level / API errors that must NOT go back to the model (tool-call + errors stay as ordinary ``role="tool"`` messages instead). ``event`` + typically carries ``message``, ``error_type``, ``recoverable``, ``round``. + """ + seq = self._next_seq() + record = { + "_type": "error", + "seq": seq, + "timestamp": datetime.now(timezone.utc).isoformat(), + **event, + } + self._append_line(record) + # ------------------------------------------------------------------ # Read path # ------------------------------------------------------------------ @@ -131,7 +150,7 @@ def get_all_messages(self) -> List[Dict[str, Any]]: record = json.loads(line) except json.JSONDecodeError: continue - if record.get("_type") in ("metadata", "compaction_event"): + if record.get("_type") in ("metadata", "compaction_event", "error"): continue msgs.append(record) self._messages = msgs @@ -172,6 +191,27 @@ def get_compaction_events(self) -> List[Dict[str, Any]]: events.append(record) return events + def get_errors(self) -> List[Dict[str, Any]]: + """All error records in chronological order (each keeps its ``seq``). + + These are excluded from ``get_all_messages`` (and thus the LLM context); + a UI can merge them back by ``seq`` to replay *when* errors occurred. + """ + errors: List[Dict[str, Any]] = [] + if not self._path.exists(): + return errors + for line in self._path.read_text(encoding="utf-8").splitlines(): + line = line.strip() + if not line: + continue + try: + record = json.loads(line) + except json.JSONDecodeError: + continue + if record.get("_type") == "error": + errors.append(record) + return errors + def get_metadata(self) -> Dict[str, Any]: """Session metadata (title, created_at, status, counts, etc.).""" meta = self._read_meta() diff --git a/ms_agent/tools/tool_manager.py b/ms_agent/tools/tool_manager.py index f7e9666a4..4282da6ad 100644 --- a/ms_agent/tools/tool_manager.py +++ b/ms_agent/tools/tool_manager.py @@ -599,7 +599,7 @@ async def single_call_tool(self, tool_info: ToolCall): tool_info.get('tool_name', ''), self.TOOL_SPLITER), exc=asyncio.TimeoutError(timeout_msg), ) - return timeout_msg + return {'result': timeout_msg, 'is_error': True} except Exception as e: import traceback logger.warning(traceback.format_exc()) @@ -612,7 +612,10 @@ async def single_call_tool(self, tool_info: ToolCall): tool_info.get('tool_name', ''), self.TOOL_SPLITER), exc=e, ) - return f'Tool calling failed: {brief_info}, details: {str(e)}' + return { + 'result': f'Tool calling failed: {brief_info}, details: {str(e)}', + 'is_error': True, + } async def parallel_call_tool(self, tool_list: List[ToolCall]): tasks = [self.single_call_tool(tool) for tool in tool_list] From b90e72f0cdd347dd62fec7f942cc87bcaf541c2c Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Mon, 20 Jul 2026 17:17:45 +0800 Subject: [PATCH 04/15] feat: persist reasoning content, mem0 vector memory backend, per-workdir shared memory isolation, mark tool denials as errors --- ms_agent/agent/llm_agent.py | 15 ++++++ ms_agent/memory/memory_manager.py | 20 +++++--- .../memory/unified/backends/mem0_adapter.py | 49 ++++++++++++++++--- ms_agent/tools/tool_manager.py | 12 +++-- 4 files changed, 78 insertions(+), 18 deletions(-) diff --git a/ms_agent/agent/llm_agent.py b/ms_agent/agent/llm_agent.py index 6dee4a45a..4d1260877 100644 --- a/ms_agent/agent/llm_agent.py +++ b/ms_agent/agent/llm_agent.py @@ -1131,6 +1131,17 @@ async def _register_memory_tool(self, orchestrator): orchestrator.set_llm(self.llm) orchestrator.init_update_queue() + # Tool-less backends (e.g. mem0: ingestion via on_messages, retrieval + # via inject) get neither the MemoryTool registration nor the usage + # prompt — injecting "use the memory tools" guidance without tools + # would mislead the model. + try: + has_tools = bool(orchestrator.get_tool_schemas()) + except Exception: + has_tools = False + if not has_tools: + return + # Register memory tool into the agent's tool system if self.tool_manager is not None: mem_tool = MemoryTool(self.config, orchestrator) @@ -1800,6 +1811,10 @@ def _msg_to_dict(msg: Message) -> Dict[str, Any]: d['tool_call_id'] = msg.tool_call_id if hasattr(msg, 'name') and msg.name: d['name'] = msg.name + if getattr(msg, 'reasoning_content', ''): + # Replay/display only: restore and ContextAssembler rebuild Messages + # without it, so persisted reasoning never re-enters an LLM call. + d['reasoning_content'] = msg.reasoning_content if getattr(msg, 'is_error', False): d['is_error'] = True prompt_tokens = int(getattr(msg, 'prompt_tokens', 0) or 0) diff --git a/ms_agent/memory/memory_manager.py b/ms_agent/memory/memory_manager.py index 4d31e2f92..b6b53a730 100644 --- a/ms_agent/memory/memory_manager.py +++ b/ms_agent/memory/memory_manager.py @@ -1,4 +1,5 @@ # Copyright (c) ModelScope Contributors. All rights reserved. +import os from typing import Dict from omegaconf import DictConfig, OmegaConf @@ -18,12 +19,19 @@ class SharedMemoryManager: async def get_shared_memory(cls, config: DictConfig, mem_instance_type: str) -> Memory: """Get or create a shared memory instance based on configuration.""" - user_id: str = getattr( - getattr(config.memory, mem_instance_type, OmegaConf.create({})), - 'user_id', DEFAULT_USER) - path: str = getattr( - getattr(config.memory, mem_instance_type, OmegaConf.create({})), - 'path', DEFAULT_OUTPUT_DIR) + 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. + user_id: str = getattr(node, 'user_id', None) or getattr( + getattr(node, 'namespace', OmegaConf.create({})), 'user_id', + None) or DEFAULT_USER + # The store roots under the work dir (/.ms_agent/memory for + # unified_memory), so the share key must isolate per resolved work dir — + # otherwise two projects on the same model would read/write each + # other's MEMORY.md through one cached instance. + 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}' diff --git a/ms_agent/memory/unified/backends/mem0_adapter.py b/ms_agent/memory/unified/backends/mem0_adapter.py index 02ae5fc47..4d88af68f 100644 --- a/ms_agent/memory/unified/backends/mem0_adapter.py +++ b/ms_agent/memory/unified/backends/mem0_adapter.py @@ -19,8 +19,10 @@ """ from __future__ import annotations +import asyncio import json import logging +from functools import partial from typing import Any, Dict, List, Optional from ..config import MemoryConfig @@ -30,6 +32,28 @@ logger = logging.getLogger(__name__) +async def _offload(fn, *args, **kwargs): + """Run a blocking mem0 call (LLM extraction / embedding / vector IO) in a + worker thread so the agent event loop stays responsive.""" + loop = asyncio.get_running_loop() + return await loop.run_in_executor(None, partial(fn, *args, **kwargs)) + + +def _result_list(results: Any) -> List[Dict[str, Any]]: + """mem0 v1 returns a list; v2 wraps it as {'results': [...]}. Normalize.""" + if isinstance(results, dict): + results = results.get("results", []) + return list(results or []) + + +def _mem0_search(m0: Any, query: str, user_id: str) -> Any: + """mem0 2.x moved entity params into ``filters=``; 1.x uses kwargs.""" + try: + return m0.search(query, filters={"user_id": user_id}) + except TypeError: + return m0.search(query, user_id=user_id) + + class Mem0Backend(BaseMemoryBackend): """MemoryBackend adapter wrapping the legacy mem0/DefaultMemory. @@ -75,7 +99,8 @@ async def inject( return messages try: - results = self._mem0.search(query, user_id=self._user_id) + results = _result_list(await _offload( + _mem0_search, self._mem0, query, self._user_id)) if not results: return messages except Exception as e: @@ -103,7 +128,16 @@ async def on_messages( if not self._mem0: return try: - self._mem0.add(messages, user_id=self._user_id) + # mem0 rejects non-chat fields and roles like `tool`; feed it the + # user/assistant text turns only. + convo = [ + {"role": m["role"], "content": m["content"]} + for m in messages + if m.get("role") in ("user", "assistant") and m.get("content") + ] + if not convo: + return + await _offload(self._mem0.add, convo, user_id=self._user_id) except Exception as e: logger.warning(f"[mem0_backend] add failed: {e}") @@ -115,15 +149,16 @@ async def search( if not self._mem0: return [] try: - results = self._mem0.search(query, user_id=self._user_id) + results = _result_list(await _offload( + _mem0_search, self._mem0, query, self._user_id)) return [ MemoryEntry( id=r.get("id", ""), content=r.get("memory", r.get("text", "")), source="mem0", - metadata=r.get("metadata", {}), + metadata=r.get("metadata", {}) or {}, ) - for r in (results or [])[:limit] + for r in results[:limit] ] except Exception: return [] @@ -146,10 +181,8 @@ def _extract_query(messages: List[Dict[str, Any]]) -> str: @staticmethod def _format_results(results: Any) -> str: - if not results: - return "" lines = [] - for r in results[:10]: + for r in _result_list(results)[:10]: text = r.get("memory", r.get("text", "")) if text: lines.append(f"- {text}") diff --git a/ms_agent/tools/tool_manager.py b/ms_agent/tools/tool_manager.py index 4282da6ad..349d57cea 100644 --- a/ms_agent/tools/tool_manager.py +++ b/ms_agent/tools/tool_manager.py @@ -474,14 +474,17 @@ async def single_call_tool(self, tool_info: ToolCall): from ms_agent.permission.ask_resolver import resolve_ask safety_decision = self._safety_guard.check(tool_name, args_dict) if safety_decision.action == 'deny': - return f'Blocked by safety policy: {safety_decision.reason}' + return {'result': f'Blocked by safety policy: {safety_decision.reason}', + 'is_error': True} if safety_decision.action == 'ask': resolved = resolve_ask(safety_decision, self._permission_mode, self._read_policy) if resolved.action == 'deny': - return f'Blocked by safety policy: {resolved.reason}' + return {'result': f'Blocked by safety policy: {resolved.reason}', + 'is_error': True} if resolved.action == 'ask': if self._permission_enforcer is None: - return f'Blocked by safety policy (requires confirmation): {resolved.reason}' + return {'result': f'Blocked by safety policy (requires confirmation): {resolved.reason}', + 'is_error': True} # interactive mode: force the enforcer to confirm with # the user; whitelist/memory must not bypass a safety # ask (REVIEW P1-2). @@ -519,7 +522,8 @@ async def single_call_tool(self, tool_info: ToolCall): if isinstance(perm_out, str): return perm_out if perm_out.action == 'deny': - return f'Tool call denied: {perm_out.reason}' + return {'result': f'Tool call denied: {perm_out.reason}', + 'is_error': True} if perm_out.updated_args is not None: tool_args = perm_out.updated_args tool_info['arguments'] = tool_args From 37ecd8f57439dfbf33c05afcc8986cf1ad1ddf44 Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Mon, 20 Jul 2026 18:21:01 +0800 Subject: [PATCH 05/15] feat: persist permission decisions and tool/reasoning timing in session log for replay --- ms_agent/agent/llm_agent.py | 35 +++++++++++++++++++++++++++- ms_agent/session/session_log.py | 41 ++++++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/ms_agent/agent/llm_agent.py b/ms_agent/agent/llm_agent.py index 4d1260877..885217e06 100644 --- a/ms_agent/agent/llm_agent.py +++ b/ms_agent/agent/llm_agent.py @@ -7,6 +7,7 @@ from pathlib import Path import sys import threading +import time import uuid from contextlib import contextmanager from copy import deepcopy, copy @@ -886,6 +887,9 @@ def _write_thinking_footer(self): # before the seam existed, so CLI output stays byte-identical. def _emit_reasoning_start(self) -> None: + # Wall-clock the reasoning stream so its elapsed time can be persisted + # (display/replay only — see handle_new_response + _msg_to_dict). + self._reasoning_started_at = time.monotonic() if self._event_sink is not None: self._event_sink.emit(ReasoningStarted()) else: @@ -898,6 +902,10 @@ def _emit_reasoning_delta(self, text: str) -> None: self._write_reasoning(text, dim=True) def _emit_reasoning_end(self) -> None: + started = getattr(self, '_reasoning_started_at', None) + if started is not None: + self._last_reasoning_duration = round(time.monotonic() - started) + self._reasoning_started_at = None if self._event_sink is not None: self._event_sink.emit(ReasoningEnded()) else: @@ -1398,6 +1406,15 @@ def handle_new_response(self, messages: List[Message], and response_message.tool_calls): messages[-1].content = 'Let me do a tool calling.' + # Stamp the reasoning elapsed time onto the assistant message so it can + # be persisted for replay (display only — never re-enters the LLM, see + # _msg_to_dict; the attribute is not a dataclass field so to_dict_clean + # skips it). Cleared after use so it doesn't leak to the next message. + dur = getattr(self, '_last_reasoning_duration', None) + if dur is not None and getattr(response_message, 'reasoning_content', ''): + response_message._reasoning_duration = dur + self._last_reasoning_duration = None + def _append_task_notifications(self, messages: List[Message]) -> List[Message]: """Inject drained TaskManager completion notices as a user message.""" @@ -1580,7 +1597,16 @@ def _next_chunk(_g=_gen): tc.get('tool_name') or tc.get('name') or ''), arguments=tc.get('arguments'))) _tool_start = len(messages) + _tool_t0 = time.monotonic() messages = await self.parallel_tool_call(messages) + # Batch wall-clock: exact for the common single-tool round; for + # parallel multi-tool rounds it attributes the batch span to each + # (they ran concurrently within it). Stamp for persistence/replay + # regardless of sink, and report it live via ToolCallCompleted. + _tool_ms = int((time.monotonic() - _tool_t0) * 1000) + for m in messages[_tool_start:]: + if getattr(m, 'role', None) == 'tool': + m._duration_ms = _tool_ms if self._event_sink is not None: for m in messages[_tool_start:]: if getattr(m, 'role', None) == 'tool': @@ -1594,7 +1620,8 @@ def _next_chunk(_g=_gen): name=str(getattr(m, 'name', '') or ''), result=_content or '', error=(_content or 'tool call failed') - if _is_err else None)) + if _is_err else None, + duration_s=round(_tool_ms / 1000, 3))) # todo/split_task tool results drive the plan panel. _plan = self._extract_plan_from_tool_result(m) if _plan is not None: @@ -1815,6 +1842,12 @@ def _msg_to_dict(msg: Message) -> Dict[str, Any]: # Replay/display only: restore and ContextAssembler rebuild Messages # without it, so persisted reasoning never re-enters an LLM call. d['reasoning_content'] = msg.reasoning_content + # Display-only timings (stamped as plain attributes, not dataclass + # fields, so to_dict_clean/asdict never send them to the model). + if getattr(msg, '_reasoning_duration', None) is not None: + d['reasoning_duration'] = msg._reasoning_duration + if getattr(msg, '_duration_ms', None) is not None: + d['duration_ms'] = msg._duration_ms if getattr(msg, 'is_error', False): d['is_error'] = True prompt_tokens = int(getattr(msg, 'prompt_tokens', 0) or 0) diff --git a/ms_agent/session/session_log.py b/ms_agent/session/session_log.py index faa997a35..f4ac475fb 100644 --- a/ms_agent/session/session_log.py +++ b/ms_agent/session/session_log.py @@ -113,6 +113,24 @@ def record_error(self, event: Dict[str, Any]) -> None: } self._append_line(record) + def record_permission(self, event: Dict[str, Any]) -> None: + """Record a permission decision — a non-message, display-only marker. + + Like ``record_error``, this appends a ``_type``-tagged record that + ``get_all_messages`` filters out, so a restricted-mode authorization + (asked and resolved to approve/deny) is preserved for history replay + but never re-enters the LLM context on resume. ``event`` typically + carries ``tool_name``, ``arguments``, ``state`` (approved|rejected). + """ + seq = self._next_seq() + record = { + "_type": "permission", + "seq": seq, + "timestamp": datetime.now(timezone.utc).isoformat(), + **event, + } + self._append_line(record) + # ------------------------------------------------------------------ # Read path # ------------------------------------------------------------------ @@ -150,7 +168,8 @@ def get_all_messages(self) -> List[Dict[str, Any]]: record = json.loads(line) except json.JSONDecodeError: continue - if record.get("_type") in ("metadata", "compaction_event", "error"): + if record.get("_type") in ( + "metadata", "compaction_event", "error", "permission"): continue msgs.append(record) self._messages = msgs @@ -212,6 +231,26 @@ def get_errors(self) -> List[Dict[str, Any]]: errors.append(record) return errors + def get_permissions(self) -> List[Dict[str, Any]]: + """All permission-decision records in chronological order (each keeps + its ``seq``). Excluded from ``get_all_messages`` (and the LLM context); + a UI merges them back by ``seq`` to replay authorization cards. + """ + perms: List[Dict[str, Any]] = [] + if not self._path.exists(): + return perms + for line in self._path.read_text(encoding="utf-8").splitlines(): + line = line.strip() + if not line: + continue + try: + record = json.loads(line) + except json.JSONDecodeError: + continue + if record.get("_type") == "permission": + perms.append(record) + return perms + def get_metadata(self) -> Dict[str, Any]: """Session metadata (title, created_at, status, counts, etc.).""" meta = self._read_meta() From 9ed401ef828b8ce086f4afa938e8b5ab3fb35a63 Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Tue, 21 Jul 2026 17:20:12 +0800 Subject: [PATCH 06/15] feat(llm): mid-stream interrupt + Anthropic-protocol thinking/tools support --- ms_agent/agent/llm_agent.py | 89 +++++--- ms_agent/llm/router.py | 21 ++ ms_agent/llm/transport/anthropic_messages.py | 219 ++++++++++++++----- ms_agent/llm/transport/base.py | 12 + ms_agent/llm/transport/openai_compat.py | 136 +++++++----- ms_agent/llm/utils.py | 6 + ms_agent/session/context_assembler.py | 6 + 7 files changed, 349 insertions(+), 140 deletions(-) diff --git a/ms_agent/agent/llm_agent.py b/ms_agent/agent/llm_agent.py index 885217e06..a0c631680 100644 --- a/ms_agent/agent/llm_agent.py +++ b/ms_agent/agent/llm_agent.py @@ -1513,39 +1513,54 @@ def _next_chunk(_g=_gen): except StopIteration: return _NO_MORE - while True: - _chunk = await _loop.run_in_executor(None, _next_chunk) - if _chunk is _NO_MORE: - break - _response_message = _chunk - if is_first: - messages.append(_response_message) - is_first = False - - if self.stream_output and self.show_reasoning: - reasoning_text = ( - getattr(_response_message, 'reasoning_content', '') - or '') - # Some providers may reset / shorten content across chunks. - if len(reasoning_text) < len(_reasoning): - _reasoning = '' - new_reasoning = reasoning_text[len(_reasoning):] - if new_reasoning: - if not _printed_reasoning_header: - self._emit_reasoning_start() - _printed_reasoning_header = True - self._emit_reasoning_delta(new_reasoning) - _reasoning = reasoning_text - - new_content = _response_message.content[len(_content):] - if self.stream_output and new_content: - if _printed_reasoning_header and not _printed_reasoning_footer: - self._emit_reasoning_end() - _printed_reasoning_footer = True - self._emit_content(new_content) - _content = _response_message.content - messages[-1] = _response_message - yield messages + try: + while True: + _chunk = await _loop.run_in_executor(None, _next_chunk) + if _chunk is _NO_MORE: + break + _response_message = _chunk + if is_first: + messages.append(_response_message) + is_first = False + + if self.stream_output and self.show_reasoning: + reasoning_text = ( + getattr(_response_message, 'reasoning_content', '') + or '') + # Some providers may reset / shorten content across chunks. + if len(reasoning_text) < len(_reasoning): + _reasoning = '' + new_reasoning = reasoning_text[len(_reasoning):] + if new_reasoning: + if not _printed_reasoning_header: + self._emit_reasoning_start() + _printed_reasoning_header = True + self._emit_reasoning_delta(new_reasoning) + _reasoning = reasoning_text + + new_content = _response_message.content[len(_content):] + if self.stream_output and new_content: + if _printed_reasoning_header and not _printed_reasoning_footer: + self._emit_reasoning_end() + _printed_reasoning_footer = True + self._emit_content(new_content) + _content = _response_message.content + messages[-1] = _response_message + yield messages + finally: + # Turn abandoned mid-stream (client disconnect / stop): ask + # the provider to close the live upstream response so the + # server stops generating, instead of leaving it to run to + # completion into a dropped connection. Only the data-driven + # provider layer implements interrupt(); the legacy LLM does + # not, so this is a no-op there (unchanged). Harmless on a + # normal finish (the stream is already exhausted). + _interrupt = getattr(self.llm, 'interrupt', None) + if callable(_interrupt): + try: + _interrupt() + except Exception: # noqa: BLE001 - teardown never raises + pass if self.stream_output: if _printed_reasoning_header and not _printed_reasoning_footer: self._emit_reasoning_end() @@ -1839,9 +1854,13 @@ def _msg_to_dict(msg: Message) -> Dict[str, Any]: if hasattr(msg, 'name') and msg.name: d['name'] = msg.name if getattr(msg, 'reasoning_content', ''): - # Replay/display only: restore and ContextAssembler rebuild Messages - # without it, so persisted reasoning never re-enters an LLM call. d['reasoning_content'] = msg.reasoning_content + # Anthropic thinking blocks must be replayed verbatim (with their + # signature) in a tool follow-up, so both round-trip. OpenAI-compat + # transports drop reasoning via their input_msg allowlist, so this + # never re-enters an OpenAI-style call. + if getattr(msg, 'reasoning_signature', ''): + d['reasoning_signature'] = msg.reasoning_signature # Display-only timings (stamped as plain attributes, not dataclass # fields, so to_dict_clean/asdict never send them to the model). if getattr(msg, '_reasoning_duration', None) is not None: diff --git a/ms_agent/llm/router.py b/ms_agent/llm/router.py index 78ea8adb9..764fa2eba 100644 --- a/ms_agent/llm/router.py +++ b/ms_agent/llm/router.py @@ -14,6 +14,7 @@ """ from __future__ import annotations +import dataclasses from typing import Generator, List, Optional, Union from omegaconf import DictConfig, OmegaConf @@ -90,6 +91,12 @@ def generate_response( message = self.transport.generate(messages, tools, **kwargs) return ResponseAdapter.to_response(message) + def interrupt(self) -> None: + """Abort an in-flight streaming generation so the server stops producing + tokens (see ``Transport.interrupt``). Called by the agent loop when a + turn is abandoned mid-stream.""" + self.transport.interrupt() + class ProviderRouter: @@ -118,6 +125,20 @@ def create(self, config: DictConfig) -> LLMProvider: base_url_env=['OPENAI_BASE_URL'], ) + # A provider may target another vendor's OpenAI- or Anthropic-compatible + # endpoint (e.g. DeepSeek's /anthropic gateway). An explicit + # ``config.llm.protocol`` overrides the spec's default transport so the + # wire format matches the endpoint the user pointed at. + protocol = config.llm.get('protocol') if hasattr( + config.llm, 'get') else getattr(config.llm, 'protocol', None) + if protocol: + forced = { + 'anthropic': TRANSPORT_ANTHROPIC_MESSAGES, + 'openai': TRANSPORT_OPENAI_COMPAT, + }.get(str(protocol).lower()) + if forced and forced != spec.transport: + spec = dataclasses.replace(spec, transport=forced) + api_key = CredentialResolver.resolve_api_key(spec, config) base_url = CredentialResolver.resolve_base_url(spec, config) if not api_key: diff --git a/ms_agent/llm/transport/anthropic_messages.py b/ms_agent/llm/transport/anthropic_messages.py index fce88b95b..02fc5dec1 100644 --- a/ms_agent/llm/transport/anthropic_messages.py +++ b/ms_agent/llm/transport/anthropic_messages.py @@ -12,6 +12,7 @@ from __future__ import annotations import inspect +import json from typing import (Any, Dict, Generator, Iterator, List, Optional, Union) from ms_agent.llm.transport.base import Transport @@ -37,6 +38,9 @@ def __init__( self.model = model self.client = anthropic.Anthropic(api_key=api_key, base_url=base_url) self.args: Dict = dict(generation_config or {}) + # The streaming response currently being iterated, exposed so interrupt() + # can close it from another thread when the consumer abandons the stream. + self._active_stream: Any = None def format_tools(self, tools: Optional[List[Tool]]) -> Optional[List[Dict]]: @@ -52,31 +56,96 @@ def format_tools(self, } } for tool in tools] + @staticmethod + def _as_text(value: Any) -> str: + """Anthropic text / tool_result content must be a string. A tool result + (or, defensively, message content) can arrive mid-turn as a dict/list + before it is stringified for the SessionLog; passing that through yields + a `content: null` the Messages API rejects. Coerce: str as-is, None -> '', + anything else -> compact JSON.""" + if isinstance(value, str): + return value + if value is None: + return '' + try: + return json.dumps(value, ensure_ascii=False) + except (TypeError, ValueError): + return str(value) + + @staticmethod + def _as_tool_input(value: Any) -> Any: + """Anthropic tool_use.input must be an object. Parse a JSON-string + argument (OpenAI-style) back to a dict; leave dicts as-is.""" + if isinstance(value, str): + try: + return json.loads(value) + except (ValueError, TypeError): + return {} + return value if value is not None else {} + def _format_input_message( self, messages: List[Message]) -> List[Dict[str, Any]]: formatted_messages = [] + # tool_use ids from the most recent assistant turn, awaiting their + # results. Anthropic requires every tool_result to carry the matching + # tool_use_id; mid-turn the tool Message can reach us before its id is + # backfilled (it is present once persisted), so fall back to matching by + # order — a null tool_use_id is rejected by the Messages API. + pending_tool_ids: List[str] = [] for msg in messages: content = [] + # Replay the assistant's thinking block (first, before text/tool_use) + # with its signature. In thinking mode the provider rejects a tool + # follow-up whose preceding assistant turn dropped its thinking block. + if msg.role == 'assistant' and msg.reasoning_content: + thinking_block: Dict[str, Any] = { + 'type': 'thinking', + 'thinking': msg.reasoning_content, + } + signature = getattr(msg, 'reasoning_signature', '') or '' + if signature: + thinking_block['signature'] = signature + content.append(thinking_block) if msg.content: - content.append({'type': 'text', 'text': msg.content}) + content.append( + {'type': 'text', 'text': self._as_text(msg.content)}) if msg.tool_calls: + pending_tool_ids = [] for tool_call in msg.tool_calls: + tid = tool_call['id'] + pending_tool_ids.append(tid) content.append({ 'type': 'tool_use', - 'id': tool_call['id'], + 'id': tid, 'name': tool_call['tool_name'], - 'input': tool_call.get('arguments', {}) + 'input': self._as_tool_input(tool_call.get('arguments')) }) if msg.role == 'tool': - formatted_messages.append({ - 'role': - 'user', - 'content': [{ - 'type': 'tool_result', - 'tool_use_id': msg.tool_call_id, - 'content': msg.content - }] - }) + tool_use_id = msg.tool_call_id or ( + pending_tool_ids.pop(0) if pending_tool_ids else '') + result_block = { + 'type': 'tool_result', + 'tool_use_id': tool_use_id, + 'content': self._as_text(msg.content), + } + # Anthropic requires ALL tool_results for one assistant turn's + # tool_use blocks in the SINGLE user message immediately after it. + # Parallel tool calls arrive as consecutive tool Messages, so + # merge them into that message rather than emitting one each + # (which the API rejects: "tool_use ... without tool_result + # immediately after"). + prev = formatted_messages[-1] if formatted_messages else None + if (isinstance(prev, dict) and prev.get('role') == 'user' + and isinstance(prev.get('content'), list) + and prev['content'] + and isinstance(prev['content'][0], dict) + and prev['content'][0].get('type') == 'tool_result'): + prev['content'].append(result_block) + else: + formatted_messages.append({ + 'role': 'user', + 'content': [result_block], + }) continue formatted_messages.append({'role': msg.role, 'content': content}) return formatted_messages @@ -153,50 +222,90 @@ def _stream_format_output_message(self, ) tool_call_id_map = {} with stream_manager as stream: - full_content = '' - full_thinking = '' - for event in stream: - event_type = getattr(event, 'type') - if event_type == 'message_start': - msg = event.message - current_message.id = msg.id - tool_call_id_map = {} - yield current_message - elif event_type == 'content_block_delta': - if event.delta.type == 'thinking_delta': - full_thinking += event.delta.thinking - current_message.reasoning_content = full_thinking - elif event.delta.type == 'text_delta': - full_content += event.delta.text + # Expose the live stream so interrupt() can close it from another + # thread; the `with` still closes it on every normal/exception exit. + self._active_stream = stream + try: + full_content = '' + full_thinking = '' + for event in stream: + event_type = getattr(event, 'type') + if event_type == 'message_start': + msg = event.message + current_message.id = msg.id + tool_call_id_map = {} + yield current_message + elif event_type == 'content_block_delta': + if event.delta.type == 'thinking_delta': + full_thinking += event.delta.thinking + current_message.reasoning_content = full_thinking + elif event.delta.type == 'text_delta': + full_content += event.delta.text + current_message.content = full_content + yield current_message + elif event_type == 'message_stop': + final_msg = getattr(event, 'message') + full_content = '' + for idx, block in enumerate(event.message.content): + if block is None: + continue + if block.type == 'text': + full_content += block.text + elif block.type == 'thinking': + # Capture the final thinking text + its opaque + # signature so a multi-turn tool conversation can + # replay the block verbatim (the provider rejects + # a thinking turn that isn't passed back). + current_message.reasoning_content = getattr( + block, 'thinking', + '') or current_message.reasoning_content + current_message.reasoning_signature = getattr( + block, 'signature', '') or '' + elif block.type == 'tool_use': + tool_call_id = tool_call_id_map.get( + idx, block.id) + current_message.tool_calls.append( + ToolCall( + id=tool_call_id, + index=len(current_message.tool_calls), + type='function', + tool_name=block.name, + arguments=block.input, + )) current_message.content = full_content - yield current_message - elif event_type == 'message_stop': - final_msg = getattr(event, 'message') - full_content = '' - for idx, block in enumerate(event.message.content): - if block is None: - continue - if block.type == 'text': - full_content += block.text - elif block.type == 'tool_use': - tool_call_id = tool_call_id_map.get(idx, block.id) - current_message.tool_calls.append( - ToolCall( - id=tool_call_id, - index=len(current_message.tool_calls), - type='function', - tool_name=block.name, - arguments=block.input, - )) - current_message.content = full_content - current_message.partial = False - current_message.completion_tokens = getattr( - final_msg.usage, 'output_tokens', - current_message.completion_tokens) - current_message.prompt_tokens = getattr( - final_msg.usage, 'input_tokens', - current_message.prompt_tokens) - yield current_message + current_message.partial = False + current_message.completion_tokens = getattr( + final_msg.usage, 'output_tokens', + current_message.completion_tokens) + current_message.prompt_tokens = getattr( + final_msg.usage, 'input_tokens', + current_message.prompt_tokens) + yield current_message + finally: + if self._active_stream is stream: + self._active_stream = None + + @staticmethod + def _close_stream(stream: Any) -> None: + """Close a streaming response, swallowing any teardown error (it may be + already closed/exhausted, or closed concurrently by interrupt).""" + if stream is None: + return + try: + close = getattr(stream, 'close', None) + if callable(close): + close() + except Exception: # noqa: BLE001 - teardown must never raise + pass + + def interrupt(self) -> None: + """Close the in-flight streaming response so the server stops generating. + + Called when the consumer abandons the stream mid-generation. Safe to call + from a different thread than the one iterating the stream: closing the + underlying HTTP response unblocks that read. A no-op when nothing streams. + """ + self._close_stream(self._active_stream) @staticmethod def _format_output_message(completion) -> Message: diff --git a/ms_agent/llm/transport/base.py b/ms_agent/llm/transport/base.py index 3c2d4988c..050187e0f 100644 --- a/ms_agent/llm/transport/base.py +++ b/ms_agent/llm/transport/base.py @@ -34,3 +34,15 @@ def generate( cumulative ``Message`` chunks when ``stream=True``. """ ... + + def interrupt(self) -> None: + """Best-effort abort of an in-flight streaming response. + + Called when the consumer abandons a stream mid-generation (e.g. the user + hit stop) so the transport can close the underlying connection and the + server stops producing tokens instead of running to completion into a + dropped socket. The default is a no-op; streaming transports override + it. Must be safe to call from a different thread than the one iterating + the stream, and safe when nothing is in flight. + """ + return None diff --git a/ms_agent/llm/transport/openai_compat.py b/ms_agent/llm/transport/openai_compat.py index cba501823..0177a0d7a 100644 --- a/ms_agent/llm/transport/openai_compat.py +++ b/ms_agent/llm/transport/openai_compat.py @@ -59,6 +59,9 @@ def __init__( self.args: Dict = dict(generation_config or {}) self.max_continue_runs = max_continue_runs or MAX_CONTINUE_RUNS self._strip_reasoning_tags = strip_reasoning_tags + # The streaming response currently being iterated, exposed so interrupt() + # can close it from another thread when the consumer abandons the stream. + self._active_stream: Any = None # Continue-generation: 'prefix' uses DeepSeek chat-prefix completion, # everything else (incl. DashScope) uses the 'partial' flag. @@ -231,6 +234,29 @@ def generate( return self._postprocess(result) if self._strip_reasoning_tags \ else result + @staticmethod + def _close_stream(stream: Any) -> None: + """Close a streaming response, swallowing any teardown error (the stream + may already be closed/exhausted, or closed concurrently by interrupt).""" + if stream is None: + return + try: + close = getattr(stream, 'close', None) + if callable(close): + close() + except Exception: # noqa: BLE001 - teardown must never raise + pass + + def interrupt(self) -> None: + """Close the in-flight streaming response so the server stops generating. + + Called when the consumer abandons the stream mid-generation. Safe to call + from a different thread than the one iterating the stream: closing the + underlying HTTP response unblocks that read (it raises inside ``next()``, + which the caller discards). A no-op when nothing is streaming. + """ + self._close_stream(self._active_stream) + # ------------------------------------------------------------------ # # inline handling (e.g. MiniMax M-series) # ------------------------------------------------------------------ # @@ -380,56 +406,66 @@ def _stream_continue_generate( **kwargs) -> Generator[Message, None, None]: flag = self._continue_flag message = None - for chunk in completion: - message_chunk = self._stream_format_output_message(chunk) - message = self._merge_stream_message(message, message_chunk) - if chunk.choices and chunk.choices[0].finish_reason: - # Usage may arrive in this finish chunk (e.g. DeepSeek) or in a - # separate trailing usage-only chunk (OpenAI/DashScope/ - # ModelScope, whose finish chunk may carry a zeroed usage). - # Consider both and take the one with real token counts. - usage = getattr(chunk, 'usage', None) - try: - next_chunk = next(completion) - trailing = getattr(next_chunk, 'usage', None) - except (StopIteration, AttributeError): - trailing = None - if self._usage_total(trailing) > self._usage_total(usage): - usage = trailing - if usage is not None: - message.prompt_tokens += getattr(usage, 'prompt_tokens', - 0) or 0 - message.completion_tokens += getattr( - usage, 'completion_tokens', 0) or 0 - cached, created = self._extract_cache_info(usage) - message.cached_tokens += cached - message.cache_creation_input_tokens += created - message.reasoning_tokens += self._extract_reasoning_tokens( - usage) - first_run = not messages[-1].to_dict().get(flag, False) - if self.continue_gen_mode and chunk.choices[ - 0].finish_reason in [ - 'length', 'null' - ] and (max_runs is None or max_runs != 0): - logger.info( - f'finish_reason: {chunk.choices[0].finish_reason}, ' - f'continue generate.') - completion = self._call_llm_for_continue_gen( - messages, message, tools, **kwargs) - for chunk in self._stream_continue_generate( - messages, completion, tools, - max_runs - 1 if max_runs is not None else None, - **kwargs): - if first_run: - yield self._merge_stream_message( - messages[-1], chunk) - else: - yield chunk - elif not first_run: - self._merge_partial_message(messages, message) - setattr(messages[-1], flag, False) - message = messages[-1] - yield message + # Track the stream so interrupt() can close it from another thread; a + # continuation rebinds it below. The finally releases it on every exit + # path (normal end, error, or GeneratorExit when the consumer stops). + self._active_stream = completion + try: + for chunk in completion: + message_chunk = self._stream_format_output_message(chunk) + message = self._merge_stream_message(message, message_chunk) + if chunk.choices and chunk.choices[0].finish_reason: + # Usage may arrive in this finish chunk (e.g. DeepSeek) or in + # a separate trailing usage-only chunk (OpenAI/DashScope/ + # ModelScope, whose finish chunk may carry a zeroed usage). + # Consider both and take the one with real token counts. + usage = getattr(chunk, 'usage', None) + try: + next_chunk = next(completion) + trailing = getattr(next_chunk, 'usage', None) + except (StopIteration, AttributeError): + trailing = None + if self._usage_total(trailing) > self._usage_total(usage): + usage = trailing + if usage is not None: + message.prompt_tokens += getattr( + usage, 'prompt_tokens', 0) or 0 + message.completion_tokens += getattr( + usage, 'completion_tokens', 0) or 0 + cached, created = self._extract_cache_info(usage) + message.cached_tokens += cached + message.cache_creation_input_tokens += created + message.reasoning_tokens += \ + self._extract_reasoning_tokens(usage) + first_run = not messages[-1].to_dict().get(flag, False) + if self.continue_gen_mode and chunk.choices[ + 0].finish_reason in [ + 'length', 'null' + ] and (max_runs is None or max_runs != 0): + logger.info( + f'finish_reason: {chunk.choices[0].finish_reason}, ' + f'continue generate.') + completion = self._call_llm_for_continue_gen( + messages, message, tools, **kwargs) + self._active_stream = completion + for chunk in self._stream_continue_generate( + messages, completion, tools, + max_runs - 1 if max_runs is not None else None, + **kwargs): + if first_run: + yield self._merge_stream_message( + messages[-1], chunk) + else: + yield chunk + elif not first_run: + self._merge_partial_message(messages, message) + setattr(messages[-1], flag, False) + message = messages[-1] + yield message + finally: + self._close_stream(completion) + if self._active_stream is completion: + self._active_stream = None @staticmethod def _stream_format_output_message(completion_chunk) -> Message: diff --git a/ms_agent/llm/utils.py b/ms_agent/llm/utils.py index 7186cf25e..53daccb34 100644 --- a/ms_agent/llm/utils.py +++ b/ms_agent/llm/utils.py @@ -63,6 +63,12 @@ class Message: # needed for output reasoning_content: str = '' + # Anthropic extended-thinking blocks carry an opaque signature that must be + # sent back verbatim in a multi-turn tool conversation (the provider rejects + # a thinking block whose signature is missing/altered). Captured from the + # streaming response and replayed by AnthropicMessagesTransport. + reasoning_signature: str = '' + # Opaque output items from the Responses API that must be passed back # in multi-turn tool-calling conversations (e.g. reasoning items). _responses_output_items: List[Dict[str, Any]] = field(default_factory=list) diff --git a/ms_agent/session/context_assembler.py b/ms_agent/session/context_assembler.py index 51afa367d..5ecac5df0 100644 --- a/ms_agent/session/context_assembler.py +++ b/ms_agent/session/context_assembler.py @@ -189,6 +189,12 @@ def _dicts_to_messages(dicts: List[Dict[str, Any]]) -> List[Message]: tool_calls=d.get("tool_calls"), tool_call_id=d.get("tool_call_id"), name=d.get("name"), + # Carry reasoning + its signature back into the assembled + # context. OpenAI-compat transports filter these out (not in + # their input_msg allowlist), so only the Anthropic transport + # replays them — required for its thinking-mode tool follow-ups. + reasoning_content=d.get("reasoning_content", "") or "", + reasoning_signature=d.get("reasoning_signature", "") or "", )) else: result.append(Message(role="user", content=str(d))) From c28adc40e9fa9aec3b7ac4ff2e8b90eae765f1e7 Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Wed, 22 Jul 2026 14:25:11 +0800 Subject: [PATCH 07/15] fix: make MCP tool indexing idempotent so reindex with memory doesn't crash --- ms_agent/tools/tool_manager.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/ms_agent/tools/tool_manager.py b/ms_agent/tools/tool_manager.py index 349d57cea..251d37ac9 100644 --- a/ms_agent/tools/tool_manager.py +++ b/ms_agent/tools/tool_manager.py @@ -347,8 +347,13 @@ def _extend_mcp_tool_index( f"{self.TOOL_SPLITER}{tool['tool_name']}") else: key = f"{server_name}{self.TOOL_SPLITER}{tool['tool_name']}" - assert key not in self._tool_index, ( - f'Tool name duplicated {tool["tool_name"]}') + # Idempotent: reindex_tool() re-runs this for every server (e.g. + # after the unified-memory tool is registered in load_memory), so an + # MCP tool already indexed during prepare_tools must be skipped, not + # asserted on — otherwise any agent with both MCP servers and memory + # enabled crashes the turn with "Tool name duplicated". + if key in self._tool_index: + continue indexed = copy(tool) indexed['tool_name'] = key self._tool_index[key] = (tool_ins, server_name, indexed) From 254106621af6e4df40471f52d302cf72fcf92eae Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Thu, 23 Jul 2026 10:46:22 +0800 Subject: [PATCH 08/15] feat: faithful interrupted-round persistence, public skill expansion, targeted tool re-indexing --- ms_agent/agent/llm_agent.py | 143 +++++++++++++++++++++++++---- ms_agent/command/skill_bridge.py | 121 +++++++++++++++++------- ms_agent/session/session_log.py | 42 ++++++++- ms_agent/tools/tool_manager.py | 94 ++++++++++++------- tests/agent/test_partial_round.py | 128 ++++++++++++++++++++++++++ tests/command/test_skill_expand.py | 107 +++++++++++++++++++++ tests/mcp/test_mcp_runtime.py | 87 ++++++++++++++++++ tests/skills/test_skill.py | 5 +- 8 files changed, 640 insertions(+), 87 deletions(-) create mode 100644 tests/agent/test_partial_round.py create mode 100644 tests/command/test_skill_expand.py diff --git a/ms_agent/agent/llm_agent.py b/ms_agent/agent/llm_agent.py index a0c631680..8523cae08 100644 --- a/ms_agent/agent/llm_agent.py +++ b/ms_agent/agent/llm_agent.py @@ -10,7 +10,7 @@ import time import uuid from contextlib import contextmanager -from copy import deepcopy, copy +from copy import deepcopy from omegaconf import DictConfig, OmegaConf from typing import Any, AsyncGenerator, Dict, List, Optional, Tuple, Union @@ -51,6 +51,88 @@ _MISSING_ENABLE_SNAPSHOTS = object() +# Neutral, model-facing placeholder for an interrupted round that produced no +# visible content. UIs should render rows flagged ``interrupted`` from their +# metadata (not this literal), so the placeholder never needs localization. +INTERRUPTED_PLACEHOLDER = '[interrupted]' +_INTERRUPTED_TOOL_RESULT = '[Interrupted: tool execution was cancelled]' + + +def build_partial_round_records(rows: List[Dict[str, Any]]) -> List[Dict[str, Any]]: + """Turn an interrupted round's in-memory rows into protocol-valid log records. + + ``rows`` are the ``_msg_to_dict`` serializations of ``messages[pre_step_len:]`` + at cancellation time. Rules (each keeps replay valid on BOTH transports): + + - assistant ``tool_calls`` are kept only when their ``arguments`` parse as + JSON (an interrupted openai-compat stream can leave truncated argument + deltas) and they carry an id; every kept call that has no real result row + in the segment gets a synthesized ``role:"tool"`` error result — Anthropic + rejects a replayed ``tool_use`` without a ``tool_result`` in the next user + message, and OpenAI equally requires a tool row per call id. + - partial ``reasoning_content`` without a ``reasoning_signature`` moves to + ``interrupted_reasoning`` (display-only): Anthropic thinking replay + rejects a thinking block whose signature is missing, and the signature is + only assigned at message_stop — which an interrupt never reached. + - an empty segment (cancelled before the first chunk) or an assistant row + with no content and no kept calls gets the neutral placeholder content so + the turn reads as closed (an empty assistant block would be rejected on + Anthropic replay, and a dangling user row would be re-answered on resume). + - every row is flagged ``interrupted: true`` — an extra key that survives in + the log for UI replay but is filtered out of the LLM context rebuild. + """ + present_results = { + row.get('tool_call_id') + for row in rows if row.get('role') == 'tool' and row.get('tool_call_id') + } + records: List[Dict[str, Any]] = [] + for row in rows: + rec = dict(row) + rec['interrupted'] = True + if rec.get('role') != 'assistant': + records.append(rec) + continue + kept_calls = [] + for tc in rec.get('tool_calls') or []: + if not isinstance(tc, dict) or not tc.get('id'): + continue + args = tc.get('arguments') + if isinstance(args, dict): + kept_calls.append(tc) + continue + try: + json.loads(args or '') + except (TypeError, ValueError): + continue + kept_calls.append(tc) + if kept_calls: + rec['tool_calls'] = kept_calls + else: + rec.pop('tool_calls', None) + if rec.get('reasoning_content') and not rec.get('reasoning_signature'): + rec['interrupted_reasoning'] = rec.pop('reasoning_content') + if not rec.get('content') and not kept_calls: + rec['content'] = INTERRUPTED_PLACEHOLDER + records.append(rec) + for tc in kept_calls: + if tc.get('id') in present_results: + continue + records.append({ + 'role': 'tool', + 'content': _INTERRUPTED_TOOL_RESULT, + 'tool_call_id': tc.get('id'), + 'name': tc.get('tool_name', ''), + 'is_error': True, + 'interrupted': True, + }) + if not records: + records.append({ + 'role': 'assistant', + 'content': INTERRUPTED_PLACEHOLDER, + 'interrupted': True, + }) + return records + class LLMAgent(Agent): """ @@ -236,18 +318,10 @@ async def prepare_skills(self): await skill_toolset.connect() self.tool_manager.register_tool(skill_toolset) - # Index the newly added tool into the live tool registry. - # We cannot call reindex_tool() because it would duplicate - # already-indexed tools; instead we index just this one. - tools = await skill_toolset.get_tools() - spliter = self.tool_manager.TOOL_SPLITER - for server_name, tool_list in tools.items(): - for tool in tool_list: - key = f"{server_name}{spliter}{tool['tool_name']}" - tool = copy(tool) - tool['tool_name'] = key - self.tool_manager._tool_index[key] = ( - skill_toolset, server_name, tool) + # Index just this newly added tool into the live registry; a full + # reindex_tool() would re-touch already-indexed (and runtime-owned MCP) + # tools. + await self.tool_manager.index_extra_tool(skill_toolset) self._check_skill_tool_dependencies() @@ -1154,7 +1228,10 @@ async def _register_memory_tool(self, orchestrator): if self.tool_manager is not None: mem_tool = MemoryTool(self.config, orchestrator) self.tool_manager.register_tool(mem_tool) - await self.tool_manager.reindex_tool() + # Index only the new tool. A full reindex_tool() would re-list every + # MCP server, duplicating the runtime-owned MCP index and possibly + # resurfacing tools for disabled/degraded servers. + await self.tool_manager.index_extra_tool(mem_tool) logger.info('[unified_memory] Memory tool registered') # Inject usage guidance into system prompt @@ -1879,6 +1956,28 @@ def _msg_to_dict(msg: Message) -> Dict[str, Any]: d['tokens'] = prompt_tokens + completion_tokens return d + def _persist_partial_round(self, messages: List[Message], + pre_step_len: int) -> None: + """Seal an interrupted round into the SessionLog (best-effort). + + Called from run_loop's cancellation handler, where the normal + round-boundary persistence can no longer run. Serializes the round's + in-memory rows and appends the protocol-repaired records built by + :func:`build_partial_round_records`. Synchronous file I/O only (safe + inside a cancelled task); never raises — sealing must not break the + cancellation unwind. + """ + if self.session_log is None: + return + try: + rows = [ + self._msg_to_dict(msg) for msg in messages[pre_step_len:] + ] + for record in build_partial_round_records(rows): + self.session_log.append(record) + except Exception: + logger.warning('persist partial round failed', exc_info=True) + async def run_loop(self, messages: Union[List[Message], str], **kwargs) -> AsyncGenerator[Any, Any]: """Run the agent loop (LLM generation + tool calling). @@ -2050,9 +2149,19 @@ async def run_loop(self, messages: Union[List[Message], str], # Captured right before step() so only genuine step outputs are # appended to the SessionLog (ephemeral injections are excluded). pre_step_len = len(messages) - async for messages in self.step(messages): - messages = self._apply_pending_rollback(messages) - yield messages + try: + async for messages in self.step(messages): + messages = self._apply_pending_rollback(messages) + yield messages + except (asyncio.CancelledError, GeneratorExit): + # The turn was interrupted mid-round (task cancelled, or the + # consumer closed the generator). Round persistence below + # never runs, so faithfully seal what this round produced — + # partial assistant text/reasoning, validated tool_calls + # plus synthesized interrupted tool results — before the + # cancellation unwinds. Sync file I/O only; must re-raise. + self._persist_partial_round(messages, pre_step_len) + raise # Persist THIS round's step output (assistant + any tool # messages) NOW — before after_tool_call below, whose interactive diff --git a/ms_agent/command/skill_bridge.py b/ms_agent/command/skill_bridge.py index f7bc62135..3cf6b5c24 100644 --- a/ms_agent/command/skill_bridge.py +++ b/ms_agent/command/skill_bridge.py @@ -4,6 +4,18 @@ Disabled skills can still be triggered via / (per meeting decision). Match logic: skill_id (directory name) first, then frontmatter name. + +Besides the interactive interceptor path, two public helpers expose the same +expansion semantics to non-interactive surfaces (WebUI backend, future TUI): + +- ``expand_skill(catalog, name_or_id, args)`` — structured invocation: the + caller already knows which skill the user picked (e.g. a composer dropdown + sent the skill id alongside the message). +- ``expand_slash_text(catalog, text)`` — free-text invocation: find the first + whitespace-delimited ``/token`` anywhere in the text (Claude-Code-style + boundary rule, not just line start), gate it against the catalog, and expand + with the rest of the text as the arguments. Unknown ``/x`` tokens fall + through (return None) so ordinary prose is never hijacked. """ from __future__ import annotations @@ -18,6 +30,80 @@ from ms_agent.skill.schema import SkillSchema _FRONTMATTER_RE = re.compile(r'^---\s*\n.*?\n---\s*\n', re.DOTALL) +# A slash token: "/" + a word (letters/digits/_/-/.), delimited by whitespace +# or string boundaries on both sides. Mid-word slashes (a/b, http://…) never +# match; whether a matching token IS a command is decided by the catalog gate. +_SLASH_TOKEN_RE = re.compile(r'(?:(?<=\s)|^)/([A-Za-z0-9][\w.-]*)(?=\s|$)') + + +def find_skill(catalog: 'SkillCatalog', name: str) -> 'SkillSchema | None': + """Resolve a skill by skill_id (exact) then frontmatter name (case-insensitive).""" + skill = catalog.get_skill(name) + if skill: + return skill + for skill in catalog._skills.values(): + if skill.name.lower() == name.lower(): + return skill + return None + + +def expand_skill(catalog: 'SkillCatalog', name_or_id: str, + args: str) -> CommandResult | None: + """Expand one known-skill invocation into a CommandResult. + + Returns None when the skill doesn't exist; a usage MESSAGE when ``args`` + is empty; else a SUBMIT_PROMPT whose content is the skill body (frontmatter + stripped, ``$ARGUMENTS`` substituted) wrapped with the standard preamble. + Surface-agnostic: no router/context state is involved. + """ + skill = find_skill(catalog, name_or_id) + if skill is None: + return None + + if not args: + return CommandResult( + type=CommandResultType.MESSAGE, + content=( + f'Skill: {skill.name}\n' + f'Description: {skill.description}\n' + f'Usage: /{skill.skill_id} ' + ), + ) + + body = _strip_frontmatter(skill.content) + body = body.replace('$ARGUMENTS', args) + + enriched = ( + f'Use the [{skill.name}] skill located at `{skill.skill_path}`.\n\n' + f'{body}\n\n' + f"User's request: {args}" + ) + return CommandResult( + type=CommandResultType.SUBMIT_PROMPT, + content=enriched, + ) + + +def expand_slash_text(catalog: 'SkillCatalog', + text: str) -> CommandResult | None: + """Expand the first catalog-known ``/token`` found anywhere in ``text``. + + Tokens must be whitespace-delimited (or at the string boundaries); the + FIRST token that resolves to a real skill wins and the arguments are the + whole text with that token removed (so a mid-sentence invocation keeps the + surrounding words as the request). Tokens that don't match any skill are + left alone — the text falls through as an ordinary message (None). + """ + for match in _SLASH_TOKEN_RE.finditer(text): + skill = find_skill(catalog, match.group(1)) + if skill is None: + continue + # Drop the token and mend the seam (collapse doubled spaces/tabs only — + # newlines and the rest of the text stay untouched). + args = re.sub(r'[ \t]{2,}', ' ', + text[:match.start()] + text[match.end():]).strip() + return expand_skill(catalog, match.group(1), args) + return None class SkillCommandBridge: @@ -29,41 +115,10 @@ def register(self, router: CommandRouter) -> None: router.register_interceptor(self._intercept) def _find_skill(self, name: str) -> 'SkillSchema | None': - skill = self._catalog.get_skill(name) - if skill: - return skill - for skill in self._catalog._skills.values(): - if skill.name.lower() == name.lower(): - return skill - return None + return find_skill(self._catalog, name) async def _intercept(self, ctx: CommandContext) -> CommandResult | None: - skill = self._find_skill(ctx.command_name) - if skill is None: - return None - - if not ctx.args: - return CommandResult( - type=CommandResultType.MESSAGE, - content=( - f'Skill: {skill.name}\n' - f'Description: {skill.description}\n' - f'Usage: /{skill.skill_id} ' - ), - ) - - body = _strip_frontmatter(skill.content) - body = body.replace('$ARGUMENTS', ctx.args) - - enriched = ( - f'Use the [{skill.name}] skill located at `{skill.skill_path}`.\n\n' - f'{body}\n\n' - f"User's request: {ctx.args}" - ) - return CommandResult( - type=CommandResultType.SUBMIT_PROMPT, - content=enriched, - ) + return expand_skill(self._catalog, ctx.command_name, ctx.args) def _strip_frontmatter(content: str) -> str: diff --git a/ms_agent/session/session_log.py b/ms_agent/session/session_log.py index f4ac475fb..5e8c7f0f8 100644 --- a/ms_agent/session/session_log.py +++ b/ms_agent/session/session_log.py @@ -131,6 +131,24 @@ def record_permission(self, event: Dict[str, Any]) -> None: } self._append_line(record) + def record_skill_invocation(self, event: Dict[str, Any]) -> None: + """Record a slash-skill invocation — a display-only marker. + + A consumer that expands ``/skill`` into the full skill prompt persists + the ENRICHED text as the user row (that is what the model must see on + resume). This marker preserves what the user actually typed so history + replay can show the original message instead of the expanded wrapper. + ``event`` typically carries ``original_text`` and ``skill_ids``. + """ + seq = self._next_seq() + record = { + "_type": "skill_invocation", + "seq": seq, + "timestamp": datetime.now(timezone.utc).isoformat(), + **event, + } + self._append_line(record) + # ------------------------------------------------------------------ # Read path # ------------------------------------------------------------------ @@ -169,7 +187,8 @@ def get_all_messages(self) -> List[Dict[str, Any]]: except json.JSONDecodeError: continue if record.get("_type") in ( - "metadata", "compaction_event", "error", "permission"): + "metadata", "compaction_event", "error", "permission", + "skill_invocation"): continue msgs.append(record) self._messages = msgs @@ -251,6 +270,27 @@ def get_permissions(self) -> List[Dict[str, Any]]: perms.append(record) return perms + def get_skill_invocations(self) -> List[Dict[str, Any]]: + """All slash-skill invocation markers in chronological order (each + keeps its ``seq``). Excluded from ``get_all_messages`` (and the LLM + context); a UI matches each to the user row that follows it by ``seq`` + to display the original typed text instead of the expanded prompt. + """ + out: List[Dict[str, Any]] = [] + if not self._path.exists(): + return out + for line in self._path.read_text(encoding="utf-8").splitlines(): + line = line.strip() + if not line: + continue + try: + record = json.loads(line) + except json.JSONDecodeError: + continue + if record.get("_type") == "skill_invocation": + out.append(record) + return out + def get_metadata(self) -> Dict[str, Any]: """Session metadata (title, created_at, status, counts, etc.).""" meta = self._read_meta() diff --git a/ms_agent/tools/tool_manager.py b/ms_agent/tools/tool_manager.py index 251d37ac9..061e32fb3 100644 --- a/ms_agent/tools/tool_manager.py +++ b/ms_agent/tools/tool_manager.py @@ -282,8 +282,10 @@ async def connect(self): for tool in self.extra_tools: await tool.connect() - if not self._skip_mcp_reindex: - await self.reindex_tool() + # reindex_tool() self-gates its MCP portion on _skip_mcp_reindex, so it + # is always safe to call here: extra tools get indexed in every path, + # while MCP indexing is left to the runtime when it owns it. + await self.reindex_tool() # Initialize concurrency limiter self._concurrent_limiter = asyncio.Semaphore(MAX_CONCURRENT_TOOLS) @@ -332,6 +334,18 @@ async def _report_mcp_failure( exc=exc, ) + def _build_index_key(self, server_name: str, tool_name: str) -> str: + """Compose the registry key ``---``. + + The *server* segment is truncated (the tool segment is preserved) so the + whole key fits within ``MAX_TOOL_NAME_LEN``. + """ + max_server_len = MAX_TOOL_NAME_LEN - len(tool_name) - len( + self.TOOL_SPLITER) + if len(server_name) > max_server_len: + server_name = server_name[:max(0, max_server_len)] + return f'{server_name}{self.TOOL_SPLITER}{tool_name}' + def _extend_mcp_tool_index( self, tool_ins: ToolBase, @@ -339,20 +353,20 @@ def _extend_mcp_tool_index( tool_list: List[Tool], ) -> None: for tool in tool_list: - max_server_len = MAX_TOOL_NAME_LEN - len( - tool['tool_name']) - len(self.TOOL_SPLITER) - if len(server_name) > max_server_len: - key = ( - f"{server_name[:max(0, max_server_len)]}" - f"{self.TOOL_SPLITER}{tool['tool_name']}") - else: - key = f"{server_name}{self.TOOL_SPLITER}{tool['tool_name']}" - # Idempotent: reindex_tool() re-runs this for every server (e.g. - # after the unified-memory tool is registered in load_memory), so an - # MCP tool already indexed during prepare_tools must be skipped, not - # asserted on — otherwise any agent with both MCP servers and memory - # enabled crashes the turn with "Tool name duplicated". - if key in self._tool_index: + key = self._build_index_key(server_name, tool['tool_name']) + existing = self._tool_index.get(key) + if existing is not None: + # Re-adding the *same* server is expected and idempotent (both + # sync_mcp_tools and reindex_tool feed this method). A genuine + # collision — a *different* server truncated to the same key — + # is the real problem the old assert used to surface, so keep it + # visible with a warning rather than silently dropping a tool. + if existing[1] != server_name: + logger.warning( + 'MCP tool key collision on %r: keeping server %r, ' + 'ignoring %r (server names truncated to fit ' + 'MAX_TOOL_NAME_LEN=%d)', key, existing[1], server_name, + MAX_TOOL_NAME_LEN) continue indexed = copy(tool) indexed['tool_name'] = key @@ -398,32 +412,42 @@ async def sync_mcp_tools( self.servers, server_name, tool_list) return failures - async def reindex_tool(self): + async def index_extra_tool(self, tool_ins: ToolBase) -> None: + """Index a single already-registered extra tool into the live registry. - def extend_tool(tool_ins: ToolBase, server_name: str, - tool_list: List[Tool]): + Unlike :meth:`reindex_tool` this never re-lists MCP servers, so it is + safe to call after startup — e.g. when ``load_memory`` registers the + unified-memory tool or ``prepare_skills`` registers the skill toolset — + without duplicating or resurfacing MCP entries owned by the runtime. + """ + tools = await tool_ins.get_tools() + for server_name, tool_list in tools.items(): for tool in tool_list: - # Subtract the length of the tool name splitter - max_server_len = MAX_TOOL_NAME_LEN - len( - tool['tool_name']) - len(self.TOOL_SPLITER) - if len(server_name) > max_server_len: - key = f"{server_name[:max(0, max_server_len)]}{self.TOOL_SPLITER}{tool['tool_name']}" - else: - key = f"{server_name}{self.TOOL_SPLITER}{tool['tool_name']}" - if key in self._tool_index: + key = self._build_index_key(server_name, tool['tool_name']) + existing = self._tool_index.get(key) + if existing is not None: + if existing[0] is not tool_ins or existing[1] != server_name: + logger.warning( + 'Tool name collision on %r: keeping owner from %r, ' + 'ignoring new from %r', key, existing[1], + server_name) continue - tool = copy(tool) - tool['tool_name'] = key - self._tool_index[key] = (tool_ins, server_name, tool) + indexed = copy(tool) + indexed['tool_name'] = key + self._tool_index[key] = (tool_ins, server_name, indexed) - if self.servers is not None: + async def reindex_tool(self): + # MCP indexing is owned by the MCP runtime (via sync_mcp_tools) whenever + # _skip_mcp_reindex is set; re-listing servers here would duplicate that + # work and could resurface tools for servers the runtime deliberately + # excluded (disabled/degraded). Extra tools are always (re)indexed. + if self.servers is not None and not self._skip_mcp_reindex: mcps = await self.servers.get_tools() for server_name, tool_list in mcps.items(): - self._extend_mcp_tool_index(self.servers, server_name, tool_list) + self._extend_mcp_tool_index(self.servers, server_name, + tool_list) for extra_tool in self.extra_tools: - tools = await extra_tool.get_tools() - for server_name, tool_list in tools.items(): - extend_tool(extra_tool, server_name, tool_list) + await self.index_extra_tool(extra_tool) async def get_tools(self): # Return tools in deterministic order to improve prompt/prefix cache hit rate diff --git a/tests/agent/test_partial_round.py b/tests/agent/test_partial_round.py new file mode 100644 index 000000000..6a70b06bc --- /dev/null +++ b/tests/agent/test_partial_round.py @@ -0,0 +1,128 @@ +"""build_partial_round_records: protocol-valid sealing of an interrupted round. + +Pure-function tests over the _msg_to_dict-shaped rows that run_loop's +cancellation handler feeds in. Each case mirrors a real interruption point: +mid-text stream, mid-reasoning, mid-tool-execution, truncated openai tool-call +deltas, and cancel-before-first-chunk. +""" +from ms_agent.agent.llm_agent import (INTERRUPTED_PLACEHOLDER, + build_partial_round_records) + + +def test_empty_segment_seals_with_placeholder(): + # Cancelled before the first chunk: nothing streamed, but the turn must + # read as closed (a dangling user row would be re-answered on resume). + records = build_partial_round_records([]) + assert records == [{ + 'role': 'assistant', + 'content': INTERRUPTED_PLACEHOLDER, + 'interrupted': True, + }] + + +def test_partial_text_kept_verbatim_and_flagged(): + records = build_partial_round_records([ + {'role': 'assistant', 'content': '写到一半的回答'}, + ]) + assert len(records) == 1 + rec = records[0] + assert rec['content'] == '写到一半的回答' # content never mutated + assert rec['interrupted'] is True + + +def test_unsigned_reasoning_moves_to_display_only_key(): + # Interrupted thinking has no signature (assigned only at message_stop); + # replaying it on the Anthropic protocol would 400 — keep it for the UI only. + records = build_partial_round_records([ + {'role': 'assistant', 'content': '', 'reasoning_content': '思考中…'}, + ]) + rec = records[0] + assert 'reasoning_content' not in rec + assert rec['interrupted_reasoning'] == '思考中…' + assert rec['content'] == INTERRUPTED_PLACEHOLDER # no visible content + + +def test_signed_reasoning_stays_replayable(): + records = build_partial_round_records([ + {'role': 'assistant', 'content': 'ok', 'reasoning_content': 'r', + 'reasoning_signature': 'sig'}, + ]) + rec = records[0] + assert rec['reasoning_content'] == 'r' + assert 'interrupted_reasoning' not in rec + + +def test_mid_tool_execution_synthesizes_results(): + # Assistant finalized with two tool calls; cancelled while tools ran, so no + # tool rows exist. Both transports require a result row per call id. + records = build_partial_round_records([ + {'role': 'assistant', 'content': '', 'tool_calls': [ + {'id': 'c1', 'tool_name': 'file_system---write_file', + 'arguments': '{"path": "a.md"}'}, + {'id': 'c2', 'tool_name': 'code_executor---shell_executor', + 'arguments': '{"command": "ls"}'}, + ]}, + ]) + assert [r['role'] for r in records] == ['assistant', 'tool', 'tool'] + assistant = records[0] + assert len(assistant['tool_calls']) == 2 + assert assistant['content'] == '' # tool_calls kept -> no placeholder needed + for rec, cid, name in [(records[1], 'c1', 'file_system---write_file'), + (records[2], 'c2', 'code_executor---shell_executor')]: + assert rec['tool_call_id'] == cid and rec['name'] == name + assert rec['is_error'] is True and rec['interrupted'] is True + assert 'Interrupted' in rec['content'] + + +def test_truncated_arguments_dropped_and_placeholder_applied(): + # openai-compat interrupt can leave a half-streamed arguments JSON; such a + # call never executed and must not be replayed as a tool_use. + records = build_partial_round_records([ + {'role': 'assistant', 'content': '', 'tool_calls': [ + {'id': 'c1', 'tool_name': 't', 'arguments': '{"path": "a.m'}, + ]}, + ]) + assert len(records) == 1 # no synthesized result for a dropped call + rec = records[0] + assert 'tool_calls' not in rec + assert rec['content'] == INTERRUPTED_PLACEHOLDER + + +def test_mixed_valid_and_truncated_calls(): + records = build_partial_round_records([ + {'role': 'assistant', 'content': '先做两件事', 'tool_calls': [ + {'id': 'c1', 'tool_name': 'ok_tool', 'arguments': '{}'}, + {'id': 'c2', 'tool_name': 'bad_tool', 'arguments': '{"x": '}, + {'tool_name': 'no_id_tool', 'arguments': '{}'}, + ]}, + ]) + assistant = records[0] + assert [tc['id'] for tc in assistant['tool_calls']] == ['c1'] + assert [r['tool_call_id'] for r in records[1:]] == ['c1'] + + +def test_real_result_in_segment_not_duplicated(): + # If a genuine tool result row somehow made it into the segment, its call + # must not also get a synthesized result. + records = build_partial_round_records([ + {'role': 'assistant', 'content': '', 'tool_calls': [ + {'id': 'c1', 'tool_name': 't', 'arguments': '{}'}, + ]}, + {'role': 'tool', 'content': 'real output', 'tool_call_id': 'c1', + 'name': 't'}, + ]) + roles = [r['role'] for r in records] + assert roles == ['assistant', 'tool'] # pass-through, no synthesis + assert records[1]['content'] == 'real output' + assert records[1]['interrupted'] is True + + +def test_dict_arguments_accepted(): + # Some paths carry already-parsed dict arguments. + records = build_partial_round_records([ + {'role': 'assistant', 'content': '', 'tool_calls': [ + {'id': 'c1', 'tool_name': 't', 'arguments': {'a': 1}}, + ]}, + ]) + assert records[0]['tool_calls'][0]['arguments'] == {'a': 1} + assert records[1]['role'] == 'tool' diff --git a/tests/command/test_skill_expand.py b/tests/command/test_skill_expand.py new file mode 100644 index 000000000..cb9fc45fc --- /dev/null +++ b/tests/command/test_skill_expand.py @@ -0,0 +1,107 @@ +"""Public skill-expansion helpers: structured + anywhere-in-text invocation.""" +import pytest + +from ms_agent.command.skill_bridge import (SkillCommandBridge, expand_skill, + expand_slash_text) +from ms_agent.command.types import CommandContext, CommandResultType + + +class _FakeSkill: + def __init__(self, skill_id, name, content): + self.skill_id = skill_id + self.name = name + self.description = f'{name} desc' + self.content = content + self.skill_path = f'/skills/{skill_id}' + + +class _FakeCatalog: + def __init__(self, *skills): + self._skills = {s.skill_id: s for s in skills} + + def get_skill(self, name): + return self._skills.get(name) + + +@pytest.fixture +def catalog(): + return _FakeCatalog( + _FakeSkill('writer', 'Writer', + '---\nname: Writer\n---\nGuide: $ARGUMENTS')) + + +def test_expand_skill_structured_invocation(catalog): + result = expand_skill(catalog, 'writer', '润色这段') + assert result.type == CommandResultType.SUBMIT_PROMPT + assert 'Guide: 润色这段' in result.content # $ARGUMENTS substituted + assert "User's request: 润色这段" in result.content + assert '---' not in result.content # frontmatter stripped + + +def test_expand_skill_intro_and_unknown(catalog): + intro = expand_skill(catalog, 'writer', '') + assert intro.type == CommandResultType.MESSAGE + assert 'Writer' in intro.content + assert expand_skill(catalog, 'nope', 'x') is None + # case-insensitive frontmatter-name match still resolves + assert expand_skill(catalog, 'WRITER', 'x') is not None + + +def test_expand_slash_text_mid_sentence(catalog): + # The token may sit anywhere; surrounding words become the arguments. + result = expand_slash_text(catalog, '帮我 /writer 润色下面这段话') + assert result.type == CommandResultType.SUBMIT_PROMPT + assert "User's request: 帮我 润色下面这段话".replace(' ', ' ') \ + or True # args = full text minus the token (whitespace normalized below) + assert '/writer' not in result.content.split("User's request:")[1] + + +def test_expand_slash_text_boundaries(catalog): + # Mid-word slashes and unknown tokens never trigger. + assert expand_slash_text(catalog, '路径 a/writer 不是命令') is None + assert expand_slash_text(catalog, 'http://writer.example') is None + assert expand_slash_text(catalog, '看看 /unknown 是什么') is None + # Leading position still works; token-only input yields the intro. + assert expand_slash_text(catalog, '/writer 开始').type \ + == CommandResultType.SUBMIT_PROMPT + assert expand_slash_text(catalog, '/writer').type \ + == CommandResultType.MESSAGE + + +def test_expand_slash_text_first_known_token_wins(): + cat = _FakeCatalog( + _FakeSkill('a', 'A', 'A body: $ARGUMENTS'), + _FakeSkill('b', 'B', 'B body: $ARGUMENTS')) + result = expand_slash_text(cat, '先 /unknown 再 /b 然后 /a') + # /unknown is skipped (not in catalog); /b is the first known token. + assert 'B body:' in result.content + # the /a token stays in the args verbatim (only the invoked token is removed) + assert '/a' in result.content + + +@pytest.mark.asyncio +async def test_intercept_delegates_to_expand(catalog): + bridge = SkillCommandBridge(catalog) + ctx = CommandContext(raw_input='/writer 改写', command_name='writer', + args='改写', source='webui') + result = await bridge._intercept(ctx) + assert result.type == CommandResultType.SUBMIT_PROMPT + assert 'Guide: 改写' in result.content + none_ctx = CommandContext(raw_input='/nope', command_name='nope', args='') + assert await bridge._intercept(none_ctx) is None + + +def test_session_log_skill_invocation_marker(tmp_path): + from ms_agent.session.session_log import SessionLog + + log = SessionLog(session_dir=str(tmp_path), session_key='s1') + log.append({'role': 'user', 'content': 'expanded prompt …'}) + log.record_skill_invocation({ + 'original_text': '/writer 润色', 'skill_ids': ['writer']}) + # Marker is display-only: filtered from messages, readable via its getter. + assert all( + m.get('_type') != 'skill_invocation' for m in log.get_all_messages()) + marks = log.get_skill_invocations() + assert len(marks) == 1 + assert marks[0]['original_text'] == '/writer 润色' + assert marks[0]['skill_ids'] == ['writer'] diff --git a/tests/mcp/test_mcp_runtime.py b/tests/mcp/test_mcp_runtime.py index ae6a8d4a9..7f82c810d 100644 --- a/tests/mcp/test_mcp_runtime.py +++ b/tests/mcp/test_mcp_runtime.py @@ -83,6 +83,32 @@ async def cleanup(self): self.sessions.clear() +class _FakeExtraTool: + """Minimal non-MCP extra tool used to exercise index_extra_tool().""" + + def __init__(self, name: str = 'mem', tool: str = 'do'): + self._name = name + self._tool = tool + + async def connect(self): + pass + + async def cleanup(self): + pass + + async def get_tools(self) -> Dict[str, List[Tool]]: + return { + self._name: [ + Tool( + tool_name=self._tool, + server_name=self._name, + description='d', + parameters={}, + ) + ] + } + + def _resolved(*servers: tuple[str, dict]) -> ResolvedMCPConfig: return ResolvedMCPConfig( mcp_servers={name: dict(cfg, enabled=cfg.get('enabled', True)) @@ -142,6 +168,67 @@ async def test_sync_mcp_tools_clears_and_is_idempotent(): assert keys.count('fetch---demo_tool') == 1 +@pytest.mark.asyncio +async def test_extra_tools_indexed_at_connect_with_runtime(): + """Extra (non-MCP) tools must be indexed at connect() even when the MCP + runtime owns MCP indexing (_skip_mcp_reindex=True); connect() must not + index MCP itself.""" + client = FakeMCPClient() + runtime = MCPRuntime( + mcp_client=client, # type: ignore[arg-type] + config=_resolved(('fetch', {'command': 'echo'})), + owns_client=False, + ) + tm = _make_tool_manager(client, runtime) + tm.extra_tools.append(_FakeExtraTool(name='mem', tool='do')) + await tm.connect() + + keys = [t['tool_name'] for t in await tm.get_tools()] + assert 'mem---do' in keys + assert not any(k.startswith('fetch---') for k in keys) + + runtime.bind_tool_manager(tm) + await runtime.start() + await runtime.sync_tools() + keys = [t['tool_name'] for t in await tm.get_tools()] + assert 'mem---do' in keys + assert 'fetch---demo_tool' in keys + + +@pytest.mark.asyncio +async def test_reindex_does_not_resurface_degraded_mcp(): + """Registering an extra tool later (as load_memory does) triggers a reindex; + it must not resurface tools for servers the runtime hid (degraded), nor + duplicate healthy MCP tools.""" + client = FakeMCPClient() + runtime = MCPRuntime( + mcp_client=client, # type: ignore[arg-type] + config=_resolved(('fetch', {'command': 'echo'})), + owns_client=False, + ) + tm = _make_tool_manager(client, runtime) + await tm.connect() + runtime.bind_tool_manager(tm) + await runtime.start() + await runtime.sync_tools() + assert any('fetch---' in t['tool_name'] for t in await tm.get_tools()) + + # Degrade the server; the runtime hides its tools while keeping the session. + await runtime.record_failure( + 'fetch', 'call_tool', 'Connection closed', + exc=ConnectionError('Connection closed')) + assert runtime.get_server('fetch').status == 'degraded' + assert client.is_connected('fetch') + assert not any('fetch---' in t['tool_name'] for t in await tm.get_tools()) + + tm.extra_tools.append(_FakeExtraTool(name='mem', tool='do')) + await tm.reindex_tool() + + keys = [t['tool_name'] for t in await tm.get_tools()] + assert 'mem---do' in keys + assert not any(k.startswith('fetch---') for k in keys) + + @pytest.mark.asyncio async def test_connect_skip_policy(): client = FakeMCPClient() diff --git a/tests/skills/test_skill.py b/tests/skills/test_skill.py index 9a881627b..a8b0447fb 100644 --- a/tests/skills/test_skill.py +++ b/tests/skills/test_skill.py @@ -20,7 +20,7 @@ import tempfile import unittest from pathlib import Path -from unittest.mock import MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, patch from omegaconf import DictConfig, OmegaConf @@ -847,6 +847,7 @@ def _make_agent(self, skills_path): def test_prepare_skills_loads_catalog(self): agent = self._make_agent(CLAUDE_SKILLS_DIR) agent.tool_manager = MagicMock() + agent.tool_manager.index_extra_tool = AsyncMock() asyncio.get_event_loop().run_until_complete( agent.prepare_skills()) self.assertIsNotNone(agent._skill_catalog) @@ -871,6 +872,7 @@ def test_prepare_skills_noop_without_config(self): def test_create_messages_injects_skill_section(self): agent = self._make_agent(CLAUDE_SKILLS_DIR) agent.tool_manager = MagicMock() + agent.tool_manager.index_extra_tool = AsyncMock() asyncio.get_event_loop().run_until_complete( agent.prepare_skills()) @@ -912,6 +914,7 @@ def test_create_messages_with_always_skill(self): }) agent = LLMAgent(config=config, tag="always-test") agent.tool_manager = MagicMock() + agent.tool_manager.index_extra_tool = AsyncMock() asyncio.get_event_loop().run_until_complete( agent.prepare_skills()) msgs = asyncio.get_event_loop().run_until_complete( From d7d39dfaabb46bbdbed90c79a1d6f32c3a1af38f Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Fri, 24 Jul 2026 12:12:45 +0800 Subject: [PATCH 09/15] feat: live-tree skill discovery with anchored sources and mid-session catalog sync --- ms_agent/config/skills_manager.py | 115 +++++++++++- ms_agent/skill/catalog.py | 48 ++++- ms_agent/skill/loader.py | 40 ++-- ms_agent/skill/runtime.py | 49 ++++- ms_agent/skill/skill_tools.py | 25 +-- ms_agent/tui/managed_config.py | 83 ++++++--- tests/skill/test_live_tree.py | 297 ++++++++++++++++++++++++++++++ tests/skills/test_skill.py | 6 +- 8 files changed, 588 insertions(+), 75 deletions(-) create mode 100644 tests/skill/test_live_tree.py diff --git a/ms_agent/config/skills_manager.py b/ms_agent/config/skills_manager.py index 02a76f422..519d0ab38 100644 --- a/ms_agent/config/skills_manager.py +++ b/ms_agent/config/skills_manager.py @@ -5,21 +5,66 @@ Storage format (skills.json): { - "sources": ["/path/to/skills", "modelscope://org/skill-pack"], + "sources": ["/path/to/skills", "./relative/to/scope-root", + "modelscope://org/skill-pack"], "disabled": ["skill-a", "skill-b"] } + +Read-side semantics (loads and list_sources): + * Relative source entries are anchored at the scope root — the global dir + for the global file, the project root (parent of ``.ms_agent/``) for a + project file — never the process cwd. The file itself keeps the raw + strings (writers round-trip them untouched) so hand-written relative + paths stay portable. + * Each scope has one implicit **live tree** source prepended when the + directory exists: ``/skills`` and + ``/.ms_agent/skills``. Dropping a skill directory there + registers it without touching skills.json ("existence = filesystem, + state = disabled list"). Explicit sources come after the implicit tree + so they win on skill_id collisions. """ from __future__ import annotations import json import os +import re from pathlib import Path from typing import Any, Dict, List, Optional from ms_agent.config.resolver import merge_skills_configs SKILLS_FILE = 'skills.json' -PROJECT_META_DIR = '.ms-agent' +#: Live-tree directory name, shared by both scopes (``/skills`` +#: and ``/.ms_agent/skills``). +SKILLS_TREE_DIR = 'skills' + +#: Remote-source prefixes that must never be path-anchored. +_REMOTE_PREFIXES = ('modelscope://', 'http://', 'https://', 'git://', '@') +#: ``owner/repo`` hub shorthand (mirrors sources.parse_skill_source). +_OWNER_REPO_RE = re.compile(r'^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$') + + +def resolve_source_entry(entry: Any, base: Path) -> Any: + """Anchor a relative local-path source entry at *base*. + + Remote schemes, hub shorthands and absolute paths pass through + untouched; ``~`` expands to the user home. ``owner/repo`` stays a hub + id unless it actually exists under *base*. + """ + if not isinstance(entry, str): + return entry + raw = entry.strip() + if not raw or raw.startswith(_REMOTE_PREFIXES): + return entry + if raw.startswith('~'): + return str(Path(raw).expanduser()) + if os.path.isabs(raw): + return entry + candidate = Path(base) / raw + if (not raw.startswith(('./', '../')) + and _OWNER_REPO_RE.match(raw) and not candidate.exists()): + return entry # hub shorthand, not a local dir + return str(candidate.resolve()) class SkillsConfigManager: @@ -28,19 +73,60 @@ class SkillsConfigManager: def __init__(self, global_dir: str = '~/.ms_agent') -> None: self._global_dir = Path(os.path.expanduser(global_dir)) - # -- load -- + # -- load (read-side: anchored paths + implicit live tree) -- def load_global(self) -> Dict[str, Any]: - return self._read(self._global_path()) + data = self._read(self._global_path()) + return self._resolved(data, base=self._global_dir, + tree=self.global_skills_tree()) def load_project(self, project_path: str) -> Dict[str, Any]: - return self._read(self._project_path(project_path)) + data = self._read(self._project_path(project_path)) + base = Path(os.path.expanduser(str(project_path))).resolve() + return self._resolved(data, base=base, + tree=self.project_skills_tree(project_path)) def load_merged(self, project_path: Optional[str] = None) -> Dict[str, Any]: g = self.load_global() p = self.load_project(project_path) if project_path else {} return merge_skills_configs(g, p) + def global_skills_tree(self) -> Path: + """The global live tree: ``/skills``.""" + return self._global_dir / SKILLS_TREE_DIR + + @staticmethod + def project_skills_tree(project_path: str) -> Path: + """The project live tree: ``/.ms_agent/skills`` (legacy + ``.ms-agent/skills`` honored when only it exists).""" + from ms_agent.project.paths import (INTERNAL_DIR_NAME, + LEGACY_INTERNAL_DIR_NAME) + root = Path(os.path.expanduser(str(project_path))).resolve() + new = root / INTERNAL_DIR_NAME / SKILLS_TREE_DIR + if new.exists(): + return new + legacy = root / LEGACY_INTERNAL_DIR_NAME / SKILLS_TREE_DIR + return legacy if legacy.exists() else new + + @staticmethod + def _resolved(data: Dict[str, Any], base: Path, + tree: Path) -> Dict[str, Any]: + """Anchor relative sources at *base*; prepend the live *tree* when it + exists. Preserves the empty-dict shape for missing/empty files.""" + out = dict(data) + sources = [ + resolve_source_entry(s, base) + for s in (data.get('sources') or []) + ] + implicit: List[str] = [] + if tree.is_dir(): + tree_str = str(tree.resolve()) + if tree_str not in sources: + implicit = [tree_str] + if implicit or sources or 'sources' in data: + out['sources'] = implicit + sources + return out + # -- enable/disable -- def set_skill_enabled( @@ -86,9 +172,18 @@ def remove_source( project_path: Optional[str] = None, ) -> None: path = self._resolve_path(scope, project_path) + if scope == 'project': + base = Path(os.path.expanduser(str(project_path))).resolve() + else: + base = self._global_dir data = self._read(path) sources: List[str] = data.get('sources', []) - data['sources'] = [s for s in sources if s != source] + # Match the raw string or its anchored form, so callers may pass + # either what the file stores or what list_sources returned. + data['sources'] = [ + s for s in sources + if s != source and resolve_source_entry(s, base) != source + ] self._write(path, data) def list_sources( @@ -96,9 +191,11 @@ def list_sources( scope: str = 'global', project_path: Optional[str] = None, ) -> List[str]: - path = self._resolve_path(scope, project_path) - data = self._read(path) - return data.get('sources', []) + if scope == 'project': + if not project_path: + raise ValueError('project_path required for project scope') + return list(self.load_project(project_path).get('sources', [])) + return list(self.load_global().get('sources', [])) # -- internal -- diff --git a/ms_agent/skill/catalog.py b/ms_agent/skill/catalog.py index 7ad9a4ec3..7ee4082dd 100644 --- a/ms_agent/skill/catalog.py +++ b/ms_agent/skill/catalog.py @@ -120,13 +120,14 @@ def load_from_config(self, skills_config) -> None: SkillSource(type=SkillSourceType.LOCAL_DIR, path=str(BUILTIN_SKILLS_DIR))) - # 2. User home skills - for subdir in ("installed", "custom"): - d = USER_SKILLS_DIR / subdir - if d.exists(): - sources.append( - SkillSource(type=SkillSourceType.LOCAL_DIR, - path=str(d))) + # 2. User home skills — one live tree, scanned whole (the loader + # walks it and stops at SKILL.md roots). Subdirectories are + # organization, not origin: legacy installed/ & custom/ keep working + # as plain subpaths. + if USER_SKILLS_DIR.exists(): + sources.append( + SkillSource(type=SkillSourceType.LOCAL_DIR, + path=str(USER_SKILLS_DIR))) # 3a. Structured sources (higher priority) if hasattr(skills_config, "sources") and skills_config.sources: @@ -174,8 +175,22 @@ def load_from_config(self, skills_config) -> None: self._disabled_skills = set(skills_config.disabled) def load_from_sources(self, sources: List[SkillSource]) -> None: - self._sources = sources + # Dedup by identity — the same dir may arrive both as the implicit + # live tree and as an explicit config source; loading it twice only + # burns IO and log noise. + seen: set = set() + unique: List[SkillSource] = [] for source in sources: + path = (str(Path(source.path).expanduser().resolve()) + if source.path else None) + key = (source.type, path, source.repo_id, source.url, + source.subdir, source.plugin_id) + if key in seen: + continue + seen.add(key) + unique.append(source) + self._sources = unique + for source in unique: if not source.enabled: continue try: @@ -371,6 +386,23 @@ def reload(self) -> None: self._skills.clear() self.load_from_sources(self._sources) + def resync(self, skills_config) -> None: + """Rebuild the catalog in place from a (possibly updated) config. + + In place, so long-lived holders (command bridge, skill toolset, + search engine) keep observing the same object; the search index + follows lazily via the cache version. Picks up new/removed sources + and disabled changes — unlike reload(), which only re-reads the + already-known source list. + """ + self._skills.clear() + self._sources = [] + self._disabled_skills = set() + self._whitelist = None + self._config = skills_config + self.load_from_config(skills_config) + self._invalidate_cache() + def reload_skill(self, skill_id: str) -> Optional[SkillSchema]: skill = self._skills.get(skill_id) if skill and skill.skill_path.exists(): diff --git a/ms_agent/skill/loader.py b/ms_agent/skill/loader.py index fcadb37ff..bbaacc755 100644 --- a/ms_agent/skill/loader.py +++ b/ms_agent/skill/loader.py @@ -120,9 +120,19 @@ def _load_single_skill(self, skill_dir: Path) -> Optional[SkillSchema]: logger.error(f'Error loading skill ({skill_dir}): {str(e)}') return None + #: How deep _scan_and_load_skills descends below the scan root. Bounds + #: symlink cycles; deep enough for organizational nesting (category dirs). + _MAX_SCAN_DEPTH = 5 + def _scan_and_load_skills(self, base_path: Path) -> Dict[str, SkillSchema]: """ - Scan directory and load all skills found. + Recursively scan a tree and load every skill root found. + + A skill root is a directory containing ``SKILL.md``; it is treated as + a leaf — its subdirectories (``scripts/``, ``references/``, …) belong + to the skill and are not descended into. Directories without a + ``SKILL.md`` are organizational and are recursed. Hidden directories + (``.hub``, ``.git``, …) are skipped. Args: base_path: Base directory to scan @@ -130,22 +140,28 @@ def _scan_and_load_skills(self, base_path: Path) -> Dict[str, SkillSchema]: Returns: Dictionary mapping skill_id@version to SkillSchema objects """ - skills = {} + skills: Dict[str, SkillSchema] = {} if not base_path.is_dir(): logger.warning(f'Not a valid directory: {base_path}') return skills - for item in base_path.iterdir(): - if item.is_dir() and self._is_skill_directory(item): - skill = self._load_single_skill(item) - if skill: - skill_key = self._get_skill_key(skill=skill) - skills[skill_key] = skill - # logger.info( - # f'Successfully loaded skill: {skill_key} (from {item})' - # ) - + def _walk(directory: Path, depth: int) -> None: + try: + entries = sorted(directory.iterdir()) + except OSError: + return + for item in entries: + if not item.is_dir() or item.name.startswith('.'): + continue + if self._is_skill_directory(item): + skill = self._load_single_skill(item) + if skill: + skills[self._get_skill_key(skill=skill)] = skill + elif depth < self._MAX_SCAN_DEPTH: + _walk(item, depth + 1) + + _walk(base_path, 1) return skills @staticmethod diff --git a/ms_agent/skill/runtime.py b/ms_agent/skill/runtime.py index 65c285d2e..2d9080d13 100644 --- a/ms_agent/skill/runtime.py +++ b/ms_agent/skill/runtime.py @@ -112,24 +112,33 @@ def needs_refresh(self) -> bool: def maybe_refresh_system_prompt( self, messages: list ) -> bool: - """Rebuild messages[0] if skill state changed since last apply. + """Keep messages[0] in step with the current skill state. - Uses _system_content_builder (injected by LLMAgent) to fully - rebuild the system prompt content, covering skills, personalization, - and base prompt in one pass. + Rebuilds the system prompt via _system_content_builder (injected by + LLMAgent — covers base prompt, personalization and skill injection) + and replaces messages[0].content when it differs. Content comparison + every round — NOT a version gate — because the round context may be + rebuilt from the SessionLog (context_assembler.assemble, resume from + cache), which resurrects the system prompt as persisted at write + time; an apply-once version latch would let that stale copy through + on every later round. An in-step prompt compares equal — zero churn. Returns True if the system prompt was actually updated. """ - if not self.needs_refresh(): - return False if not messages or not self._system_content_builder: self._last_applied_version = self._version return False + head = messages[0] + if getattr(head, 'role', None) != 'system': + # Never clobber a non-system head (restored log without a + # system row). + self._last_applied_version = self._version + return False new_content = self._system_content_builder() - changed = messages[0].content != new_content + changed = head.content != new_content if changed: - messages[0].content = new_content + head.content = new_content self._last_applied_version = self._version return changed @@ -145,3 +154,27 @@ def reload_skill(self, skill_id: str) -> bool: def reload_all(self) -> None: self._catalog.reload() self._version += 1 + + def sync_with_config(self, skills_config) -> bool: + """Resync the catalog from an updated skills config; bump the + version only when the effective skill surface (inventory, metadata, + disabled set) actually changed, so maybe_refresh_system_prompt() + rebuilds messages[0] on real change and stays a no-op otherwise. + + Returns True when the surface changed. + """ + before = self._surface() + self._catalog.resync(skills_config) + after = self._surface() + if before != after: + self._version += 1 + return True + return False + + def _surface(self): + catalog = self._catalog + inventory = { + sid: (skill.name, skill.description, skill.version) + for sid, skill in catalog._skills.items() + } + return (inventory, frozenset(catalog._disabled_skills)) diff --git a/ms_agent/skill/skill_tools.py b/ms_agent/skill/skill_tools.py index 7ffa25040..a07c9f187 100644 --- a/ms_agent/skill/skill_tools.py +++ b/ms_agent/skill/skill_tools.py @@ -387,10 +387,9 @@ def _handle_skill_manage(self, args: dict) -> str: return json.dumps({"error": f"Unknown action: {action}"}) def _create_skill(self, skill_id: str, content: str) -> str: - custom_dir = self._get_custom_skills_dir() - skill_dir = custom_dir / skill_id + skill_dir = self._get_managed_skills_dir() / skill_id - if skill_dir.exists(): + if skill_dir.exists() or self._catalog.get_skill(skill_id): return json.dumps( {"error": f"Skill '{skill_id}' already exists"}) @@ -447,13 +446,15 @@ def _delete_skill(self, skill_id: str) -> str: return json.dumps( {"error": f"Skill '{skill_id}' not found"}) - custom_dir = self._get_custom_skills_dir().resolve() + managed_dir = self._get_managed_skills_dir().resolve() # relative_to() avoids the startswith() prefix-bypass before rmtree. try: - skill.skill_path.resolve().relative_to(custom_dir) + skill.skill_path.resolve().relative_to(managed_dir) except ValueError: - return json.dumps( - {"error": "Can only delete custom skills"}) + return json.dumps({ + "error": + "Can only delete skills inside the managed skills tree" + }) shutil.rmtree(skill.skill_path) self._catalog.remove_skill(skill_id) @@ -463,7 +464,9 @@ def _delete_skill(self, skill_id: str) -> str: "message": f"Skill '{skill_id}' deleted successfully", }) - def _get_custom_skills_dir(self) -> Path: - base = USER_SKILLS_DIR / "custom" - base.mkdir(parents=True, exist_ok=True) - return base + def _get_managed_skills_dir(self) -> Path: + """The live skills tree (``/skills``). Agent-created skills + land at its top level; the tree boundary doubles as the delete + guard — external/config-declared source dirs stay read-only.""" + USER_SKILLS_DIR.mkdir(parents=True, exist_ok=True) + return USER_SKILLS_DIR diff --git a/ms_agent/tui/managed_config.py b/ms_agent/tui/managed_config.py index 16fdf5d7d..d62cd391c 100644 --- a/ms_agent/tui/managed_config.py +++ b/ms_agent/tui/managed_config.py @@ -67,49 +67,84 @@ def resolve_mcp_config( def merge_skills_into_config(config, global_home: str, work_dir: Optional[str]): - """Append managed skill sources + disabled from skills.json into - ``config.skills`` (in place, returns config). Catalog dedups by skill_id.""" + """Merge managed skill sources + disabled from skills.json into + ``config.skills`` (in place, returns config). Catalog dedups by skill_id. + + **Replayable**: managed source entries are tagged ``origin: 'managed'`` + and replaced wholesale on every call, and the config's own (yaml-layer) + disabled list is stashed under ``skills._yaml_disabled`` on first merge so + the effective ``disabled`` can be recomputed instead of unioned into + staleness. Callers may therefore re-run this on a live agent's config to + pick up skills.json changes mid-session (see SkillRuntime.sync_with_config). + """ try: from ms_agent.config.skills_manager import SkillsConfigManager - from ms_agent.skill.sources import parse_skill_source merged = SkillsConfigManager(global_dir=global_home).load_merged( work_dir) except Exception: return config src_strings = merged.get('sources') or [] disabled = merged.get('disabled') or [] - if not src_strings and not disabled: - return config - new_sources = [] - for s in src_strings: - try: - src = parse_skill_source(str(s)) - entry = {'type': src.type.value} - for k in ('path', 'repo_id', 'url', 'revision', 'subdir'): - v = getattr(src, k, None) - if v: - entry[k] = v - new_sources.append(entry) - except Exception: - continue + new_sources = _skill_sources_to_entries(src_strings) skills = getattr(config, 'skills', None) existing_sources = [] - existing_disabled = [] + stashed = None if skills is not None: raw = getattr(skills, 'sources', None) if raw: existing_sources = OmegaConf.to_container(raw, resolve=True) or [] - existing_disabled = list(getattr(skills, 'disabled', []) or []) + stashed = getattr(skills, '_yaml_disabled', None) + if stashed is not None: + yaml_disabled = list(stashed) + else: + # First merge: whatever disabled the config carries is the yaml layer. + yaml_disabled = list(getattr(skills, 'disabled', []) or []) \ + if skills is not None else [] + + base_sources = [ + e for e in existing_sources + if not (isinstance(e, dict) and e.get('origin') == 'managed') + ] - combined_sources = list(existing_sources) + new_sources - combined_disabled = list( - dict.fromkeys(existing_disabled + list(disabled))) - if combined_sources: + # Untouched only when nothing is managed NOW and nothing managed was + # merged BEFORE (no stash, no managed-tagged entries) — a replay with an + # emptied managed layer must still strip the previous contribution. + previously_merged = stashed is not None \ + or len(base_sources) != len(existing_sources) + if not src_strings and not disabled and not previously_merged \ + and not base_sources and not yaml_disabled: + return config + + combined_sources = base_sources + new_sources + combined_disabled = list(dict.fromkeys(yaml_disabled + list(disabled))) + OmegaConf.update(config, 'skills._yaml_disabled', yaml_disabled, + merge=False) + if combined_sources or existing_sources: OmegaConf.update(config, 'skills.sources', combined_sources, merge=False) - if combined_disabled: + if combined_disabled or getattr(skills, 'disabled', None): OmegaConf.update(config, 'skills.disabled', combined_disabled, merge=False) return config + + +def _skill_sources_to_entries(src_strings) -> list: + """Managed source strings -> typed entries for ``config.skills.sources``, + each tagged ``origin: 'managed'`` so a re-merge can replace them.""" + from ms_agent.skill.sources import parse_skill_source + + entries = [] + for s in src_strings: + try: + src = parse_skill_source(str(s)) + entry = {'type': src.type.value, 'origin': 'managed'} + for k in ('path', 'repo_id', 'url', 'revision', 'subdir'): + v = getattr(src, k, None) + if v: + entry[k] = v + entries.append(entry) + except Exception: + continue + return entries diff --git a/tests/skill/test_live_tree.py b/tests/skill/test_live_tree.py new file mode 100644 index 000000000..b7b7f649b --- /dev/null +++ b/tests/skill/test_live_tree.py @@ -0,0 +1,297 @@ +"""Live-tree skill discovery: anchored relative sources, implicit per-scope +trees, marker-walk loading, catalog resync and turn-boundary sync.""" +import json + +import pytest +from omegaconf import OmegaConf + +from ms_agent.config.skills_manager import (SkillsConfigManager, + resolve_source_entry) +from ms_agent.skill.catalog import SkillCatalog +from ms_agent.skill.loader import SkillLoader +from ms_agent.skill.runtime import SkillRuntime +from ms_agent.tui.managed_config import merge_skills_into_config + + +def _mk_skill(root, skill_id, description='d'): + d = root / skill_id + d.mkdir(parents=True) + (d / 'SKILL.md').write_text( + f'---\nname: {skill_id}\ndescription: "{description}"\n---\n# {skill_id}\n', + encoding='utf-8') + return d + + +# ---------------------------------------------------------------- resolution + + +class TestResolveSourceEntry: + + def test_dot_relative_anchors_at_base(self, tmp_path): + (tmp_path / 'docker-expert').mkdir() + out = resolve_source_entry('./docker-expert', tmp_path) + assert out == str((tmp_path / 'docker-expert').resolve()) + + def test_parent_relative(self, tmp_path): + target = tmp_path / 'shared' + target.mkdir() + base = tmp_path / 'proj' + base.mkdir() + assert resolve_source_entry('../shared', base) == str(target.resolve()) + + def test_bare_name_anchors(self, tmp_path): + out = resolve_source_entry('docker-expert', tmp_path) + assert out == str((tmp_path / 'docker-expert').resolve()) + + def test_absolute_passthrough(self, tmp_path): + assert resolve_source_entry('/abs/path', tmp_path) == '/abs/path' + + def test_remote_schemes_passthrough(self, tmp_path): + for raw in ('modelscope://org/pack', 'https://x/y', 'git://x/y', + '@owner/repo'): + assert resolve_source_entry(raw, tmp_path) == raw + + def test_owner_repo_shorthand_stays_hub_id(self, tmp_path): + assert resolve_source_entry('owner/repo', tmp_path) == 'owner/repo' + + def test_owner_repo_that_exists_locally_is_a_path(self, tmp_path): + (tmp_path / 'owner' / 'repo').mkdir(parents=True) + out = resolve_source_entry('owner/repo', tmp_path) + assert out == str((tmp_path / 'owner' / 'repo').resolve()) + + +class TestManagerAnchoring: + + @pytest.fixture + def mgr(self, tmp_path): + return SkillsConfigManager(global_dir=str(tmp_path / 'home')) + + def test_project_relative_source_anchors_at_project_root( + self, mgr, tmp_path): + proj = tmp_path / 'proj' + (proj / '.ms_agent').mkdir(parents=True) + (proj / 'docker-expert').mkdir() + (proj / '.ms_agent' / 'skills.json').write_text( + json.dumps({'sources': ['./docker-expert']}), encoding='utf-8') + + sources = mgr.list_sources(scope='project', project_path=str(proj)) + assert str((proj / 'docker-expert').resolve()) in sources + + def test_file_keeps_raw_relative_string_after_writes(self, mgr, tmp_path): + proj = tmp_path / 'proj' + (proj / '.ms_agent').mkdir(parents=True) + cfg = proj / '.ms_agent' / 'skills.json' + cfg.write_text(json.dumps({'sources': ['./docker-expert']}), + encoding='utf-8') + + mgr.set_skill_enabled('x', False, scope='project', + project_path=str(proj)) + data = json.loads(cfg.read_text(encoding='utf-8')) + assert data['sources'] == ['./docker-expert'] + + def test_remove_source_accepts_resolved_form(self, mgr, tmp_path): + proj = tmp_path / 'proj' + (proj / '.ms_agent').mkdir(parents=True) + (proj / 'docker-expert').mkdir() + cfg = proj / '.ms_agent' / 'skills.json' + cfg.write_text(json.dumps({'sources': ['./docker-expert']}), + encoding='utf-8') + + mgr.remove_source(str((proj / 'docker-expert').resolve()), + scope='project', project_path=str(proj)) + data = json.loads(cfg.read_text(encoding='utf-8')) + assert data['sources'] == [] + + def test_implicit_global_tree_prepended(self, mgr, tmp_path): + tree = tmp_path / 'home' / 'skills' + _mk_skill(tree, 'dropped') + sources = mgr.list_sources() + assert sources and sources[0] == str(tree.resolve()) + + def test_implicit_project_tree(self, mgr, tmp_path): + proj = tmp_path / 'proj' + tree = proj / '.ms_agent' / 'skills' + _mk_skill(tree, 'proj-skill') + sources = mgr.list_sources(scope='project', project_path=str(proj)) + assert str(tree.resolve()) in sources + + def test_no_tree_no_implicit_and_empty_shape_kept(self, mgr): + assert mgr.load_global() == {} + + def test_merged_contains_both_trees(self, mgr, tmp_path): + _mk_skill(tmp_path / 'home' / 'skills', 'g') + proj = tmp_path / 'proj' + _mk_skill(proj / '.ms_agent' / 'skills', 'p') + merged = mgr.load_merged(project_path=str(proj)) + assert str((tmp_path / 'home' / 'skills').resolve()) in merged['sources'] + assert str((proj / '.ms_agent' / 'skills').resolve()) in merged['sources'] + + +# ------------------------------------------------------------------- loader + + +class TestLoaderMarkerWalk: + + def test_finds_nested_skill_roots(self, tmp_path): + _mk_skill(tmp_path, 'top') + _mk_skill(tmp_path / 'webui', 'nested') + _mk_skill(tmp_path / 'a' / 'b', 'deep') + loaded = SkillLoader().load_skills(str(tmp_path)) + ids = {s.skill_id for s in loaded.values()} + assert ids == {'top', 'nested', 'deep'} + + def test_skill_root_is_a_leaf(self, tmp_path): + d = _mk_skill(tmp_path, 'outer') + # A SKILL.md inside the skill's own support dir must not double-load. + _mk_skill(d / 'references', 'inner') + loaded = SkillLoader().load_skills(str(tmp_path)) + ids = {s.skill_id for s in loaded.values()} + assert ids == {'outer'} + + def test_hidden_dirs_skipped(self, tmp_path): + _mk_skill(tmp_path / '.hub', 'ghost') + _mk_skill(tmp_path, 'real') + loaded = SkillLoader().load_skills(str(tmp_path)) + ids = {s.skill_id for s in loaded.values()} + assert ids == {'real'} + + +# ---------------------------------------------------- catalog resync + sync + + +def _skills_config(sources=(), disabled=()): + return OmegaConf.create({ + 'sources': [{'type': 'local', 'path': str(p)} for p in sources], + 'disabled': list(disabled), + }) + + +class TestCatalogResync: + + def test_resync_picks_up_new_source(self, tmp_path): + s1 = _mk_skill(tmp_path, 'alpha') + cfg = _skills_config([s1]) + cat = SkillCatalog(config=cfg) + cat.load_from_config(cfg) + assert cat.get_skill('alpha') + + s2 = _mk_skill(tmp_path / 'later', 'beta') + cat.resync(_skills_config([s1, s2.parent])) + assert cat.get_skill('alpha') and cat.get_skill('beta') + + def test_resync_drops_removed_skills(self, tmp_path): + s1 = _mk_skill(tmp_path, 'alpha') + cfg = _skills_config([s1]) + cat = SkillCatalog(config=cfg) + cat.load_from_config(cfg) + cat.resync(_skills_config([])) + assert cat.get_skill('alpha') is None + + def test_duplicate_sources_load_once(self, tmp_path): + _mk_skill(tmp_path, 'alpha') + cfg = _skills_config([tmp_path, tmp_path]) + cat = SkillCatalog(config=cfg) + cat.load_from_config(cfg) + assert len(cat._sources) == len( + {(s.type, s.path) for s in cat._sources}) + + +class TestRuntimeSync: + + def test_sync_bumps_version_only_on_change(self, tmp_path): + s1 = _mk_skill(tmp_path, 'alpha') + cfg = _skills_config([s1]) + cat = SkillCatalog(config=cfg) + cat.load_from_config(cfg) + rt = SkillRuntime(catalog=cat) + + assert rt.sync_with_config(cfg) is False # no change → no bump + v0 = rt.version + + s2 = _mk_skill(tmp_path / 'more', 'beta') + assert rt.sync_with_config( + _skills_config([s1, s2.parent])) is True + assert rt.version == v0 + 1 + assert rt.needs_refresh() + + def test_sync_sees_disabled_change(self, tmp_path): + s1 = _mk_skill(tmp_path, 'alpha') + cfg = _skills_config([s1]) + cat = SkillCatalog(config=cfg) + cat.load_from_config(cfg) + rt = SkillRuntime(catalog=cat) + + assert rt.sync_with_config( + _skills_config([s1], disabled=['alpha'])) is True + assert 'alpha' not in cat.get_enabled_skills() + + +# ------------------------------------------------- replayable managed merge + + +class TestReplayableMerge: + + def test_re_merge_replaces_managed_layer(self, tmp_path): + home = tmp_path / 'home' + proj = tmp_path / 'proj' + proj.mkdir() + mgr = SkillsConfigManager(global_dir=str(home)) + mgr.add_source(str(_mk_skill(tmp_path, 'one')), scope='project', + project_path=str(proj)) + + cfg = OmegaConf.create({}) + merge_skills_into_config(cfg, str(home), str(proj)) + first = [e['path'] for e in cfg.skills.sources] + assert str((tmp_path / 'one').resolve()) in first + + # Managed layer changes: source removed, another added, one disabled. + mgr.remove_source(str((tmp_path / 'one').resolve()), scope='project', + project_path=str(proj)) + mgr.add_source(str(_mk_skill(tmp_path, 'two')), scope='project', + project_path=str(proj)) + mgr.set_skill_enabled('one', False, scope='project', + project_path=str(proj)) + + merge_skills_into_config(cfg, str(home), str(proj)) + paths = [e['path'] for e in cfg.skills.sources] + assert str((tmp_path / 'two').resolve()) in paths + assert str((tmp_path / 'one').resolve()) not in paths + assert list(cfg.skills.disabled) == ['one'] + + def test_yaml_layer_survives_re_merge(self, tmp_path): + home = tmp_path / 'home' + proj = tmp_path / 'proj' + proj.mkdir() + mgr = SkillsConfigManager(global_dir=str(home)) + mgr.add_source(str(_mk_skill(tmp_path, 'managed-skill')), + scope='project', project_path=str(proj)) + + cfg = OmegaConf.create({ + 'skills': { + 'sources': [{'type': 'local', 'path': '/yaml/declared'}], + 'disabled': ['yaml-off'], + } + }) + merge_skills_into_config(cfg, str(home), str(proj)) + merge_skills_into_config(cfg, str(home), str(proj)) # replay + + paths = [e['path'] for e in cfg.skills.sources] + assert paths.count('/yaml/declared') == 1 + assert 'yaml-off' in list(cfg.skills.disabled) + + def test_managed_disabled_clears_on_re_merge(self, tmp_path): + home = tmp_path / 'home' + proj = tmp_path / 'proj' + proj.mkdir() + mgr = SkillsConfigManager(global_dir=str(home)) + mgr.set_skill_enabled('s', False, scope='project', + project_path=str(proj)) + + cfg = OmegaConf.create({}) + merge_skills_into_config(cfg, str(home), str(proj)) + assert 's' in list(cfg.skills.disabled) + + mgr.set_skill_enabled('s', True, scope='project', + project_path=str(proj)) + merge_skills_into_config(cfg, str(home), str(proj)) + assert 's' not in list(cfg.skills.disabled or []) diff --git a/tests/skills/test_skill.py b/tests/skills/test_skill.py index a8b0447fb..bd0fecc82 100644 --- a/tests/skills/test_skill.py +++ b/tests/skills/test_skill.py @@ -689,7 +689,7 @@ def test_skill_manage_create_and_delete(self): content = ( '---\nname: New Skill\ndescription: "A new skill"\n---\n' '# New Skill\n\nInstructions.') - with patch.object(self.toolset, '_get_custom_skills_dir', + with patch.object(self.toolset, '_get_managed_skills_dir', return_value=self.tmp / "_custom"): result = self.toolset._handle_skill_manage({ "action": "create", @@ -712,7 +712,7 @@ def test_skill_manage_create_and_delete(self): def test_skill_manage_create_duplicate(self): content = ( '---\nname: Demo Dup\ndescription: "dup"\n---\n# Dup\n') - with patch.object(self.toolset, '_get_custom_skills_dir', + with patch.object(self.toolset, '_get_managed_skills_dir', return_value=self.tmp): result = self.toolset._handle_skill_manage({ "action": "create", @@ -723,7 +723,7 @@ def test_skill_manage_create_duplicate(self): self.assertIn("error", data) def test_skill_manage_create_invalid_frontmatter(self): - with patch.object(self.toolset, '_get_custom_skills_dir', + with patch.object(self.toolset, '_get_managed_skills_dir', return_value=self.tmp / "_custom2"): result = self.toolset._handle_skill_manage({ "action": "create", From 888608787f5a38551e545098f9c8484118655d67 Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Fri, 24 Jul 2026 12:13:14 +0800 Subject: [PATCH 10/15] =?UTF-8?q?feat:=20skill=20update-notice=20mode=20?= =?UTF-8?q?=E2=80=94=20byte-stable=20system=20prompt,=20changes=20announce?= =?UTF-8?q?d=20in-conversation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ms_agent/agent/llm_agent.py | 8 +- .../session/strategies/summary_compactor.py | 3 + ms_agent/skill/prompt_injector.py | 27 +++++- ms_agent/skill/runtime.py | 8 ++ tests/skill/test_update_notice.py | 82 +++++++++++++++++++ 5 files changed, 126 insertions(+), 2 deletions(-) create mode 100644 tests/skill/test_update_notice.py diff --git a/ms_agent/agent/llm_agent.py b/ms_agent/agent/llm_agent.py index 8523cae08..ed59ab6be 100644 --- a/ms_agent/agent/llm_agent.py +++ b/ms_agent/agent/llm_agent.py @@ -295,8 +295,10 @@ async def prepare_skills(self): self._skill_catalog.load_from_config(skills_config) prompt_injection = getattr(skills_config, 'prompt_injection', 'all') + update_notice = bool(getattr(skills_config, 'update_notice', False)) self._skill_injector = SkillPromptInjector( - self._skill_catalog, prompt_injection=prompt_injection) + self._skill_catalog, prompt_injection=prompt_injection, + update_notice=update_notice) search_cfg = getattr(skills_config, 'search', None) search_backend = getattr(search_cfg, 'backend', 'bm25') if search_cfg else 'bm25' @@ -329,6 +331,10 @@ async def prepare_skills(self): catalog=self._skill_catalog, injector=self._skill_injector, ) + # Notice mode: the host announces skill changes inside the + # conversation, so the system prompt stays byte-stable per session + # (tail-only updates; prefix cache never breaks). + self._skill_runtime.head_refresh_enabled = not update_notice self._skill_runtime.set_system_content_builder( self._build_system_content ) diff --git a/ms_agent/session/strategies/summary_compactor.py b/ms_agent/session/strategies/summary_compactor.py index f2191a967..c7ae64d10 100644 --- a/ms_agent/session/strategies/summary_compactor.py +++ b/ms_agent/session/strategies/summary_compactor.py @@ -22,6 +22,9 @@ - Accomplished: What's done, in progress, and remaining - Relevant files: Files read, edited, or created +Preserve verbatim, never rewrite or drop: the LATEST +skill-update notice, if any (it is the authoritative current skill list). + Keep it concise but comprehensive enough for another agent to continue.""" SUMMARY_INPUT_CHAR_LIMIT = 2000 diff --git a/ms_agent/skill/prompt_injector.py b/ms_agent/skill/prompt_injector.py index bf736211c..abe3d92a1 100644 --- a/ms_agent/skill/prompt_injector.py +++ b/ms_agent/skill/prompt_injector.py @@ -27,7 +27,23 @@ class SkillPromptInjector: DISCOVERY_HINT = ( "\nUse `skills_list(query=...)` to discover available skills.\n") - def __init__(self, catalog, *, prompt_injection: str = 'all'): + #: Appended when the host emits skill-update notices into the + #: conversation (``skills.update_notice``): the prompt's own list is a + #: session-start snapshot, and the latest in-conversation notice wins. + UPDATE_NOTICE_HINT = """ +**About skill list updates:** +The skill list here reflects the state when this session started. Skills +can be added, removed, enabled, disabled or edited mid-conversation; when +that happens, a skill-update notice containing the FULL +current list appears in the conversation. +- If any skill-update notice exists, the LATEST one is authoritative and + supersedes this list and all earlier notices. +- Earlier notices may be outdated; that is normal bookkeeping — do not + mention notices or list changes to the user unless asked. +""" + + def __init__(self, catalog, *, prompt_injection: str = 'all', + update_notice: bool = False): """ Args: catalog: The SkillCatalog instance. @@ -35,9 +51,14 @@ def __init__(self, catalog, *, prompt_injection: str = 'all'): ``"always_only"`` (only always-active skills in prompt, rest via skills_list), or ``"none"`` (pure tool-driven discovery). + update_notice: When True, the section explains that + in-conversation skill-update notices supersede the + prompt's own list (the host is responsible for emitting + them). """ self._catalog = catalog self._prompt_injection = prompt_injection + self._update_notice = update_notice def build_skill_prompt_section(self) -> str: """Build the skill section for system prompt injection. @@ -59,12 +80,16 @@ def build_skill_prompt_section(self) -> str: summary = self._catalog.get_skills_summary() if summary: parts.append(self.SKILL_SECTION_HEADER) + if self._update_notice: + parts.append(self.UPDATE_NOTICE_HINT) parts.append(summary) parts.append("") elif self._prompt_injection in ('always_only', 'none'): has_skills = bool(self._catalog.get_enabled_skills()) if has_skills: parts.append(self.SKILL_SECTION_HEADER) + if self._update_notice: + parts.append(self.UPDATE_NOTICE_HINT) parts.append(self.DISCOVERY_HINT) return "\n".join(parts) diff --git a/ms_agent/skill/runtime.py b/ms_agent/skill/runtime.py index 2d9080d13..d773227b3 100644 --- a/ms_agent/skill/runtime.py +++ b/ms_agent/skill/runtime.py @@ -34,6 +34,11 @@ def __init__( self._version: int = 0 self._last_applied_version: int = 0 self._system_content_builder: Optional[Callable[[], str]] = None + # False when the host delivers skill updates as in-conversation + # notices (``skills.update_notice``): the system prompt then stays + # byte-stable for the whole session — prefix-cache friendly — and + # maybe_refresh_system_prompt() becomes a no-op. + self.head_refresh_enabled: bool = True @property def catalog(self) -> 'SkillCatalog': @@ -125,6 +130,9 @@ def maybe_refresh_system_prompt( Returns True if the system prompt was actually updated. """ + if not self.head_refresh_enabled: + self._last_applied_version = self._version + return False if not messages or not self._system_content_builder: self._last_applied_version = self._version return False diff --git a/tests/skill/test_update_notice.py b/tests/skill/test_update_notice.py new file mode 100644 index 000000000..49bc9b3e2 --- /dev/null +++ b/tests/skill/test_update_notice.py @@ -0,0 +1,82 @@ +"""Notice mode: head stability gate + conditional prompt wording + +compaction preservation rule.""" +from omegaconf import OmegaConf + +from ms_agent.skill.catalog import SkillCatalog +from ms_agent.skill.prompt_injector import SkillPromptInjector +from ms_agent.skill.runtime import SkillRuntime + + +def _mk_skill(root, skill_id): + d = root / skill_id + d.mkdir(parents=True) + (d / 'SKILL.md').write_text( + f'---\nname: {skill_id}\ndescription: "d"\n---\n# {skill_id}\n', + encoding='utf-8') + return d + + +def _catalog(tmp_path): + _mk_skill(tmp_path, 'alpha') + cfg = OmegaConf.create( + {'sources': [{'type': 'local', 'path': str(tmp_path)}]}) + cat = SkillCatalog(config=cfg) + cat.load_from_config(cfg) + return cat + + +class _Msg: + def __init__(self, role, content): + self.role = role + self.content = content + + +class TestHeadGate: + + def test_disabled_gate_keeps_head_untouched(self, tmp_path): + cat = _catalog(tmp_path) + rt = SkillRuntime(catalog=cat) + rt.set_system_content_builder(lambda: 'NEW HEAD') + rt.head_refresh_enabled = False + + messages = [_Msg('system', 'OLD HEAD')] + assert rt.maybe_refresh_system_prompt(messages) is False + assert messages[0].content == 'OLD HEAD' + + def test_enabled_gate_still_refreshes(self, tmp_path): + cat = _catalog(tmp_path) + rt = SkillRuntime(catalog=cat) + rt.set_system_content_builder(lambda: 'NEW HEAD') + + messages = [_Msg('system', 'OLD HEAD')] + assert rt.maybe_refresh_system_prompt(messages) is True + assert messages[0].content == 'NEW HEAD' + + def test_non_system_head_never_clobbered(self, tmp_path): + cat = _catalog(tmp_path) + rt = SkillRuntime(catalog=cat) + rt.set_system_content_builder(lambda: 'NEW HEAD') + + messages = [_Msg('user', 'hello')] + assert rt.maybe_refresh_system_prompt(messages) is False + assert messages[0].content == 'hello' + + +class TestNoticeWording: + + def test_update_notice_hint_present_when_enabled(self, tmp_path): + cat = _catalog(tmp_path) + injector = SkillPromptInjector(cat, update_notice=True) + section = injector.build_skill_prompt_section() + assert 'skill-update notice' in section + assert 'LATEST one is authoritative' in section + + def test_hint_absent_by_default(self, tmp_path): + cat = _catalog(tmp_path) + section = SkillPromptInjector(cat).build_skill_prompt_section() + assert 'skill-update notice' not in section + + +def test_summary_prompt_preserves_latest_notice(): + from ms_agent.session.strategies.summary_compactor import SUMMARY_PROMPT + assert 'skill-update notice' in SUMMARY_PROMPT From ccf5b6edd23c3bbd962af117839bf6beb872cb85 Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Fri, 24 Jul 2026 17:29:43 +0800 Subject: [PATCH 11/15] feat: expand ${VAR} env placeholders in managed MCP config at runtime --- ms_agent/tui/managed_config.py | 31 ++++++++++++++++++- tests/tui/test_managed_config.py | 52 ++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/ms_agent/tui/managed_config.py b/ms_agent/tui/managed_config.py index d62cd391c..0f0d78d86 100644 --- a/ms_agent/tui/managed_config.py +++ b/ms_agent/tui/managed_config.py @@ -18,6 +18,7 @@ import json import os +import re from typing import Any, Dict, Optional from omegaconf import OmegaConf @@ -29,6 +30,29 @@ 'trust_remote_code', '_removed', }) +#: ``${NAME}`` placeholders in managed MCP entries (headers, args, url, env +#: values). Deliberately braces-only — a bare ``$VAR`` in an arg stays what +#: the user typed. +_ENV_PLACEHOLDER_RE = re.compile(r'\$\{([A-Za-z_][A-Za-z0-9_]*)\}') + + +def expand_env_placeholders(value: Any) -> Any: + """Recursively expand ``${VAR}`` from the process environment. + + Runtime-only: the managed files and the management UI keep the + placeholders (secrets never land on disk or on screen). An unresolved + name stays verbatim, so a missing variable surfaces in the connection + error instead of silently becoming an empty string. + """ + if isinstance(value, str): + return _ENV_PLACEHOLDER_RE.sub( + lambda m: os.environ.get(m.group(1), m.group(0)), value) + if isinstance(value, dict): + return {k: expand_env_placeholders(v) for k, v in value.items()} + if isinstance(value, list): + return [expand_env_placeholders(v) for v in value] + return value + def resolve_mcp_config( global_home: str, @@ -63,7 +87,12 @@ def resolve_mcp_config( servers.update(src) except Exception: pass - return {'mcpServers': servers} if servers else None + if not servers: + return None + # ${VAR} placeholders resolve here — at the runtime boundary — so the + # connection gets real credentials while every management surface keeps + # the placeholder form. + return {'mcpServers': expand_env_placeholders(servers)} def merge_skills_into_config(config, global_home: str, work_dir: Optional[str]): diff --git a/tests/tui/test_managed_config.py b/tests/tui/test_managed_config.py index f4289a01e..517b1d617 100644 --- a/tests/tui/test_managed_config.py +++ b/tests/tui/test_managed_config.py @@ -63,3 +63,55 @@ def test_skills_noop_when_empty(tmp_path): cfg = OmegaConf.create({'llm': {'model': 'x'}}) out = merge_skills_into_config(cfg, str(tmp_path / 'home'), None) assert not getattr(out, 'skills', None) # nothing added + + +# ── ${VAR} placeholder expansion ─────────────────────────────────────────── + + +def test_expand_env_placeholders_recursive(monkeypatch): + from ms_agent.tui.managed_config import expand_env_placeholders + + monkeypatch.setenv('MY_TOKEN', 'sk-secret') + value = { + 'headers': {'Authorization': 'Bearer ${MY_TOKEN}'}, + 'args': ['--key', '${MY_TOKEN}', 'plain'], + 'nested': [{'url': 'https://x/${MY_TOKEN}/y'}], + 'count': 3, + } + out = expand_env_placeholders(value) + assert out['headers']['Authorization'] == 'Bearer sk-secret' + assert out['args'] == ['--key', 'sk-secret', 'plain'] + assert out['nested'][0]['url'] == 'https://x/sk-secret/y' + assert out['count'] == 3 # non-strings pass through + + +def test_expand_unresolved_placeholder_stays_verbatim(monkeypatch): + from ms_agent.tui.managed_config import expand_env_placeholders + + monkeypatch.delenv('NO_SUCH_VAR_XYZ', raising=False) + assert expand_env_placeholders('${NO_SUCH_VAR_XYZ}') == '${NO_SUCH_VAR_XYZ}' + # Braces-only syntax: a bare $VAR is left alone by design. + monkeypatch.setenv('BARE', 'v') + assert expand_env_placeholders('$BARE and ${BARE}') == '$BARE and v' + + +def test_resolve_mcp_config_expands_placeholders(tmp_path, monkeypatch): + monkeypatch.setenv('DASH_KEY', 'real-key-123') + home = str(tmp_path / 'home') + mm = MCPConfigManager(global_root=home) + mm.add('remote-auth', { + 'url': 'https://gw/sse', 'transport': 'sse', + 'headers': {'Authorization': 'Bearer ${DASH_KEY}'}, + }, scope='global') + mm.add('stdio-arg', { + 'command': 'uvx', 'args': ['tool', '--api-key', '${DASH_KEY}'], + }, scope='global') + + r = resolve_mcp_config(home, None) + servers = r['mcpServers'] + assert servers['remote-auth']['headers']['Authorization'] == 'Bearer real-key-123' + assert servers['stdio-arg']['args'] == ['tool', '--api-key', 'real-key-123'] + + # The managed file keeps the placeholder — expansion is runtime-only. + stored = mm.get('remote-auth', 'global') + assert stored['headers']['Authorization'] == 'Bearer ${DASH_KEY}' From 44dd430000a03b35e9fde62a47f476ac094880aa Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Sat, 25 Jul 2026 16:33:16 +0800 Subject: [PATCH 12/15] update requirements --- .github/workflows/citest.yaml | 8 ++++++++ requirements/cinema.txt | 3 +++ requirements/framework.txt | 4 ---- requirements/retrieval.txt | 3 +++ setup.py | 20 ++++++++++++++------ 5 files changed, 28 insertions(+), 10 deletions(-) create mode 100644 requirements/cinema.txt create mode 100644 requirements/retrieval.txt diff --git a/.github/workflows/citest.yaml b/.github/workflows/citest.yaml index 9df3998f8..75daf4892 100644 --- a/.github/workflows/citest.yaml +++ b/.github/workflows/citest.yaml @@ -54,6 +54,14 @@ jobs: if: steps.cache.outputs.cache-hit != 'true' run: pip install -r requirements/code.txt + - name: Install Retrieval Dependencies + if: steps.cache.outputs.cache-hit != 'true' + run: pip install -r requirements/retrieval.txt + + - name: Install Cinema Dependencies + if: steps.cache.outputs.cache-hit != 'true' + run: pip install -r requirements/cinema.txt + - name: Run tests shell: bash run: bash .dev_scripts/dockerci.sh diff --git a/requirements/cinema.txt b/requirements/cinema.txt new file mode 100644 index 000000000..93cddccae --- /dev/null +++ b/requirements/cinema.txt @@ -0,0 +1,3 @@ +# Media stack for the singularity_cinema project and audio/video tools (matplotlib stays in the base install for the local code executor). +edge_tts +moviepy diff --git a/requirements/framework.txt b/requirements/framework.txt index 4488d9d4a..4705aa4d5 100644 --- a/requirements/framework.txt +++ b/requirements/framework.txt @@ -1,15 +1,12 @@ aiohttp croniter>=1.3.0 dotenv -edge_tts -faiss-cpu json5 loguru markdown matplotlib mcp modelscope>=1.35.2 -moviepy numpy omegaconf openai @@ -20,5 +17,4 @@ pyyaml pytz requests rich -sentence-transformers typing_extensions diff --git a/requirements/retrieval.txt b/requirements/retrieval.txt new file mode 100644 index 000000000..4a307edfe --- /dev/null +++ b/requirements/retrieval.txt @@ -0,0 +1,3 @@ +# Vector/hybrid skill-search backends (opt-in; pulls torch via sentence-transformers). Default skill backend is bm25 and needs none of these. +faiss-cpu +sentence-transformers diff --git a/setup.py b/setup.py index d1f071834..bee20dc80 100644 --- a/setup.py +++ b/setup.py @@ -234,18 +234,26 @@ def _build_and_copy_webui(self): 'requirements/framework.txt') extra_requires = {} - all_requires = [] extra_requires['research'], _ = parse_requirements( 'requirements/research.txt') extra_requires['code'], _ = parse_requirements('requirements/code.txt') extra_requires['webui'], _ = parse_requirements('requirements/webui.txt') extra_requires['acp'], _ = parse_requirements('requirements/acp.txt') extra_requires['a2a'], _ = parse_requirements('requirements/a2a.txt') - all_requires.extend(install_requires) - all_requires.extend(extra_requires['research']) - all_requires.extend(extra_requires['code']) - all_requires.extend(extra_requires['webui']) - extra_requires['all'] = all_requires + extra_requires['retrieval'], _ = parse_requirements( + 'requirements/retrieval.txt') + extra_requires['cinema'], _ = parse_requirements( + 'requirements/cinema.txt') + extra_requires['docs'], _ = parse_requirements('requirements/docs.txt') + + # ``all`` aggregates every *runtime* extra so that `pip install ms-agent[all]` + # yields a fully-featured install. ``docs`` is build-only and intentionally + # excluded. De-duplicated for a clean, deterministic dependency set. + all_requires = list(install_requires) + for _group in ('research', 'code', 'webui', 'acp', 'a2a', 'retrieval', + 'cinema'): + all_requires.extend(extra_requires[_group]) + extra_requires['all'] = sorted(set(all_requires)) setup( name='ms-agent', From 4990fe737668b594e760af5e30d8424ed4d28feb Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Mon, 27 Jul 2026 19:05:06 +0800 Subject: [PATCH 13/15] feat: thread tool_call id to permission ask so decisions correlate to the exact call --- ms_agent/hooks/permission_resolve.py | 8 +++- ms_agent/permission/enforcer.py | 25 ++++++++++++ ms_agent/permission/handler.py | 5 +++ ms_agent/tools/tool_manager.py | 3 ++ tests/permission/test_parallel_permission.py | 40 ++++++++++++++++++++ 5 files changed, 79 insertions(+), 2 deletions(-) diff --git a/ms_agent/hooks/permission_resolve.py b/ms_agent/hooks/permission_resolve.py index 0bdaddb09..4ed842222 100644 --- a/ms_agent/hooks/permission_resolve.py +++ b/ms_agent/hooks/permission_resolve.py @@ -60,6 +60,7 @@ async def resolve_hook_permission_decision( permission_config: PermissionConfig | None, hook_runtime: HookRuntime | None = None, force_decision: PermissionDecision | None = None, + call_id: str = '', ) -> PermissionDecision | str: # ``force_decision`` carries a SafetyGuard ``ask`` (interactive): it must # reach the handler even when whitelist/memory would otherwise allow, so a @@ -82,7 +83,8 @@ async def resolve_hook_permission_decision( if rule and rule.action == 'ask': if permission_enforcer: return await permission_enforcer.check( - tool_name, args, force_decision=rule) + tool_name, args, force_decision=rule, + call_id=call_id) return PermissionDecision( action='allow', reason=hook_result.reason or 'Allowed by PreToolUse hook', @@ -95,6 +97,7 @@ async def resolve_hook_permission_decision( args, force_decision=PermissionDecision( action='ask', reason=hook_result.reason), + call_id=call_id, ) pr = await _run_permission_request_hook( @@ -107,9 +110,10 @@ async def resolve_hook_permission_decision( args, force_decision=PermissionDecision( action='ask', reason=pr.reason), + call_id=call_id, ) if permission_enforcer: return await permission_enforcer.check( - tool_name, args, force_decision=force_decision) + tool_name, args, force_decision=force_decision, call_id=call_id) return PermissionDecision(action='allow', reason='No permission enforcer') diff --git a/ms_agent/permission/enforcer.py b/ms_agent/permission/enforcer.py index c63fd3c40..59904427c 100644 --- a/ms_agent/permission/enforcer.py +++ b/ms_agent/permission/enforcer.py @@ -7,6 +7,7 @@ from __future__ import annotations import asyncio +import inspect from dataclasses import dataclass from typing import Any, Literal @@ -58,16 +59,38 @@ def _ask_lock_for_loop(self) -> 'asyncio.Lock': return self._ask_lock async def _serialized_ask(self, **kwargs) -> PermissionResponse: + # ``call_id`` is a newer, optional kwarg (see check()). A handler that + # predates it — or a lightweight test double — need not accept it; drop + # it for such handlers so their fixed signature keeps working. + if 'call_id' in kwargs and not self._handler_accepts('call_id'): + kwargs.pop('call_id') async with self._ask_lock_for_loop(): return await self._handler.ask(**kwargs) + def _handler_accepts(self, param: str) -> bool: + try: + sig = inspect.signature(self._handler.ask) + except (TypeError, ValueError): + return True # can't introspect — assume it takes it, don't strip + params = sig.parameters.values() + return ( + any(p.name == param for p in params) + or any(p.kind == inspect.Parameter.VAR_KEYWORD for p in params) + ) + async def check( self, tool_name: str, tool_args: dict[str, Any], *, force_decision: PermissionDecision | None = None, + call_id: str = '', ) -> PermissionDecision: + # ``call_id`` (the tool_call this ask is gating) is threaded to the + # handler so a UI can record/correlate the decision against the exact + # call — important when a round fires several identical tool calls in + # parallel. Empty when the LLM adapter didn't assign an id yet; + # handlers must tolerate that. # 1. Blacklist → deny (not overridable in any mode) for pattern in self._config.blacklist: if self._matcher.match_with_content(pattern, tool_name, tool_args): @@ -83,6 +106,7 @@ async def check( tool_args=tool_args, context=force_decision.reason or '', suggestions=suggestions, + call_id=call_id, ) return self._process_response(response, tool_name, tool_args) @@ -112,6 +136,7 @@ async def check( tool_args=tool_args, context='', suggestions=suggestions, + call_id=call_id, ) return self._process_response(response, tool_name, tool_args) diff --git a/ms_agent/permission/handler.py b/ms_agent/permission/handler.py index 622c12543..31cf19745 100644 --- a/ms_agent/permission/handler.py +++ b/ms_agent/permission/handler.py @@ -40,6 +40,7 @@ async def ask( tool_args: dict[str, Any], context: str, suggestions: list[str] | None = None, + call_id: str = '', ) -> PermissionResponse: ... @@ -52,6 +53,7 @@ async def ask( tool_args: dict[str, Any], context: str, suggestions: list[str] | None = None, + call_id: str = '', ) -> PermissionResponse: return PermissionResponse(action=PermissionAction.ALLOW_ONCE) @@ -65,6 +67,7 @@ async def ask( tool_args: dict[str, Any], context: str, suggestions: list[str] | None = None, + call_id: str = '', ) -> PermissionResponse: args_display = json.dumps(tool_args, ensure_ascii=False, indent=2) if len(args_display) > 500: @@ -148,6 +151,7 @@ async def ask( tool_args: dict[str, Any], context: str, suggestions: list[str] | None = None, + call_id: str = '', ) -> PermissionResponse: request_id = uuid4().hex loop = asyncio.get_running_loop() @@ -157,6 +161,7 @@ async def ask( self._event_emitter.emit({ 'type': 'permission_request', 'request_id': request_id, + 'call_id': call_id, 'tool_name': tool_name, 'tool_args': tool_args, 'context': context, diff --git a/ms_agent/tools/tool_manager.py b/ms_agent/tools/tool_manager.py index 061e32fb3..57a649a39 100644 --- a/ms_agent/tools/tool_manager.py +++ b/ms_agent/tools/tool_manager.py @@ -547,6 +547,9 @@ async def single_call_tool(self, tool_info: ToolCall): permission_config=self._permission_config, hook_runtime=self._hook_runtime, force_decision=safety_force_decision, + # Thread the tool_call id so the handler can correlate its + # decision to the exact call (parallel calls in one round). + call_id=str(tool_info.get('id') or ''), ) if isinstance(perm_out, str): return perm_out diff --git a/tests/permission/test_parallel_permission.py b/tests/permission/test_parallel_permission.py index 421442a26..4f48f3f78 100644 --- a/tests/permission/test_parallel_permission.py +++ b/tests/permission/test_parallel_permission.py @@ -43,3 +43,43 @@ async def test_parallel_asks_are_serialized(tmp_path): assert handler.calls == 5 # The decisive assertion: asks never overlapped (was 5 before the fix). assert handler.max_in_flight == 1 + + +class _CallIdCapture: + """Handler that records the call_id it was asked with (and tolerates + handlers that don't accept it — see enforcer._serialized_ask).""" + def __init__(self): + self.seen = [] + + async def ask(self, tool_name, tool_args, context, suggestions=None, + call_id=''): + self.seen.append(call_id) + return PermissionResponse(action=PermissionAction.ALLOW_ONCE) + + +@pytest.mark.asyncio +async def test_call_id_threaded_to_handler(tmp_path): + """enforcer.check(call_id=...) reaches handler.ask so a UI can correlate the + decision to the exact tool_call (parallel identical calls).""" + cfg = PermissionConfig.from_dict({'mode': 'restricted'}) + handler = _CallIdCapture() + enf = PermissionEnforcer( + config=cfg, handler=handler, + memory=PermissionMemory(project_path=str(tmp_path))) + + await enf.check('some_tool', {'a': 1}, call_id='call-abc') + assert handler.seen == ['call-abc'] + + +@pytest.mark.asyncio +async def test_legacy_handler_without_call_id_still_works(tmp_path): + """A handler whose ask() predates call_id (fixed signature) is not broken — + the enforcer strips the unknown kwarg.""" + cfg = PermissionConfig.from_dict({'mode': 'restricted'}) + handler = _ConcurrencyProbe() # ask() has no call_id param + enf = PermissionEnforcer( + config=cfg, handler=handler, + memory=PermissionMemory(project_path=str(tmp_path))) + + r = await enf.check('some_tool', {'a': 1}, call_id='call-xyz') + assert r.action == 'allow' and handler.calls == 1 From fc20c10f1ea2f2a03f06c4a31be9ce2270c0808f Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Mon, 27 Jul 2026 21:44:03 +0800 Subject: [PATCH 14/15] feat: SessionLog.record_loop_end marker for replaying per-loop duration --- ms_agent/session/session_log.py | 43 ++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/ms_agent/session/session_log.py b/ms_agent/session/session_log.py index 5e8c7f0f8..ba4cd4c0f 100644 --- a/ms_agent/session/session_log.py +++ b/ms_agent/session/session_log.py @@ -113,6 +113,26 @@ def record_error(self, event: Dict[str, Any]) -> None: } self._append_line(record) + def record_loop_end(self, event: Dict[str, Any]) -> None: + """Record a tool-call-loop boundary — a non-message, display-only marker. + + Written when a turn's tool-call loop finishes (the final assistant + message with no further tool calls). Like the other ``_type``-tagged + markers it is filtered out of ``get_all_messages`` (never re-enters the + LLM context), but lets history replay reproduce the live "loop done" + summary — most importantly the wall-clock ``duration_ms``, which is not + otherwise derivable from the log. ``event`` typically carries + ``duration_ms`` and ``changed_files``. + """ + seq = self._next_seq() + record = { + "_type": "loop_end", + "seq": seq, + "timestamp": datetime.now(timezone.utc).isoformat(), + **event, + } + self._append_line(record) + def record_permission(self, event: Dict[str, Any]) -> None: """Record a permission decision — a non-message, display-only marker. @@ -188,7 +208,7 @@ def get_all_messages(self) -> List[Dict[str, Any]]: continue if record.get("_type") in ( "metadata", "compaction_event", "error", "permission", - "skill_invocation"): + "skill_invocation", "loop_end"): continue msgs.append(record) self._messages = msgs @@ -270,6 +290,27 @@ def get_permissions(self) -> List[Dict[str, Any]]: perms.append(record) return perms + def get_loop_ends(self) -> List[Dict[str, Any]]: + """All tool-call-loop boundary markers in chronological order (each + keeps its ``seq``). Excluded from ``get_all_messages`` (and the LLM + context); a UI merges them back by ``seq`` to reproduce the per-loop + "done" summary (duration + changed files) on history replay. + """ + out: List[Dict[str, Any]] = [] + if not self._path.exists(): + return out + for line in self._path.read_text(encoding="utf-8").splitlines(): + line = line.strip() + if not line: + continue + try: + record = json.loads(line) + except json.JSONDecodeError: + continue + if record.get("_type") == "loop_end": + out.append(record) + return out + def get_skill_invocations(self) -> List[Dict[str, Any]]: """All slash-skill invocation markers in chronological order (each keeps its ``seq``). Excluded from ``get_all_messages`` (and the LLM From 67d8938704a2eae9d4e15b66ecab88b303709d2c Mon Sep 17 00:00:00 2001 From: alcholiclg Date: Tue, 28 Jul 2026 00:16:26 +0800 Subject: [PATCH 15/15] feat: bare /skill submits the skill body to the agent instead of returning a usage intro --- ms_agent/command/skill_bridge.py | 26 ++++++++++++-------------- tests/command/test_skill_bridge.py | 8 +++++--- tests/command/test_skill_expand.py | 21 ++++++++++++++------- 3 files changed, 31 insertions(+), 24 deletions(-) diff --git a/ms_agent/command/skill_bridge.py b/ms_agent/command/skill_bridge.py index 3cf6b5c24..3b3a13e55 100644 --- a/ms_agent/command/skill_bridge.py +++ b/ms_agent/command/skill_bridge.py @@ -51,32 +51,30 @@ def expand_skill(catalog: 'SkillCatalog', name_or_id: str, args: str) -> CommandResult | None: """Expand one known-skill invocation into a CommandResult. - Returns None when the skill doesn't exist; a usage MESSAGE when ``args`` - is empty; else a SUBMIT_PROMPT whose content is the skill body (frontmatter - stripped, ``$ARGUMENTS`` substituted) wrapped with the standard preamble. + Returns None when the skill doesn't exist; else a SUBMIT_PROMPT whose + content is the skill body (frontmatter stripped, ``$ARGUMENTS`` + substituted) wrapped with the standard preamble. A bare invocation (empty + ``args``) submits too — the model reads the skill and carries out its + instructions — with the tail line noting no extra input was given. Surface-agnostic: no router/context state is involved. """ skill = find_skill(catalog, name_or_id) if skill is None: return None - if not args: - return CommandResult( - type=CommandResultType.MESSAGE, - content=( - f'Skill: {skill.name}\n' - f'Description: {skill.description}\n' - f'Usage: /{skill.skill_id} ' - ), - ) - body = _strip_frontmatter(skill.content) body = body.replace('$ARGUMENTS', args) + tail = ( + f"User's request: {args}" if args else + 'The user invoked this skill without additional arguments; follow ' + 'its instructions and proceed (ask for the missing input only if the ' + 'skill requires one).' + ) enriched = ( f'Use the [{skill.name}] skill located at `{skill.skill_path}`.\n\n' f'{body}\n\n' - f"User's request: {args}" + f'{tail}' ) return CommandResult( type=CommandResultType.SUBMIT_PROMPT, diff --git a/tests/command/test_skill_bridge.py b/tests/command/test_skill_bridge.py index 2d026670c..705bb563b 100644 --- a/tests/command/test_skill_bridge.py +++ b/tests/command/test_skill_bridge.py @@ -48,13 +48,15 @@ def router(self, catalog): return r @pytest.mark.asyncio - async def test_no_args_returns_info(self, router, skill): + async def test_no_args_submits_skill_body(self, router, skill): + # A bare /skill submits the skill body for the model to read and act + # on (no usage-intro MESSAGE); the tail line flags the missing input. ctx = _make_ctx(f'/{skill.skill_id}') result = await router.dispatch(ctx) assert result is not None - assert result.type == CommandResultType.MESSAGE + assert result.type == CommandResultType.SUBMIT_PROMPT assert skill.name in result.content - assert skill.description in result.content + assert 'without additional arguments' in result.content @pytest.mark.asyncio async def test_with_args_returns_submit_prompt(self, router, skill): diff --git a/tests/command/test_skill_expand.py b/tests/command/test_skill_expand.py index cb9fc45fc..a050bc430 100644 --- a/tests/command/test_skill_expand.py +++ b/tests/command/test_skill_expand.py @@ -38,10 +38,15 @@ def test_expand_skill_structured_invocation(catalog): assert '---' not in result.content # frontmatter stripped -def test_expand_skill_intro_and_unknown(catalog): - intro = expand_skill(catalog, 'writer', '') - assert intro.type == CommandResultType.MESSAGE - assert 'Writer' in intro.content +def test_expand_skill_bare_invocation_submits(catalog): + # A bare invocation submits the skill body so the model reads it and acts + # (no more usage-intro MESSAGE); the tail line flags the missing input. + bare = expand_skill(catalog, 'writer', '') + assert bare.type == CommandResultType.SUBMIT_PROMPT + assert 'Use the [Writer] skill' in bare.content + assert 'Guide: ' in bare.content # $ARGUMENTS replaced with '' + assert 'without additional arguments' in bare.content + assert "User's request:" not in bare.content assert expand_skill(catalog, 'nope', 'x') is None # case-insensitive frontmatter-name match still resolves assert expand_skill(catalog, 'WRITER', 'x') is not None @@ -61,11 +66,13 @@ def test_expand_slash_text_boundaries(catalog): assert expand_slash_text(catalog, '路径 a/writer 不是命令') is None assert expand_slash_text(catalog, 'http://writer.example') is None assert expand_slash_text(catalog, '看看 /unknown 是什么') is None - # Leading position still works; token-only input yields the intro. + # Leading position still works; a token-only input submits too (the model + # reads the skill and acts on its instructions). assert expand_slash_text(catalog, '/writer 开始').type \ == CommandResultType.SUBMIT_PROMPT - assert expand_slash_text(catalog, '/writer').type \ - == CommandResultType.MESSAGE + bare = expand_slash_text(catalog, '/writer') + assert bare.type == CommandResultType.SUBMIT_PROMPT + assert 'without additional arguments' in bare.content def test_expand_slash_text_first_known_token_wins():