fix(mcp): make agentic MCP server removal actually delete the row - #14272
Merged
Conversation
remove_agentic_mcp_server called update_server without delete=True under a
comment claiming "Empty config removes the server". That has never been true:
the flag-less call is a full replace, so users who had the langflow-agentic
server were left with an empty-config row, and users who never had it gained
a brand-new empty-config row (the create path runs when no row exists) —
a broken entry in their MCP servers list either way. The same behavior
existed under the old file-based store (the entry was set to {}).
Pass delete=True so the row is actually removed. For users without the
server, update_server raises HTTPException, which the existing per-user
except/continue block already handles.
The helper currently has no callers (the agentic-experience disable path was
never wired up), so nothing ships the broken behavior today; this fixes the
latent utility and pins the semantics with tests before anyone wires it up.
Contributor
WalkthroughAgentic MCP server removal now explicitly deletes the database row. New async SQLite tests verify removal of the targeted server, preservation of unrelated servers, and no row creation when the target server is absent. ChangesAgentic MCP deletion
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 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 |
github-actions
Bot
added
bug
Contributor
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend/tests/unit/api/utils/test_agentic_mcp.py (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop redundant
pytest.mark.asynciodecorators.Async tests are auto-detected by the repository configuration, so remove the markers and the now-unused
pytestimport.Proposed cleanup
-import pytest ... -@pytest.mark.asyncio async def test_remove_agentic_mcp_server_removes_row(): ... -@pytest.mark.asyncio async def test_remove_agentic_mcp_server_absent_is_noop():Based on learnings,
pytest-asynciois configured withasyncio_mode = "auto", so these markers are unnecessary.Also applies to: 56-56, 85-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/tests/unit/api/utils/test_agentic_mcp.py` at line 16, Remove the redundant pytest.mark.asyncio decorators from the async tests in this file and delete the now-unused pytest import. Leave the test implementations and async behavior unchanged.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/backend/tests/unit/api/utils/test_agentic_mcp.py`:
- Line 16: Remove the redundant pytest.mark.asyncio decorators from the async
tests in this file and delete the now-unused pytest import. Leave the test
implementations and async behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 93b29ea6-c4e6-4303-b78f-f96340d230a4
📒 Files selected for processing (2)
src/backend/base/langflow/api/utils/mcp/agentic_mcp.pysrc/backend/tests/unit/api/utils/test_agentic_mcp.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-1.12.0 #14272 +/- ##
==================================================
+ Coverage 61.26% 61.41% +0.14%
==================================================
Files 2347 2346 -1
Lines 238587 238339 -248
Branches 35590 35585 -5
==================================================
+ Hits 146180 146365 +185
+ Misses 90585 90159 -426
+ Partials 1822 1815 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
remove_agentic_mcp_server"removes" thelangflow-agenticMCP server by callingupdate_serverwith an empty config, under a comment claiming "Empty config removes the server". That has never been true — for either the DB-backed store or the old file-based one:{}-config row (the flag-less call is a full replace), andEither way, a broken
langflow-agenticentry surfaces in the user's MCP servers list.Mitigating factor: the helper currently has no callers on
mainorrelease-1.12.0(the agentic-experience disable path was never wired up), so nothing ships the broken behavior today. This fixes the latent utility and pins its semantics before anything wires it up. Found while tracingupdate_servercallers during review of #14005.Fix
Pass
delete=Trueso the row is actually removed. A user without the server raisesHTTPExceptionfrom the delete path, which the existing per-userexcept/continueblock already handles — andservers_removednow counts only actual removals.Tests
src/backend/tests/unit/api/utils/test_agentic_mcp.py— real functions against a real SQLite DB, in the style oftest_mcp_db_store.py:removal deletes the
langflow-agenticrow and leaves the user's other servers untoucheda user without the server gains no empty-config row, and no error propagates
Both tests fail against the unfixed code (verified red) and pass with the fix (verified green)
Existing
test_mcp_db_store.pysuite passes alongsideruff lint/format clean; pre-commit hooks pass
Summary by CodeRabbit
Bug Fixes
Tests