[fix] Correct the env-var name in the MCP-disabled error (AGENTA_AGENT_MCP_SERVERS_ENABLED)#4899
Conversation
…T_MCP_SERVERS_ENABLED) The MCPDisabledError message and the runner SSRF-guard comment told users to set AGENTA_AGENT_ENABLE_MCP to enable MCP servers. The runner actually gates MCP on AGENTA_AGENT_MCP_SERVERS_ENABLED (services/oss/src/agent/tools/resolver.py), so a user following the remediation set a no-op var and MCP stayed disabled. Correct the env-var name in both the error message and the comment. Text only, no behavior change. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
dosubot
Bot
added
the
size:XS
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughTwo files rename the MCP capability flag from MCP Flag Rename
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🤖 The AI agent says: Preserving the F-047 fix from agent-workflows QA. The MCP-disabled error and the runner SSRF-guard comment told users to set Changed (2 files):
What I need from you: a quick code review of a text-only fix. Confirm One call to make: I kept the commit to the two user-facing files. So the existing test ( Please do not merge. This is left for the orchestrator to fold back into |
| super().__init__( | ||
| "MCP servers are disabled on this deployment " | ||
| "(set AGENTA_AGENT_ENABLE_MCP to enable them), but the request declared " | ||
| "(set AGENTA_AGENT_MCP_SERVERS_ENABLED to enable them), but the request declared " |
There was a problem hiding this comment.
🤖 The AI agent says: F-047 fix. This MCPDisabledError message named AGENTA_AGENT_ENABLE_MCP, which nothing reads. It now names the real gate AGENTA_AGENT_MCP_SERVERS_ENABLED (read at services/oss/src/agent/tools/resolver.py:24).
| * as request headers to this author-supplied URL, so an attacker-controlled config could point it | ||
| * at an internal/metadata endpoint and exfiltrate a credential (a classic server-side request | ||
| * forgery). The capability is flag-gated (`AGENTA_AGENT_ENABLE_MCP`, off by default) and | ||
| * forgery). The capability is flag-gated (`AGENTA_AGENT_MCP_SERVERS_ENABLED`, off by default) and |
There was a problem hiding this comment.
🤖 The AI agent says: Same F-047 fix in the runner SSRF-guard comment: the flag is AGENTA_AGENT_MCP_SERVERS_ENABLED, not AGENTA_AGENT_ENABLE_MCP. Comment text only, no behavior change.
|
🤖 The AI agent says: @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Find target files =="
fd -a 'resolver.py|errors.py|mcp.ts|test_resolution.py|run_matrix.py' .
echo
echo "== Outline resolver.py =="
ast-grep outline services/oss/src/agent/tools/resolver.py --view expanded || true
echo
echo "== Relevant slice: services/oss/src/agent/tools/resolver.py =="
sed -n '1,120p' services/oss/src/agent/tools/resolver.py
echo
echo "== Outline errors.py =="
ast-grep outline sdks/python/agenta/sdk/agents/mcp/errors.py --view expanded || true
echo
echo "== Full file: sdks/python/agenta/sdk/agents/mcp/errors.py =="
sed -n '1,200p' sdks/python/agenta/sdk/agents/mcp/errors.py
echo
echo "== Outline mcp.ts =="
ast-grep outline services/agent/src/engines/sandbox_agent/mcp.ts --view expanded || true
echo
echo "== Full file: services/agent/src/engines/sandbox_agent/mcp.ts =="
sed -n '1,220p' services/agent/src/engines/sandbox_agent/mcp.ts
echo
echo "== Relevant slice: services/oss/tests/pytest/unit/agent/tools/test_resolution.py =="
sed -n '1,160p' services/oss/tests/pytest/unit/agent/tools/test_resolution.py
echo
echo "== Relevant slice: docs/design/agent-workflows/projects/qa/scripts/run_matrix.py =="
sed -n '210,245p' docs/design/agent-workflows/projects/qa/scripts/run_matrix.py
echo
echo "== Search for both env vars in repo =="
rg -n -C2 'AGENTA_AGENT_(ENABLE_MCP|MCP_SERVERS_ENABLED)' .Length of output: 75081
I verified that
So: no blocking concerns on the two-file text fix. My recommendation on scope:
If you want the minimal assertion, it would be: assert "AGENTA_AGENT_MCP_SERVERS_ENABLED" in message✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdks/python/agenta/sdk/agents/mcp/errors.py (1)
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider locking in the env-var name with a one-line test assertion.
The existing unit test checks
"disabled"and server names in the message, but does not assert the exact env-var string. Addingassert "AGENTA_AGENT_MCP_SERVERS_ENABLED" in messagewould directly guard against regression to the old incorrect nameAGENTA_AGENT_ENABLE_MCP. This is low-risk and matches the prior review recommendation.# In services/oss/tests/pytest/unit/agent/tools/test_resolution.py ~line 89 assert "AGENTA_AGENT_MCP_SERVERS_ENABLED" in messageAs per coding guidelines, API or SDK changes require running
ruff formatthenruff check --fixbefore committing, and all errors must be fixed before committing.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a21156fa-3c7b-4981-8178-ddde2690c8ae
📒 Files selected for processing (2)
sdks/python/agenta/sdk/agents/mcp/errors.pyservices/agent/src/engines/sandbox_agent/mcp.ts
mmabrouk
added
lgtm
1 participant
🤖 The AI agent says: I opened this PR on Mahmoud's behalf to preserve the F-047 fix found during agent-workflows QA.
Context
The MCP-disabled error and a runner SSRF-guard comment named the wrong env var. Both told users to set
AGENTA_AGENT_ENABLE_MCPto turn MCP servers on. The runner does not read that variable. It gates MCP onAGENTA_AGENT_MCP_SERVERS_ENABLED(services/oss/src/agent/tools/resolver.py:24). A user who followed the remediation set a no-op variable, and MCP stayed off. QA flagged this as F-047.Scope / risk
Text only, two files:
sdks/python/agenta/sdk/agents/mcp/errors.py— theMCPDisabledErrormessage.services/agent/src/engines/sandbox_agent/mcp.ts— the SSRF-guard comment.No code path, gate, or default changes. The gate (
AGENTA_AGENT_MCP_SERVERS_ENABLED, off by default) is untouched. Nothing reads the corrected string at runtime, so there is no behavior change.Interface reference
This aligns user-facing remediation text with the real config gate. No interface field changes.
AGENTA_AGENT_MCP_SERVERS_ENABLED, read atservices/oss/src/agent/tools/resolver.py:24; declared in the compose files (e.g.hosting/docker-compose/oss/docker-compose.dev.yml:456), defaultfalse.AGENTA_AGENT_ENABLE_MCP(no code reads this name).AGENTA_AGENT_MCP_SERVERS_ENABLED.How to QA
Prereqs: the SDK MCP resolver; MCP disabled.
AGENTA_AGENT_MCP_SERVERS_ENABLEDunset (or falsey).mcp_servers.MCPDisabledError. Its message now reads "...set AGENTA_AGENT_MCP_SERVERS_ENABLED to enable them...".Test command:
Edge cases: the existing test (
services/oss/tests/pytest/unit/agent/tools/test_resolution.py:80) asserts only that the message contains "disabled" and the server names, so it stays green. It does not assert the var name. A one-line assertion (assert "AGENTA_AGENT_MCP_SERVERS_ENABLED" in message) would lock this fix against regression; left out here to keep the commit to the two user-facing files.Out of scope: a QA helper at
docs/design/agent-workflows/projects/qa/scripts/run_matrix.py:227still mentions the old var in a comment string. Left for a separate change to keep this PR to the two user-facing files.https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc