diff --git a/ms_agent/agent/llm_agent.py b/ms_agent/agent/llm_agent.py index 542f61f77..7d1df805a 100644 --- a/ms_agent/agent/llm_agent.py +++ b/ms_agent/agent/llm_agent.py @@ -466,44 +466,61 @@ def _build_system_content(self) -> str: return content def _build_workspace_internals_section(self) -> str: - """Describe the framework's own directories, when they sit in the - working directory. - - Only for the layout where they do. A project opened from an existing - folder keeps its records elsewhere, and telling that agent to watch out - for a ``sessions/`` directory it will never encounter would be a - fabricated warning. + """Describe where the framework's own records live. + + Which of the two descriptions applies is decided by where the session + log ACTUALLY writes, not by what the working directory happens to + contain: a mounted project may well have a user-owned ``sessions/`` + directory of its own, and describing that as framework transcripts + would be false. The directory-existence check is only the fallback for + callers that run without a session log. """ - from ms_agent.prompting.builtin import WORKSPACE_INTERNALS_HINT + from ms_agent.prompting.builtin import (TRANSCRIPTS_INSIDE, + TRANSCRIPTS_OUTSIDE, + WORKSPACE_RECORDS_HINT) from ms_agent.utils.workspace_context import resolve_workspace_root try: workspace_root = Path(resolve_workspace_root(self.config)) except Exception: # noqa: BLE001 - never break prompt assembly return '' - if not (workspace_root / 'sessions').is_dir(): - return '' - - # The directory the log is actually writing to, not the agent's tag: - # naming a path that does not exist is worse than naming none, since - # the model will go looking for it. - session_dir = None - log = getattr(self, 'session_log', None) - directory = getattr(log, 'directory', None) - if directory is not None: - session_dir = Path(directory).name - if not session_dir: - session_dir = getattr(self.runtime, 'session_id', None) - try: home = str(global_home()) except Exception: # noqa: BLE001 home = '~/.ms_agent' - hint = WORKSPACE_INTERNALS_HINT.format( - session_line=(f' This conversation is `sessions/{session_dir}/`.' - if session_dir else ''), - home=home) - return hint + + directory = getattr( + getattr(self, 'session_log', None), 'directory', None) + records_inside = None + if directory is not None: + try: + Path(directory).relative_to(workspace_root) + records_inside = True + except ValueError: + records_inside = False + if records_inside is None: + # No session log to consult (bare SDK / tests): fall back to the + # managed layout's signature. + if not (workspace_root / 'sessions').is_dir(): + return '' + records_inside = True + + if records_inside: + # The directory the log is actually writing to, not the agent's + # tag: naming a path that does not exist is worse than naming + # none, since the model will go looking for it. + session_dir = Path(directory).name if directory else None + if not session_dir: + session_dir = getattr(self.runtime, 'session_id', None) + transcripts_where = TRANSCRIPTS_INSIDE.format(session_line=( + f' This conversation is `sessions/{session_dir}/`.' + if session_dir else '')) + else: + transcripts_where = TRANSCRIPTS_OUTSIDE.format( + session_dir=str(Path(directory))) + + return WORKSPACE_RECORDS_HINT.format( + transcripts_where=transcripts_where, home=home) def _check_skill_tool_dependencies(self): """Warn if skills are enabled but essential tools are missing.""" diff --git a/ms_agent/prompting/builtin.py b/ms_agent/prompting/builtin.py index e262491f5..762e8dc44 100644 --- a/ms_agent/prompting/builtin.py +++ b/ms_agent/prompting/builtin.py @@ -24,7 +24,7 @@ #: Bump when a template below changes materially. The workspace sidecar #: records the version + sha256 written, so untouched files upgrade silently #: while user-edited files are left alone (see workspace_files.py). -TEMPLATE_VERSION = 1 +TEMPLATE_VERSION = 2 BASE_AGENT_PROMPT = """\ You are MS-Agent, a general-purpose assistant. You help with everyday work of @@ -50,6 +50,10 @@ the machine: sending, publishing, deleting, paying, or overwriting user files. - The user's data is private. Never move it somewhere the user didn't intend. +- Credentials are off-limits unless the task genuinely requires them: private + keys, `.env` values, tokens, and password or cookie stores. When one is + truly needed, read the minimum and never repeat a secret's value into a + reply, file, log, or command line. - Never bypass permission or approval mechanisms, even when asked to hurry. """ @@ -59,9 +63,9 @@ about: Personality and working attitude. Edit freely — this file is yours. --- -# Who You Are +## Who You Are -## Temperament +### Temperament - **Direct.** Skip filler openers like "Great question!" — give the answer or start the work. - **Has judgment.** You may disagree and prefer things, with reasons. Don't @@ -71,13 +75,13 @@ - **Plain words.** Lead with the conclusion, then the detail. Avoid jargon walls. -## With your user +### With your user - You work for a real person on real tasks, not a demo audience. Assume competence; don't oversell or coddle. - Unsure means saying so. Never paper over a gap with a confident tone. - You are a guest. Their files, schedule, and accounts belong to them. -## Boundaries +### Boundaries - Private things stay private. - Outward actions (sending, publishing, deleting) get confirmed first. """ @@ -167,53 +171,63 @@ #: mechanism: without it, models plausibly (and wrongly) tell users their #: system prompt is a session-start snapshot that cannot pick up file edits. LIVE_FILES_HINT = """\ -The persona, instructions and profile above come from workspace files \ +The persona, instructions and profile above come from files \ (SOUL.md, AGENTS.md, PROFILE.md) that stay live during the conversation: \ edits apply from the next round, and this system prompt always shows the \ current file content. When files change mid-conversation, a \ at the start of a user turn lists which ones changed. \ The ~/.ms_agent/... source labels are logical names — on this machine those \ -files actually live in {home}; project AGENTS.md files live in the project \ -directory.""" - -#: Injected when the framework keeps its own records INSIDE the working -#: directory, which is the layout of a managed project. -#: -#: Without it the agent has no way to tell its own bookkeeping apart from the -#: user's material, and the confusion is not hypothetical: searching the -#: workspace for a phrase finds that phrase in the transcript of the very -#: request being served, because the prompt was written there moments earlier. -#: Every hit is real, every hit is worthless, and the model has no reason to -#: suspect it. Naming the directories, and saying what changing them does, is -#: cheaper and less brittle than hiding them — hidden, they would also be -#: unavailable when the user genuinely asks about history or configuration. -WORKSPACE_INTERNALS_HINT = """\ -## Framework files in your working directory - -Two things under your working directory are maintained by the framework rather \ -than written by the user: - -- `sessions/` — a full transcript of every conversation in this project, \ -including the user's messages verbatim.{session_line} -- `.ms_agent/` — this project's state: `memory/` (what is remembered across \ +files actually live in {home}; a project's own AGENTS.md lives in its \ +`.ms_agent/`.""" + +#: Where the framework keeps this project's records — ONE description for +#: both layouts. A managed project's working directory doubles as its records +#: directory; a mounted project keeps records in the data directory. Either +#: way the agent needs the same three facts: where transcripts are, what +#: ``.ms_agent/`` is, and that a search reaching those records matches its own +#: echo (the confusion is not hypothetical: the request being served is +#: already on disk when the search runs). Contents are described by example, +#: not enumerated — the exact file set varies by configuration and version, +#: and the model can list the directory when it matters. +WORKSPACE_RECORDS_HINT = """\ +## Your workspace and the framework's records + +The framework keeps records for this project in two places: + +- **Conversation transcripts** — {transcripts_where} Every conversation is \ +recorded verbatim, including the user's messages. +- **`.ms_agent/` under your working directory** — project state and \ +per-project configuration: for example `memory/` (what is remembered across \ conversations), `snapshots/` (a git repository of previous workspace \ -versions), `permission_memory.json` (approvals the user chose to keep), \ -`web_search/` (cached search results), `mcp.json` and `project.json`. - -Settings that apply to every project live separately, in {home} — the location \ -is configurable, so a machine may have several and this conversation is using \ -that one. - -When you search the workspace, matches inside those two directories are the \ -framework's record of this and earlier conversations, not the user's content. \ -Anything you were just asked is already written to `sessions/`, so searching \ -for a phrase from the request will match your own transcript. Exclude them \ -unless the user is asking about history or configuration, and never cite such \ -a match as if it were something you found in their material. - -You may read these files, and edit them when asked. Be aware that editing \ -`memory/` or `permission_memory.json` changes how later conversations behave, \ -and that `snapshots/` is what makes reverting possible.""" +versions), `permission_memory.json` (approvals the user chose to keep), and \ +`mcp.json` / `skills/` / `AGENTS.md` when this project configures them. The \ +exact contents vary — list the directory when you need to know. Everything \ +else in the working directory is the user's own material. + +Because transcripts contain what you were just asked, any search that \ +reaches them — in the workspace or anywhere else on this machine — will \ +match your own conversation. Treat such matches as the framework's records, \ +not as something found in the user's material, and leave them out of \ +results unless the user is asking about history or configuration. + +You may read all of these records, and edit them when asked. Editing \ +`memory/` or `permission_memory.json` changes how later conversations \ +behave; `snapshots/` is what makes reverting possible. + +Settings that apply to every project live in {home}.""" + +#: ``transcripts_where`` for the layout whose working directory doubles as +#: the records directory. +TRANSCRIPTS_INSIDE = ("in `sessions/` at the root of your working " + "directory.{session_line}") + +#: ``transcripts_where`` for a mounted project: records live in the data +#: directory, and a `sessions/` folder in the workspace — if there is one — +#: belongs to the user. +TRANSCRIPTS_OUTSIDE = ( + 'outside the working directory: this conversation is recorded at ' + '`{session_dir}`, and the project\'s other sessions sit beside it. Any ' + '`sessions/` directory inside the working directory is the user\'s own.') #: Filename -> template registry used by workspace_files.ensure logic. HOME_FILE_TEMPLATES = { diff --git a/ms_agent/skill/prompt_injector.py b/ms_agent/skill/prompt_injector.py index a0042c557..c411a4aea 100644 --- a/ms_agent/skill/prompt_injector.py +++ b/ms_agent/skill/prompt_injector.py @@ -5,7 +5,7 @@ class SkillPromptInjector: """Builds the skill section to inject into the system prompt.""" - SKILL_SECTION_HEADER = """# Available Skills + SKILL_SECTION_HEADER = """## Available Skills You have access to specialized skills that extend your capabilities. Each skill is a set of instructions and resources for handling specific tasks. @@ -21,7 +21,7 @@ class SkillPromptInjector: """ ALWAYS_SKILLS_HEADER = ( - '# Active Skills\n\n' + '## Active Skills\n\n' 'The following skills are always active. Follow their instructions.\n') DISCOVERY_HINT = ( diff --git a/ms_agent/tools/code/local_code_executor.py b/ms_agent/tools/code/local_code_executor.py index 576d6b9c6..c1490742d 100644 --- a/ms_agent/tools/code/local_code_executor.py +++ b/ms_agent/tools/code/local_code_executor.py @@ -563,7 +563,7 @@ async def _get_tools_inner(self) -> Dict[str, Any]: 'temp directory); reading credential files and running ' 'code inline (python -c, heredocs) may require ' 'approval. Large output is spilled to ' - '.ms_agent_artifacts and the result says where. Use ' + '.ms_agent/artifacts and the result says where. Use ' 'run_in_background=true for a long command: it returns ' 'a task_id immediately.'), parameters={ diff --git a/ms_agent/tools/mcp_client.py b/ms_agent/tools/mcp_client.py index d29ed403a..9fd7da7d0 100644 --- a/ms_agent/tools/mcp_client.py +++ b/ms_agent/tools/mcp_client.py @@ -5,6 +5,7 @@ import copy import os import re +import shutil from contextlib import AsyncExitStack, suppress from datetime import timedelta from mcp import ClientSession, ListToolsResult, StdioServerParameters @@ -61,6 +62,63 @@ def _int_env(name: str, default: int) -> int: DEFAULT_STREAMABLE_HTTP_TIMEOUT = timedelta(seconds=30) DEFAULT_STREAMABLE_HTTP_SSE_READ_TIMEOUT = timedelta(seconds=60 * 5) +#: Variables a stdio server's child process inherits from this one. The MCP +#: SDK's default child environment is deliberately tiny, which strips proxy, +#: TLS and package-index settings — a cold ``uvx `` then downloads +#: without the proxy the machine needs and runs into the startup timeout. +#: Identity, toolchain and network settings pass through; credentials do not. +#: A server's configured ``env`` is applied on top and wins per key. +_STDIO_ENV_PASSTHROUGH = ( + 'PATH', 'HOME', 'USER', 'LOGNAME', 'SHELL', 'TMPDIR', 'TERM', 'TZ', + 'LANG', 'LC_ALL', 'LC_CTYPE', + 'SSL_CERT_FILE', 'SSL_CERT_DIR', 'REQUESTS_CA_BUNDLE', + # ALL_PROXY is deliberately absent: a socks5:// value makes any child + # whose httpx lacks the socksio extra fail on its FIRST request, and the + # child's venv is not ours to fix. The scheme-specific variables cover + # the download-acceleration need without that trap. + 'HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY', + 'http_proxy', 'https_proxy', 'no_proxy', + 'UV_INDEX_URL', 'UV_DEFAULT_INDEX', 'PIP_INDEX_URL', + 'npm_config_registry', +) + +#: Where a bare stdio command is looked for when PATH does not know it. A +#: backend launched by an IDE or launchd runs with a minimal PATH while the +#: user's ``uvx``/``npx`` lives in one of these. +_STDIO_EXTRA_BIN_DIRS = ( + os.path.expanduser('~/.local/bin'), + '/opt/homebrew/bin', + '/usr/local/bin', +) + + +def stdio_child_env(config_env: Optional[dict]) -> dict: + env = { + key: os.environ[key] + for key in _STDIO_ENV_PASSTHROUGH if os.environ.get(key) + } + for key, value in (config_env or {}).items(): + env[str(key)] = str(value) + return env + + +def resolve_stdio_command(command: str) -> str: + """Absolute path for *command*, searching PATH then the usual user dirs.""" + if os.path.sep in command: + return command + found = shutil.which(command) + if found: + return found + for base in _STDIO_EXTRA_BIN_DIRS: + candidate = os.path.join(base, command) + if os.path.isfile(candidate) and os.access(candidate, os.X_OK): + return candidate + searched = ':'.join([os.environ.get('PATH', '')] + + list(_STDIO_EXTRA_BIN_DIRS)) + raise FileNotFoundError( + f"stdio MCP command '{command}' was not found on this machine. " + f'Searched: {searched}') + _PYDANTIC_DOC_LINK = re.compile(r'\s*For further information visit \S+') _PYDANTIC_FIELD_LINE = re.compile(r'^(\S+?)(?:\.\w+)?\n\s+(.+?)\s*\[type=', @@ -130,8 +188,18 @@ def __init__( async def call_tool(self, server_name: str, tool_name: str, tool_args: dict): - response = await self.sessions[server_name].call_tool( - tool_name, tool_args) + session = self.sessions.get(server_name) + if session is None: + # The server's tools stay in the tool index after its connection + # dies (a failed call tears the session down), so the model can + # still address it — and used to get a bare KeyError. Name what + # actually happened and what to do instead. + raise RuntimeError( + f"MCP server '{server_name}' is not connected (it failed or " + 'was disconnected earlier in this conversation). Its tools ' + 'are unavailable for now — do not retry this call; use a ' + 'different approach or tell the user the server is down.') + response = await session.call_tool(tool_name, tool_args) texts = [] resources = [] @@ -369,10 +437,15 @@ async def _open_session(self, stack: AsyncExitStack, server_name: str, if not args: raise ValueError( "'args' parameter is required for stdio connection") + if os.name == 'nt': + child_env = kwargs.get('env') + else: + command = resolve_stdio_command(command) + child_env = stdio_child_env(kwargs.get('env')) server_params = StdioServerParameters( command=command, args=args, - env=kwargs.get('env'), + env=child_env, encoding=kwargs.get('encoding', DEFAULT_ENCODING), encoding_error_handler=kwargs.get( 'encoding_error_handler', DEFAULT_ENCODING_ERROR_HANDLER), diff --git a/ms_agent/utils/artifact_manager.py b/ms_agent/utils/artifact_manager.py index 35ddc74b0..d669708e2 100644 --- a/ms_agent/utils/artifact_manager.py +++ b/ms_agent/utils/artifact_manager.py @@ -1,5 +1,5 @@ # Copyright (c) ModelScope Contributors. All rights reserved. -"""Spill large tool outputs to disk under output_dir/.ms_agent_artifacts/.""" +"""Spill large tool outputs to disk under output_dir/.ms_agent/artifacts/.""" from __future__ import annotations diff --git a/tests/mcp/test_stdio_env.py b/tests/mcp/test_stdio_env.py new file mode 100644 index 000000000..9bc3c88dc --- /dev/null +++ b/tests/mcp/test_stdio_env.py @@ -0,0 +1,50 @@ +"""A stdio server's child process must be spawnable and networked: the MCP +SDK's default child env strips proxy/index settings, and a backend launched +with a minimal PATH cannot see the user's uvx at all.""" +import os +from unittest import mock + +import pytest + +from ms_agent.tools.mcp_client import (stdio_child_env, resolve_stdio_command) + + +def test_child_env_carries_network_settings_but_not_credentials(): + parent = { + 'PATH': '/usr/bin', + 'HOME': '/home/tester', + 'HTTPS_PROXY': 'http://127.0.0.1:7890', + 'UV_DEFAULT_INDEX': 'https://mirror.example/simple', + 'OPENAI_API_KEY': 'must-not-leak', + } + with mock.patch.dict(os.environ, parent, clear=True): + env = stdio_child_env(None) + assert env['HTTPS_PROXY'] == 'http://127.0.0.1:7890' + assert env['UV_DEFAULT_INDEX'] == 'https://mirror.example/simple' + assert 'OPENAI_API_KEY' not in env + + # The server's configured env wins per key. + with mock.patch.dict(os.environ, parent, clear=True): + env = stdio_child_env({'HTTPS_PROXY': 'http://other:1'}) + assert env['HTTPS_PROXY'] == 'http://other:1' + + +def test_command_is_found_in_user_bin_dirs_when_path_is_minimal(tmp_path): + exe = tmp_path / 'uvx' + exe.write_text('#!/bin/sh\n') + exe.chmod(0o755) + with mock.patch.dict(os.environ, {'PATH': '/usr/bin'}, clear=True), \ + mock.patch('ms_agent.tools.mcp_client._STDIO_EXTRA_BIN_DIRS', + (str(tmp_path), )): + assert resolve_stdio_command('uvx') == str(exe) + + +def test_missing_command_names_what_was_searched(tmp_path): + with mock.patch.dict(os.environ, {'PATH': '/usr/bin'}, clear=True), \ + mock.patch('ms_agent.tools.mcp_client._STDIO_EXTRA_BIN_DIRS', + (str(tmp_path), )): + with pytest.raises(FileNotFoundError) as err: + resolve_stdio_command('no-such-tool-xyz') + message = str(err.value) + assert 'no-such-tool-xyz' in message + assert str(tmp_path) in message diff --git a/tests/prompting/test_workspace_files.py b/tests/prompting/test_workspace_files.py index dc6e4635c..e7ec98ddb 100644 --- a/tests/prompting/test_workspace_files.py +++ b/tests/prompting/test_workspace_files.py @@ -26,7 +26,7 @@ def test_pristine_templates_strip_to_empty(): def test_soul_template_is_real_content(): body = wf.strip_for_injection(builtin.SOUL_TEMPLATE) - assert body.startswith('# Who You Are') + assert body.startswith('## Who You Are') assert 'version:' not in body # frontmatter stripped diff --git a/tests/prompting/test_workspace_internals_hint.py b/tests/prompting/test_workspace_internals_hint.py index 6d1fe30af..3c8cb52f7 100644 --- a/tests/prompting/test_workspace_internals_hint.py +++ b/tests/prompting/test_workspace_internals_hint.py @@ -25,14 +25,40 @@ class _Log: return agent -def test_section_is_absent_when_records_live_elsewhere(tmp_path): - """A project opened from an existing folder keeps its transcripts outside - the working directory; there is nothing to warn about.""" +def test_section_is_absent_without_a_session_log(tmp_path): + """No sessions/ in the workspace and no log to point at: say nothing.""" workspace = tmp_path / 'plain-project' workspace.mkdir() assert _agent_for(workspace)._build_workspace_internals_section() == '' +def test_external_records_are_named_with_their_real_path(tmp_path): + """A project opened from an existing folder keeps its transcripts in the + data directory. Without saying where, the model guesses when asked — the + observed guess was a `conversations/` directory that does not exist. + + The folder here also contains a `sessions/` directory of the USER'S own: + where the log writes decides which description applies, not what the + working directory happens to contain.""" + workspace = tmp_path / 'mounted-project' + (workspace / 'sessions').mkdir(parents=True) # user's own, not ours + records = tmp_path / 'data' / 'projects' / 'p1' / 'sessions' / 'abc123' + records.mkdir(parents=True) + + section = _agent_for( + workspace, session_id='abc123', + log_dir=records)._build_workspace_internals_section() + + assert str(records) in section + assert '.ms_agent/' in section + assert 'outside the working directory' in section + assert "the user's own" in section + assert 'at the root of your working directory' not in section + # The echo warning is shared: a machine-wide search reaches the records + # wherever they live. + assert 'match your own conversation' in section + + def test_section_names_the_directories_and_this_session(tmp_path): workspace = tmp_path / 'managed-project' (workspace / 'sessions' / 'abc123').mkdir(parents=True) @@ -44,8 +70,8 @@ def test_section_names_the_directories_and_this_session(tmp_path): assert '.ms_agent/' in section assert 'sessions/abc123/' in section # The reason the agent needs this at all: its own prompt is already on - # disk, so searching for a phrase from the request matches the transcript. - assert 'searching' in section + # disk, so a search for a phrase from the request finds the transcript. + assert 'match your own conversation' in section def test_the_named_directory_is_the_one_being_written_to(tmp_path):