feat(mcp-tools): adding mcp support to the framework

[agustin-anastasia]

Description

Adds OAuth 2.1 support for MCP server definitions and fixes MCP tool assignment to assistants during migrations.
Also hardens MCP file generation and MCP tool ID resolution so assistant tools are populated reliably.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Refactoring (no functional changes)

Related Issues

Fixes #CSP-1651

Changes Made

• Added MCP OAuth support across framework entities:
  • auth_type (none / headers / oauth2)
  • oauth_client_id
  • oauth_scopes
• Updated addmcptools to support OAuth flow and generate stable mcp_servers.py / mcp_tools.py output (fixed indentation/dedent corruption cases).
• Updated MCP migration payloads to send OAuth config correctly and avoid storing MCP server URL in .env (URL now generated in class definition).
• Fixed assistant tool resolution to include MCP tool IDs in _assistant_payload.
• Improved MCP remote ID harvesting in migrations to support real backend response shapes (list, results, configured_tools, tools) and fallback listing.
• Added regression tests for MCP tool ID extraction and assistant MCP tool mapping.

Testing Done

• Unit tests pass locally
• Added new tests for new functionality
• Tested manually

Checklist

• My code follows the project's style guidelines
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• New and existing unit tests pass locally with my changes

[MicaPerdomo]

Review: feat(mcp-tools): adding mcp support to the framework

1. PR Summary

• What it changes: Adds MCP (Model Context Protocol) server and tool support — new base classes, API client methods, interactive CLI command, migration primitives, and loader/collector integration. Also rewrites how tools, FAQs, fixed responses, and lessons are resolved and synced during migrations (switching from migration-state-based to live-class-based resolution).
• What it solves: CSP-1651 — MCP server/tool support in the framework.
• Areas affected: cogsol/tools/__init__.py, cogsol/core/api.py, cogsol/core/loader.py, cogsol/core/mcp.py (new), cogsol/core/migrations.py, cogsol/db/migrations.py, cogsol/management/commands/addmcptools.py (new), cogsol/management/commands/migrate.py (major rewrite), cogsol/management/commands/startproject.py, tests.
• Risk level: High — 1554 additions, 345 deletions across 11 files, including a major rewrite of migrate.py sync logic.

2. Findings

Important — OAuth client_secret is collected but silently discarded

addmcptools prompts the user for oauth_client_secret and then prints:

"It will be sent to the CogSol API when you run python manage.py migrate"
"Please re-enter it if prompted during migrate."

But migrate never includes client_secret in the MCP server payload — it only sends client_id and scopes inside oauth_config. And migrate has no input() prompts. The secret is silently lost. The message is misleading. Either send the secret during migrate, or rewrite the message to say the OAuth flow is completed from the CogSol portal.

Important — Generated Python code is vulnerable to special characters in user input

In addmcptools.py, user-provided strings are interpolated directly into Python source:

f'    name = "{server_name}"'
f'    description = "{server_description}"'
f'    description = """{t_desc}"""'

If server_name contains ", server_description contains ", or t_desc contains """, the generated code is invalid Python. Use repr() for all user-provided string values in generated code.

Important — _tool_script_from_class breaks with multi-line run() signatures

The method extracts the function body using lines[def_idx + 1:], assuming the def run(...) signature fits on one line. If the signature spans multiple lines:

def run(
    self,
    chat=None,
    data=None,
):
    response = "hello"

The signature continuation lines (self,, chat=None,, etc.) get included as script code, producing invalid output. _tool_script_from_state handles this correctly using AST body positions (run_node.body[0].lineno), but _tool_script_from_class doesn't.

Important — Scope creep: MCP feature mixed with sync logic rewrite

This PR does two things:

1. Adds MCP support (new classes, CLI, API methods, migration primitives)
2. Rewrites how _sync_with_api resolves tools, FAQs, fixed, lessons — switching from migration-state-based to live-class-based resolution

The MCP additions are relatively safe, but the sync rewrite touches the critical path for all existing migrations and changes the source of truth from migration state to live Python classes. These should ideally be separate PRs so each can be reviewed and tested independently.

Important — Rollback retry is redundant and incomplete

The new rollback code retries deletion after _delete_created_entry fails:

try:
    self._delete_created_entry(client, kind, assistant_id, obj_id)
except Exception as e:
    print(f"Warning: failed to rollback {kind} {obj_id}: {e}")
    try:
        if kind == "faq" and assistant_id is not None:
            client.delete_common_question(assistant_id, obj_id)
        ...

_delete_created_entry already dispatches to the same client.delete_* methods. If it fails, retrying the exact same API call is pointless. Also, "mcp_tool" is not handled in either the retry block or _delete_created_entry, so MCP tools created during a failed migration won't be rolled back.

Important — _ask_secret uses input() instead of getpass.getpass()

OAuth client secrets are prompted with input(), making them visible in the terminal. getpass is stdlib and should be used here.

Important — FAQs over-sync when only fixed/lessons are touched

FAQs lack the if faq_filter: guard that fixed responses and lessons have. If a migration only touches a fixed response for agent "B", all FAQs for agent "B" are re-synced unnecessarily because faq_filter.get("B") returns None (falsy), skipping the filter. Not destructive (upserts are idempotent), but inconsistent and inefficient. The three entity types should be handled symmetrically.

Minor — Migration order changed without explanation

-apps = [str(app)] if app else ["data", "agents"]
+apps = [str(app)] if app else ["agents", "data"]

This reverses the default execution order. If agent migrations reference content entities created by data migrations, agents-first could cause failures. No justification in the PR description.

Minor — Mutable class-level default in BaseMCPServer

class BaseMCPServer:
    headers: dict[str, str] = {}

Shared mutable default across all subclasses. Not currently mutated at runtime, but a known Python footgun. Safer to use None with initialization in __init__.

Minor — Minimal test coverage for scope of changes

74 lines of tests for 1554 additions. The MCP tool ID extraction tests are good, but there are no tests for: addmcptools flow, MCP server payload construction, the rewritten FAQs/fixed/lessons sync logic, _tool_script_from_class, _prompt_text resolution, or the migration order change.

3. Regression risks

• High: The rewrite of tool resolution (_resolve_tool_id now works with live class instances instead of migration state strings) could break tool ID mapping for existing projects
• High: The switch from _name_aliases to _tool_key + class introspection changes how tools are looked up in remote_ids
• Medium: Migration order change (agents before data) could affect projects with cross-app dependencies
• Medium: ast.unparse in _tool_helper_source loses comments from helper functions in tool scripts

4. Missing or recommended tests

• Test _tool_script_from_class with multi-line run() signatures
• Test _assistant_payload with mixed tool types (scripts, retrieval, MCP)
• Test generated code from addmcptools with special characters in names/descriptions
• Test FAQs/fixed/lessons sync with selective touched to verify correct filtering
• Test full makemigrations + migrate cycle with MCP servers and tools

5. Final verdict

Not approvable yet

The MCP additions are well-structured, but the PR mixes them with a high-risk rewrite of core sync logic that lacks test coverage. Recommended path:

1. Fix the client_secret handling (either include in payload or fix the misleading message)
2. Use repr() for user-provided strings in generated Python code
3. Fix _tool_script_from_class to handle multi-line signatures (use AST body positions like _tool_script_from_state)
4. Add getpass for secret input
5. Add "mcp_tool" to rollback handling
6. Consider splitting into two PRs: MCP feature + sync logic refactor
7. Add test coverage for sync logic rewrite