[codex] Sanitize empty optional MCP arguments#7160
[codex] Sanitize empty optional MCP arguments#7160Tobi1chi wants to merge 4 commits intoAstrBotDevs:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a recursive sanitization function for MCP tool arguments to remove null values, empty strings, and empty collections. A review comment identifies an inconsistency where empty dictionaries are not handled the same as empty lists and suggests a more idiomatic implementation using comprehensions.
dosubot
bot
added
the
size:L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
call_tool_with_reconnect,sanitized_argumentsis coerced to{}when the sanitizer returns_EMPTY_MCP_ARGUMENT, which will change the type for non-dict inputs (e.g.,Noneor empty string); consider only defaulting to{}when the originalargumentsis a mapping or leaving non-mapping arguments asNone/their original type after sanitization. - The test helper
load_mcp_client_modulerelies onMCP_CLIENT_MODULE_PATH = REPO_ROOT / "astrbot/core/agent/mcp_client.py"and manual module construction, which is quite brittle; consider importingastrbot.core.agent.mcp_clientnormally and mocking its dependencies instead of using a path-based dynamic loader.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `call_tool_with_reconnect`, `sanitized_arguments` is coerced to `{}` when the sanitizer returns `_EMPTY_MCP_ARGUMENT`, which will change the type for non-dict inputs (e.g., `None` or empty string); consider only defaulting to `{}` when the original `arguments` is a mapping or leaving non-mapping arguments as `None`/their original type after sanitization.
- The test helper `load_mcp_client_module` relies on `MCP_CLIENT_MODULE_PATH = REPO_ROOT / "astrbot/core/agent/mcp_client.py"` and manual module construction, which is quite brittle; consider importing `astrbot.core.agent.mcp_client` normally and mocking its dependencies instead of using a path-based dynamic loader.
## Individual Comments
### Comment 1
<location path="astrbot/core/agent/mcp_client.py" line_range="385-390" />
<code_context>
+ sanitized_arguments = _sanitize_mcp_arguments(arguments)
+ if sanitized_arguments is _EMPTY_MCP_ARGUMENT:
+ sanitized_arguments = {}
+ if sanitized_arguments != arguments:
+ logger.debug(
+ "Sanitized MCP tool %s arguments from %s to %s",
+ tool_name,
+ arguments,
+ sanitized_arguments,
+ )
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Debug logging of full arguments may expose sensitive data and be expensive
This logs both the raw and sanitized arguments, which can leak credentials/PII into logs and adds overhead for large payloads. Consider redacting known sensitive fields, truncating large values, or guarding this behind a more verbose/debug-only flag so it’s not enabled in typical deployments.
</issue_to_address>
### Comment 2
<location path="tests/unit/test_mcp_client.py" line_range="95-104" />
<code_context>
+ return module
+
+
+def test_sanitize_mcp_arguments_removes_nested_empty_collections():
+ mcp_client_module = load_mcp_client_module()
+
+ sanitized = mcp_client_module._sanitize_mcp_arguments(
+ {
+ "query": "hello",
+ "filters": {"tags": [], "scope": {}},
+ "metadata": {"owner": "", "visibility": None},
+ }
+ )
+
+ assert sanitized == {"query": "hello"}
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add more unit tests for `_sanitize_mcp_arguments` to cover top-level empties, lists, and non-empty scalar passthrough.
This test covers the nested dict/list case, but `_sanitize_mcp_arguments` has other behaviors that should be exercised:
- Top-level empties (`None`, `""`, `[]`, `{}`) returning the empty sentinel (and ultimately `{}` at the call site).
- Mixed lists (e.g. `["", None, "x", {"a": ""}]`) to verify only empty elements are removed.
- Lists of dicts where some dicts become empty and are dropped vs preserved.
- Scalar passthrough (numbers, booleans, non-empty strings) unchanged.
A small `@pytest.mark.parametrize("value, expected", [...])` test would cover these cases without much duplication.
</issue_to_address>
### Comment 3
<location path="tests/unit/test_mcp_client.py" line_range="109-118" />
<code_context>
+ assert sanitized == {"query": "hello"}
+
+
+@pytest.mark.asyncio
+async def test_call_tool_with_reconnect_falls_back_to_empty_top_level_arguments():
+ mcp_client_module = load_mcp_client_module()
+
+ client = mcp_client_module.MCPClient()
+ client.session = types.SimpleNamespace(call_tool=AsyncMock(return_value="ok"))
+
+ result = await client.call_tool_with_reconnect(
+ tool_name="search",
+ arguments={"filters": {}, "query": ""},
+ read_timeout_seconds=mcp_client_module.timedelta(seconds=1),
+ )
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend `call_tool_with_reconnect` tests to cover the retry path and ensure sanitized arguments are used on all attempts.
This test covers the empty-arguments case, but it doesn’t exercise the retry path. Please add an async test where `session.call_tool` raises `anyio.ClosedResourceError` on the first call and succeeds on the second (e.g., `side_effect=[ClosedResourceError(), "ok"]`), asserting that:
- The retry actually occurs.
- The same sanitized payload is passed on both attempts.
Optionally, also simulate `client.session` being replaced between attempts to confirm sanitized arguments are still used with the new session instance.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if sanitized_arguments != arguments: | ||
| logger.debug( | ||
| "Sanitized MCP tool %s arguments from %s to %s", | ||
| tool_name, | ||
| arguments, | ||
| sanitized_arguments, |
There was a problem hiding this comment.
🚨 suggestion (security): Debug logging of full arguments may expose sensitive data and be expensive
This logs both the raw and sanitized arguments, which can leak credentials/PII into logs and adds overhead for large payloads. Consider redacting known sensitive fields, truncating large values, or guarding this behind a more verbose/debug-only flag so it’s not enabled in typical deployments.
|
原issue: #7116 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4200317e2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4200317e2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4200317e2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97dc7c87ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| anyio_module = types.ModuleType("anyio") | ||
| anyio_module.ClosedResourceError = type("ClosedResourceError", (Exception,), {}) | ||
| sys.modules["anyio"] = anyio_module | ||
|
|
There was a problem hiding this comment.
load_mcp_client_module() writes fake modules into sys.modules and never restores them, so once this helper runs, later tests in the same pytest process can import these incomplete stubs (anyio, mcp, and related astrbot.* entries) instead of real modules, causing order-dependent failures or hiding real regressions. Please patch these entries via monkeypatch (or equivalent teardown) so global import state is restored after each test.
Useful? React with 👍 / 👎.
1 participant
Summary
Why
Some MCP servers reject requests when optional arguments are present but empty. This change strips empty placeholder values before the tool call so those optional fields are omitted instead of being sent as invalid payload.
Validation
./.venv-precommit/bin/pre-commit run --from-ref upstream/master --to-ref HEADSummary by Sourcery
Sanitize MCP tool call arguments to strip empty optional values before dispatch and log any changes while preserving the reconnect-and-retry behavior.
Bug Fixes:
Enhancements:
Tests: