lllyfff 99c926b7bb
fix(mcp): bring-up has no timeout and externalized tool outputs are counted as undelivered artifacts (#4657)
* fix: bound MCP server bring-up timeouts and exclude externalized tool outputs from delivery verification

Two related robustness fixes:

1. MCP server bring-up was unbounded. tool_call_timeout only covered
   session.call_tool(); tool discovery (subprocess spawn + initialize +
   tools/list) and persistent stdio session initialization could hang
   forever, blocking agent construction (and on the Gateway event loop,
   the whole process). Add a per-server session_init_timeout
   (default DEFAULT_MCP_SESSION_INIT_TIMEOUT = 60s, null disables) that
   bounds both discovery and pooled-session initialization. The session
   pool's existing cancellation handling tears down a session stuck
   mid-creation in its own task.

2. ToolOutputBudgetMiddleware externalizes oversized tool outputs into
   outputs/.tool-results/ (configurable tool_output.storage_subdir). The
   workspace-change scanner and run delivery verification counted those
   files as produced artifacts, so any run that externalized a tool output
   without also presenting a real artifact failed with
   "Artifact delivery incomplete". Exclude TOOL_RESULTS_DIRNAME via a
   shared constant (mirroring BROWSER_FRAMES_DIRNAME) and thread the
   configured storage_subdir through snapshot capture so both
   workspace-changes events and delivery verification stay clean.

* review: enforce single-segment tool_output.storage_subdir; document discovery-timeout cleanup

Address review feedback:

1. A custom tool_output.storage_subdir with a path separator (e.g.
   cache/tool-results) silently no-oped the workspace-scanner exclusion:
   os.walk yields one-segment dirnames, so a nested value never matched and
   its files were counted as produced artifacts again. ToolOutputConfig now
   validates storage_subdir as a single directory name (rejects separators,
   .., absolute, empty) with tests, so the exclusion is always sound.

2. The discovery-timeout path now documents why cancellation is safe, mirroring
   the session-init note: discovery runs inside the adapter's nested async
   context managers, and stdio_client's finally terminates the process tree
   (SIGTERM->SIGKILL on POSIX, process-tree on Windows), so a timed-out npx
   subprocess and its children are reaped rather than accumulating.

* review: log session-init timeouts and align API response model default with runtime config

Address second-round review feedback:

1. A session-init timeout raised TimeoutError without any log, unlike the
   discovery timeout which logs a WARNING. Wrap the bounded get_session in a
   try/except that logs the timeout (server name + seconds) and re-raises, so
   operators can diagnose tool-call failures caused by hung MCP sessions.

2. McpServerConfigResponse.session_init_timeout defaulted to None while
   McpServerConfig defaults to 60s: a server created via PUT /api/mcp/config
   without the field was persisted with null (no timeout) while the same
   server created in the config file got 60s. Align the response-model default
   to DEFAULT_MCP_SESSION_INIT_TIMEOUT so API-created and file-created servers
   behave the same; an explicit null still opts out.

* review: narrow the discovery-timeout handler to the bounded wait_for path

The except TimeoutError clause covered both the bounded wait_for branch and
the bare discovery branch. With session_init_timeout opted out (None), a
TimeoutError raised by discovery itself would hit the %.1f format with None:
logging raises TypeError internally, the WARNING is silently dropped, and a
--- Logging error --- traceback goes to stderr.

Narrow the handler to wrap only the wait_for call, where the branch condition
guarantees the timeout value is not None. A discovery-internal TimeoutError on
the opted-out path now falls through to the generic failure handler and is
reported as 'tool discovery failed' with exc_info. Covered by a regression
test that asserts the skip is reported without any broken format.
2026-08-05 08:56:18 +08:00

346 lines
10 KiB
Python

from __future__ import annotations
import fnmatch
import hashlib
import os
from codecs import BOM_UTF16_BE, BOM_UTF16_LE, getincrementaldecoder
from pathlib import Path
from deerflow.constants import BROWSER_FRAMES_DIRNAME, TOOL_RESULTS_DIRNAME
from .types import (
DiffUnavailableReason,
FileSnapshot,
WorkspaceChangeLimits,
WorkspaceRoot,
WorkspaceSnapshot,
)
EXCLUDED_DIR_NAMES = {
".git",
".hg",
".svn",
".cache",
".next",
".venv",
# Transient per-step browser screenshots: live progress feedback surfaced in
# the browser panel + inline thumbnails, not workspace deliverables. Shared
# constant with the browser tools so the name cannot drift out of sync.
BROWSER_FRAMES_DIRNAME,
# Externalized oversized tool outputs (the tool-output budget middleware's
# default storage_subdir): process feedback the model reads back via
# read_file, not workspace deliverables — same intent as the browser frames
# exclusion above. Without this, a run that externalizes any tool output
# would trip run delivery verification (produced output never presented)
# and fail as an error. Custom storage_subdir values are passed through
# ``extra_excluded_dir_names`` instead.
TOOL_RESULTS_DIRNAME,
"__pycache__",
"build",
"dist",
"node_modules",
}
BINARY_EXTENSIONS = {
".7z",
".avif",
".bmp",
".class",
".db",
".dll",
".dmg",
".doc",
".docx",
".exe",
".gif",
".gz",
".ico",
".jar",
".jpeg",
".jpg",
".mov",
".mp3",
".mp4",
".o",
".pdf",
".png",
".pyc",
".so",
".tar",
".webp",
".xls",
".xlsx",
".zip",
}
SENSITIVE_PATH_PATTERNS = (
".env",
".env.*",
"*api_key*",
"*apikey*",
"*.key",
"*.pem",
"*credential*",
"*password*",
"*private_key*",
"*secret*",
"*token*",
)
SAMPLE_BYTES = 4096
_UTF16_BOMS = (BOM_UTF16_LE, BOM_UTF16_BE)
def is_sensitive_workspace_path(path: str) -> bool:
normalized = path.lower()
parts = [part.lower() for part in Path(path).parts]
basename = parts[-1] if parts else normalized
for pattern in SENSITIVE_PATH_PATTERNS:
if fnmatch.fnmatch(basename, pattern) or fnmatch.fnmatch(normalized, pattern):
return True
if any(fnmatch.fnmatch(part, pattern) for part in parts):
return True
return False
def scan_workspace_roots(
roots: list[WorkspaceRoot],
*,
limits: WorkspaceChangeLimits | None = None,
include_text: bool = True,
text_paths: set[str] | None = None,
text_cache_dir: Path | None = None,
extra_excluded_dir_names: frozenset[str] | None = None,
) -> WorkspaceSnapshot:
resolved_limits = limits or WorkspaceChangeLimits()
cache_dir = Path(text_cache_dir) if text_cache_dir is not None else None
if cache_dir is not None:
cache_dir.mkdir(parents=True, exist_ok=True)
# Operator-customized tool_output.storage_subdir values arrive here; the
# default name is already part of EXCLUDED_DIR_NAMES, so merging is safe.
# Only single-segment directory names are meaningful: os.walk yields
# one-segment dirnames, so a nested value like "cache/tool-results" would
# never match. ToolOutputConfig enforces the single-segment contract, so a
# multi-segment value is a caller error, not a silent no-op.
excluded_dir_names = EXCLUDED_DIR_NAMES | extra_excluded_dir_names if extra_excluded_dir_names else EXCLUDED_DIR_NAMES
files: dict[str, FileSnapshot] = {}
scanned = 0
truncated = False
for root in roots:
if not root.host_path.exists():
continue
for dirpath, dirnames, filenames in os.walk(root.host_path, followlinks=False):
dirnames[:] = [dirname for dirname in dirnames if dirname not in excluded_dir_names and not (Path(dirpath) / dirname).is_symlink()]
for filename in sorted(filenames):
if scanned >= resolved_limits.max_scanned_files:
truncated = True
return WorkspaceSnapshot(
files=files,
truncated=truncated,
text_cache_dir=str(cache_dir) if cache_dir is not None else None,
)
host_file = Path(dirpath) / filename
if host_file.is_symlink():
# A symlink must never be followed for stat/content purposes: its
# target can point anywhere on the host (including outside the
# scanned root), so it is recorded as a metadata-only stub -
# mirroring how binary/large/sensitive-looking files are handled
# below - instead of being silently omitted from the snapshot.
symlink_snapshot = _snapshot_symlink(root, host_file)
if symlink_snapshot is not None:
files[symlink_snapshot.path] = symlink_snapshot
scanned += 1
continue
if not host_file.is_file():
continue
snapshot = _snapshot_file(
root,
host_file,
limits=resolved_limits,
include_text=include_text,
text_paths=text_paths,
text_cache_dir=cache_dir,
)
if snapshot is not None:
files[snapshot.path] = snapshot
scanned += 1
return WorkspaceSnapshot(
files=files,
truncated=truncated,
text_cache_dir=str(cache_dir) if cache_dir is not None else None,
)
def _snapshot_file(
root: WorkspaceRoot,
host_file: Path,
*,
limits: WorkspaceChangeLimits,
include_text: bool,
text_paths: set[str] | None,
text_cache_dir: Path | None,
) -> FileSnapshot | None:
try:
stat = host_file.stat()
size = stat.st_size
mtime_ns = stat.st_mtime_ns
relative = host_file.relative_to(root.host_path).as_posix()
virtual_path = f"{root.virtual_prefix}/{relative}"
sensitive = is_sensitive_workspace_path(virtual_path)
except OSError:
return None
if sensitive:
return FileSnapshot(
path=virtual_path,
root=root.name,
size=size,
mtime_ns=mtime_ns,
sha256=None,
binary=False,
sensitive=True,
text=None,
content_unavailable_reason="sensitive",
)
try:
sample = host_file.read_bytes()[:SAMPLE_BYTES] if size <= SAMPLE_BYTES else _read_sample(host_file)
except OSError:
return None
binary = host_file.suffix.lower() in BINARY_EXTENSIONS or _looks_binary(sample)
sha256 = _sha256_file(host_file) if size <= limits.max_file_bytes_for_diff else None
text: str | None = None
text_path: str | None = None
reason: DiffUnavailableReason | None = None
should_include_text = include_text and (text_paths is None or virtual_path in text_paths)
if binary:
reason = "binary"
elif size > limits.max_file_bytes_for_diff:
reason = "large"
elif not should_include_text:
text = None
else:
try:
raw = host_file.read_bytes()
except OSError:
return None
decoded = _decode_text_bytes(raw)
if decoded is None:
binary = True
reason = "binary"
elif text_cache_dir is not None:
text_path = str(_cache_text_file(decoded, virtual_path, text_cache_dir))
else:
text = decoded
return FileSnapshot(
path=virtual_path,
root=root.name,
size=size,
mtime_ns=mtime_ns,
sha256=sha256,
binary=binary,
sensitive=sensitive,
text=text,
text_path=text_path,
content_unavailable_reason=reason,
)
def _snapshot_symlink(root: WorkspaceRoot, host_file: Path) -> FileSnapshot | None:
# Deliberately never follows the link (no read_bytes()/open() on the target):
# the target may point anywhere on the host, including outside the scanned
# root, so stat'ing or reading through it here would risk exposing arbitrary
# host file content/metadata as if it belonged to the workspace.
try:
stat = host_file.lstat()
size = stat.st_size
mtime_ns = stat.st_mtime_ns
relative = host_file.relative_to(root.host_path).as_posix()
virtual_path = f"{root.virtual_prefix}/{relative}"
sensitive = is_sensitive_workspace_path(virtual_path)
except OSError:
return None
try:
target = os.readlink(host_file)
except OSError:
target = None
return FileSnapshot(
path=virtual_path,
root=root.name,
size=size,
mtime_ns=mtime_ns,
sha256=None,
binary=False,
sensitive=sensitive,
text=None,
content_unavailable_reason="symlink",
symlink=True,
symlink_target=target,
)
def _cache_text_file(text: str, virtual_path: str, cache_dir: Path) -> Path:
cache_name = hashlib.sha256(virtual_path.encode("utf-8")).hexdigest()
target = cache_dir / cache_name
target.write_text(text, encoding="utf-8")
return target
def _read_sample(path: Path) -> bytes:
with path.open("rb") as file:
return file.read(SAMPLE_BYTES)
def _sha256_file(path: Path) -> str:
digest = hashlib.sha256()
with path.open("rb") as file:
for chunk in iter(lambda: file.read(1024 * 1024), b""):
digest.update(chunk)
return digest.hexdigest()
def _decode_text_bytes(data: bytes) -> str | None:
for encoding in ("utf-8-sig", "utf-8"):
try:
return data.decode(encoding)
except UnicodeDecodeError:
continue
if data.startswith(_UTF16_BOMS):
try:
return data.decode("utf-16")
except UnicodeDecodeError:
return None
return None
def _sample_decodes_as_text(sample: bytes, encoding: str) -> bool:
try:
decoder = getincrementaldecoder(encoding)()
decoder.decode(sample, final=False)
except UnicodeDecodeError:
return False
return True
def _looks_binary(sample: bytes) -> bool:
if sample.startswith(_UTF16_BOMS) and _sample_decodes_as_text(sample, "utf-16"):
return False
if b"\x00" in sample:
return True
if _sample_decodes_as_text(sample, "utf-8"):
return False
return True