mirror of
https://github.com/langchain-ai/deepagents.git
synced 2026-07-22 01:35:28 -04:00
fix(code): clearer MCP config JSON parse errors (#4327)
Deep Agents Code now shows a clearer, actionable error (with a hint and a caret pointing at the offending line) when an MCP config file contains invalid JSON. --- When an MCP config file (e.g. `~/.deepagents/.mcp.json`) contains invalid JSON, the error now surfaces an actionable hint (trailing comma, `//`/`/*` comments, missing value/delimiter/property name) and a caret snippet pointing at the offending line, instead of just the raw decoder message. The error is still a `json.JSONDecodeError`, so the file path is added by existing callers and the multi-line text renders in the `/mcp` error modal. Made by [Open SWE](https://openswe.vercel.app/agents/cd550fdd-ee5a-3c41-a8ea-5dcd09b8c06f) ## References - Plan: https://openswe.vercel.app/agents/cd550fdd-ee5a-3c41-a8ea-5dcd09b8c06f/plan --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
This commit is contained in:
Mason Daugherty
@@ -604,6 +604,112 @@ def _validate_tool_filter_fields(
|
||||
raise ValueError(error_msg)
|
||||
|
||||
|
||||
def _looks_like_comment(doc: str, lineno: int) -> bool:
|
||||
"""Return `True` if the offending line *begins* with `//` or `/*`.
|
||||
|
||||
Only the failing line is checked, and only its leading characters (after
|
||||
stripping indentation). A `url` value such as `"url": "https://..."`
|
||||
begins with a quote, not `//`, so a URL scheme inside a quoted string
|
||||
never triggers a false comment hint.
|
||||
|
||||
Args:
|
||||
doc: Full source text that failed to parse.
|
||||
lineno: 1-based line number of the error; out-of-range values
|
||||
return `False`.
|
||||
|
||||
Returns:
|
||||
`True` when the stripped failing line starts with `//` or `/*`.
|
||||
"""
|
||||
lines = doc.splitlines()
|
||||
if lineno < 1 or lineno > len(lines):
|
||||
return False
|
||||
stripped = lines[lineno - 1].lstrip()
|
||||
return stripped.startswith(("//", "/*"))
|
||||
|
||||
|
||||
def _json_error_hint(exc: json.JSONDecodeError) -> str | None:
|
||||
"""Return an actionable hint for a common JSON mistake, or `None`.
|
||||
|
||||
Checks are ordered most-specific-first (trailing comma, then comment,
|
||||
then generic decoder-message keywords) so a more precise hint wins when
|
||||
several could apply.
|
||||
|
||||
Args:
|
||||
exc: The decode error to classify.
|
||||
|
||||
Returns:
|
||||
A hint string for a recognized mistake, or `None` when no specific
|
||||
guidance applies.
|
||||
"""
|
||||
msg = exc.msg.lower()
|
||||
if "trailing comma" in msg:
|
||||
return (
|
||||
"Hint: JSON does not allow trailing commas. Remove the comma "
|
||||
"before the closing '}' or ']'."
|
||||
)
|
||||
if _looks_like_comment(exc.doc, exc.lineno):
|
||||
return "Hint: JSON does not allow comments (// or /* */). Remove them."
|
||||
if "expecting property name" in msg:
|
||||
return (
|
||||
"Hint: check for trailing commas, a missing key, or an unquoted "
|
||||
"property name near this position."
|
||||
)
|
||||
if "expecting value" in msg:
|
||||
return (
|
||||
"Hint: check for a missing value, an extra comma, or unquoted text "
|
||||
"near this position."
|
||||
)
|
||||
if "delimiter" in msg:
|
||||
return (
|
||||
"Hint: check for a missing comma, ':', or closing bracket near "
|
||||
"this position."
|
||||
)
|
||||
return None
|
||||
|
||||
|
||||
def _trailing_comma_pos(doc: str, pos: int) -> int | None:
|
||||
"""Return the comma position for decoder errors at a trailing comma."""
|
||||
if pos < 0 or pos >= len(doc) or doc[pos] not in "}]":
|
||||
return None
|
||||
idx = pos - 1
|
||||
while idx >= 0 and doc[idx].isspace():
|
||||
idx -= 1
|
||||
if idx >= 0 and doc[idx] == ",":
|
||||
return idx
|
||||
return None
|
||||
|
||||
|
||||
def _json_error_snippet(
|
||||
doc: str, lineno: int, colno: int, *, pos: int | None = None
|
||||
) -> str | None:
|
||||
"""Build a caret snippet pointing at a JSON error location.
|
||||
|
||||
Args:
|
||||
doc: Full source text that failed to parse.
|
||||
lineno: 1-based line number of the error.
|
||||
colno: 1-based column number of the error.
|
||||
pos: 0-based absolute error offset, if available.
|
||||
|
||||
Returns:
|
||||
A two-line `<source line>` + caret string, or `None` when the line
|
||||
is out of range or blank.
|
||||
"""
|
||||
if pos is not None:
|
||||
trailing_pos = _trailing_comma_pos(doc, pos)
|
||||
if trailing_pos is not None:
|
||||
lineno = doc.count("\n", 0, trailing_pos) + 1
|
||||
line_start = doc.rfind("\n", 0, trailing_pos) + 1
|
||||
colno = trailing_pos - line_start + 1
|
||||
lines = doc.splitlines()
|
||||
if lineno < 1 or lineno > len(lines):
|
||||
return None
|
||||
source = lines[lineno - 1].rstrip()
|
||||
if not source:
|
||||
return None
|
||||
caret_col = max(0, min(colno - 1, len(source)))
|
||||
return f" {source}\n {' ' * caret_col}^"
|
||||
|
||||
|
||||
def load_mcp_config(config_path: str) -> dict[str, Any]:
|
||||
"""Load and validate MCP configuration from a JSON file.
|
||||
|
||||
@@ -647,7 +753,17 @@ def load_mcp_config(config_path: str) -> dict[str, Any]:
|
||||
with path.open(encoding="utf-8") as file_obj:
|
||||
config = json.load(file_obj)
|
||||
except json.JSONDecodeError as exc:
|
||||
error_msg = f"Invalid JSON in MCP config file: {exc.msg}"
|
||||
# Build a layered message: core reason, an actionable hint for common
|
||||
# mistakes, then a caret snippet last so the auto-appended
|
||||
# "line X column Y" suffix reads as the location of the caret.
|
||||
parts = [f"Invalid JSON in MCP config file: {exc.msg}"]
|
||||
hint = _json_error_hint(exc)
|
||||
if hint is not None:
|
||||
parts.append(hint)
|
||||
snippet = _json_error_snippet(exc.doc, exc.lineno, exc.colno, pos=exc.pos)
|
||||
if snippet is not None:
|
||||
parts.append(snippet)
|
||||
error_msg = "\n".join(parts)
|
||||
raise json.JSONDecodeError(error_msg, exc.doc, exc.pos) from exc
|
||||
|
||||
if "mcpServers" not in config:
|
||||
|
||||
@@ -24,6 +24,7 @@ from deepagents_code.mcp_tools import (
|
||||
_apply_tool_filter,
|
||||
_check_remote_server,
|
||||
_check_stdio_server,
|
||||
_json_error_snippet,
|
||||
_load_tools_from_config,
|
||||
_normalize_mcp_arguments,
|
||||
classify_discovered_configs,
|
||||
@@ -317,6 +318,136 @@ class TestLoadMCPConfig:
|
||||
with pytest.raises(json.JSONDecodeError):
|
||||
load_mcp_config(str(path))
|
||||
|
||||
def test_trailing_comma_error_has_hint_and_snippet(self, tmp_path: Path) -> None:
|
||||
"""A trailing comma surfaces an actionable hint plus a caret snippet."""
|
||||
path = tmp_path / "bad.json"
|
||||
path.write_text(
|
||||
'{\n "mcpServers": {\n "fs": {\n "command": "x",\n },\n }\n}'
|
||||
)
|
||||
with pytest.raises(json.JSONDecodeError) as exc_info:
|
||||
load_mcp_config(str(path))
|
||||
message = str(exc_info.value)
|
||||
assert "Invalid JSON in MCP config file" in message
|
||||
assert "trailing commas" in message
|
||||
assert "line" in message
|
||||
assert "column" in message
|
||||
# The caret must point at the offending comma, not merely be present:
|
||||
# find the caret line and the source line above it (both share the
|
||||
# same indent), then confirm the character under the `^` is the comma.
|
||||
msg_lines = message.splitlines()
|
||||
caret_idx = next(
|
||||
i for i, line in enumerate(msg_lines) if line.lstrip().startswith("^")
|
||||
)
|
||||
source_line = msg_lines[caret_idx - 1]
|
||||
caret_col = msg_lines[caret_idx].index("^")
|
||||
assert source_line[caret_col] == ","
|
||||
|
||||
def test_missing_value_error_keeps_decoder_caret(self, tmp_path: Path) -> None:
|
||||
"""A missing value keeps the caret at the decoder-reported token."""
|
||||
path = tmp_path / "bad.json"
|
||||
path.write_text('{"mcpServers": {"fs": {"command": }, "other": {}}}')
|
||||
with pytest.raises(json.JSONDecodeError) as exc_info:
|
||||
load_mcp_config(str(path))
|
||||
message = str(exc_info.value)
|
||||
assert "missing value" in message
|
||||
msg_lines = message.splitlines()
|
||||
caret_idx = next(
|
||||
i for i, line in enumerate(msg_lines) if line.lstrip().startswith("^")
|
||||
)
|
||||
source_line = msg_lines[caret_idx - 1]
|
||||
caret_col = msg_lines[caret_idx].index("^")
|
||||
assert source_line[caret_col] == "}"
|
||||
|
||||
def test_comment_error_has_hint(self, tmp_path: Path) -> None:
|
||||
"""A JSON file with a comment surfaces a comment-specific hint.
|
||||
|
||||
The underlying decoder message is "Expecting property name...", so a
|
||||
passing assertion proves the comment heuristic fired and won the
|
||||
ordering rather than the generic property-name branch.
|
||||
"""
|
||||
path = tmp_path / "bad.json"
|
||||
path.write_text('{\n // not allowed\n "mcpServers": {}\n}')
|
||||
with pytest.raises(json.JSONDecodeError) as exc_info:
|
||||
load_mcp_config(str(path))
|
||||
message = str(exc_info.value)
|
||||
assert "comments" in message
|
||||
# The comment hint must win over the generic property-name hint;
|
||||
# "missing key" is unique to that hint and absent from both the raw
|
||||
# decoder message and the comment hint.
|
||||
assert "missing key" not in message
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("content", "expected_hint_fragment"),
|
||||
[
|
||||
# "Expecting value" -> missing-value hint.
|
||||
('{"mcpServers": }', "missing value"),
|
||||
# "Expecting property name..." (unquoted key) -> property-name hint.
|
||||
("{\n mcpServers: {}\n}", "property name"),
|
||||
# "Expecting ',' delimiter" -> missing-comma hint.
|
||||
('{"a": 1 "b": 2}', "missing comma"),
|
||||
],
|
||||
)
|
||||
def test_json_error_hint_branches(
|
||||
self, tmp_path: Path, content: str, expected_hint_fragment: str
|
||||
) -> None:
|
||||
"""Each recognized decoder message yields its specific hint."""
|
||||
path = tmp_path / "bad.json"
|
||||
path.write_text(content)
|
||||
with pytest.raises(json.JSONDecodeError) as exc_info:
|
||||
load_mcp_config(str(path))
|
||||
assert expected_hint_fragment in str(exc_info.value)
|
||||
|
||||
def test_url_scheme_does_not_trigger_comment_hint(self, tmp_path: Path) -> None:
|
||||
"""A `://` URL on the failing line is not mistaken for a comment.
|
||||
|
||||
This is the entire reason `_looks_like_comment` checks `startswith`
|
||||
rather than substring containment; the guard must stay covered so a
|
||||
refactor cannot silently emit a bogus comment hint on URL configs.
|
||||
"""
|
||||
path = tmp_path / "bad.json"
|
||||
# Missing comma after the URL value, so the error lands on the URL line.
|
||||
path.write_text(
|
||||
'{\n "mcpServers": {\n "remote": {\n'
|
||||
' "url": "https://example.com" "type": "http"\n'
|
||||
" }\n }\n}"
|
||||
)
|
||||
with pytest.raises(json.JSONDecodeError) as exc_info:
|
||||
load_mcp_config(str(path))
|
||||
message = str(exc_info.value)
|
||||
assert "comments" not in message
|
||||
# The URL line is rendered in the snippet and treated as a delimiter
|
||||
# error, not a comment.
|
||||
assert "https://" in message
|
||||
assert "missing comma" in message
|
||||
|
||||
def test_unrecognized_error_has_no_hint(self, tmp_path: Path) -> None:
|
||||
"""An error matching no known pattern omits the hint line entirely."""
|
||||
path = tmp_path / "bad.json"
|
||||
path.write_text('{"mcpServers": {"fs": {"command": "x}}')
|
||||
with pytest.raises(json.JSONDecodeError) as exc_info:
|
||||
load_mcp_config(str(path))
|
||||
message = str(exc_info.value)
|
||||
assert "Invalid JSON in MCP config file" in message
|
||||
assert "Hint:" not in message
|
||||
|
||||
def test_json_error_snippet_blank_line_returns_none(self) -> None:
|
||||
"""A blank failing line yields no snippet (avoids a bare caret)."""
|
||||
assert _json_error_snippet("{\n\n}", 2, 1) is None
|
||||
|
||||
def test_json_error_snippet_out_of_range_returns_none(self) -> None:
|
||||
"""A line number past the source (e.g. truncated input) yields None."""
|
||||
assert _json_error_snippet("{}", 5, 1) is None
|
||||
|
||||
def test_json_error_snippet_clamps_caret_to_line_end(self) -> None:
|
||||
"""A column past the line length pins the caret to the line end."""
|
||||
source = ' "abc"'
|
||||
snippet = _json_error_snippet(source, 1, 999)
|
||||
assert snippet is not None
|
||||
caret_line = snippet.splitlines()[1]
|
||||
# Snippet lines carry a 4-space indent; the caret offset within the
|
||||
# source text must not exceed its length.
|
||||
assert caret_line.index("^") - 4 == len(source)
|
||||
|
||||
def test_missing_mcpservers_field(self, write_config: Callable[..., str]) -> None:
|
||||
"""Config without `mcpServers` field is rejected."""
|
||||
path = write_config({"other": {}})
|
||||
|
||||
Reference in New Issue
Block a user