Skip to content

Conversation

@saqadri
Copy link
Collaborator

@saqadri saqadri commented Oct 22, 2025

Wrapped async tool registration in a factory so each generated adapter captures its own workflow name instead of accidentally reusing the last loop value.

This was caught thanks to @rholinshead's example, where grade_story_async was dispatching notify_progress since it was using the last registered workflow name.

Basically the issue would happen in the cloud sample because more tools follow the async declaration, mutating the shared loop variable, whereas our other mcp_agent_server example had grade_story_async as its last tool declaration.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed async workflow name binding issue in tool wrapper invocations
    • Corrected configuration path for Linux installations to use standard directory
  • New Features

    • Added descriptive help text to the install command arguments

@coderabbitai
Copy link

coderabbitai bot commented Oct 22, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

The PR updates the Claude Desktop Linux configuration path for MCP agent CLI installation, enhances the install command's help documentation, fixes a late-binding issue in server async wrapper generation via a factory pattern, and adds comprehensive tests for tool wrapper workflow name and parameter propagation.

Changes

Cohort / File(s) Summary
CLI Install Command Updates
src/mcp_agent/cli/commands/install.py, src/mcp_agent/cli/main.py
Updates Claude Desktop Linux config path from ~/Library/Application Support/Claude/ to ~/.config/Claude/, adds help attribute to the server_identifier argument in the install function signature, and reformats command registration and parameter declarations for readability.
Server Async Wrapper Factory
src/mcp_agent/server/app_server.py
Introduces _make_async_wrapper() factory function to resolve late-binding issues in async wrapper closures within loops by explicitly capturing the workflow name at wrapper creation time.
CLI Install Tests Formatting
tests/cli/commands/test_install.py
Reformats existing test code with adjusted line breaks and indentation; no functional changes to test logic.
Tool Wrapper Integration Tests
tests/server/test_tool_decorators.py
Adds two new test cases verifying that tool wrappers correctly capture and propagate workflow names and run parameters for both async and sync registered tools.

Sequence Diagram

sequenceDiagram
    participant Test
    participant MCPApp
    participant ToolRegistry
    participant ToolWrapper
    participant WorkflowRunner as Mocked Workflow<br/>Runner

    Test->>MCPApp: register_tool(async_fn / sync_fn)
    MCPApp->>ToolRegistry: register with wrapper
    ToolRegistry->>ToolWrapper: _make_async_wrapper(workflow_name)<br/>creates closure
    Test->>ToolWrapper: invoke registered tool
    ToolWrapper->>WorkflowRunner: _workflow_run(workflow_name,<br/>run_parameters)
    WorkflowRunner-->>ToolWrapper: workflow_run result
    Test->>Test: assert captured workflow_name<br/>& parameters match expected
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Mixed complexity with functional changes across multiple areas: CLI path update and signature enhancement (straightforward), async factory pattern fix (moderate logic scrutiny), and new test coverage (verification of test intent and mock setup).

Possibly related PRs

Suggested reviewers

  • rholinshead

Poem

🐰 A wrapper once danced in a loop,
Its bindings grew loose—quite a scoop!
A factory came, with names held tight,
Late-binding fixed—now workflows run right!
Config paths shift from lib to .config,
Tests verify tools: tick-tock, tick-tock!

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/async_tool_closure

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between af72b71 and 0f58849.

📒 Files selected for processing (5)
  • src/mcp_agent/cli/commands/install.py (14 hunks)
  • src/mcp_agent/cli/main.py (1 hunks)
  • src/mcp_agent/server/app_server.py (1 hunks)
  • tests/cli/commands/test_install.py (18 hunks)
  • tests/server/test_tool_decorators.py (1 hunks)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@saqadri saqadri merged commit 3fd8cd6 into main Oct 22, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant