Skip to content

Add skill lanes for MCP tool serialization#2308

Open
RitwijParmar wants to merge 2 commits into
dimensionalOS:mainfrom
RitwijParmar:codex/skill-lane-serialization
Open

Add skill lanes for MCP tool serialization#2308
RitwijParmar wants to merge 2 commits into
dimensionalOS:mainfrom
RitwijParmar:codex/skill-lane-serialization

Conversation

@RitwijParmar
Copy link
Copy Markdown

Problem

Addresses #2204. DimOS skills currently do not expose whether a tool can run concurrently with another tool. That is risky for robot/world-mutating skills such as motion or manipulation: two MCP calls can be dispatched at the same time even when they should share an execution lane.

Closes DIM-XXX

Solution

  • Add a parameterized @skill(lane="motion") form while preserving the existing bare @skill decorator.
  • Carry the lane through SkillInfo discovery so module skill metadata knows which lane a skill belongs to.
  • Include lane metadata in MCP tools/list responses under tool _meta.
  • Serialize concurrent MCP tools/call requests that share the same non-empty lane, while leaving unlaned or differently-laned skills concurrent.

How to Test

  • /tmp/dimos/.venv/bin/python -m ruff check dimos/agents/annotation.py dimos/core/module.py dimos/agents/mcp/mcp_server.py dimos/agents/test_skill_result.py dimos/agents/mcp/test_mcp_server.py
  • /tmp/dimos/.venv/bin/python -m pytest dimos/agents/test_skill_result.py dimos/agents/mcp/test_mcp_server.py

Note: I used Python 3.12 through uv. A full uv run sync initially hit local pyaudio/PortAudio headers on this Mac, so I provisioned a minimal env for the focused files above.

Contributor License Agreement

  • I have read and approved the CLA.

Signed-off-by: Ritwij Aryan Parmar <ritwij.aryan.parmar@gmail.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 29, 2026

Greptile Summary

This PR introduces skill execution lanes — an opt-in lane parameter on the @skill decorator that serializes concurrent MCP tools/call requests sharing the same lane, preventing races in motion and other world-mutating skills.

  • annotation.py: skill is split into a _decorate_skill helper and a dual-overload public function; the lane is stored as __skill_lane__ on the wrapped callable and read during module introspection into SkillInfo.lane.
  • mcp_server.py: _handle_tools_call acquires a per-lane asyncio.Lock from an injectable lane_locks dict before dispatching to the thread pool, and tools/list exposes lane metadata under each tool's _meta key; the injectable dict design (defaulting to the app-level global) lets tests run in isolated event loops without hitting the loop-binding problem noted in the prior review.
  • Tests cover lane metadata exposure, concurrency serialization (verified via max_active == 1), and cross-event-loop isolation.

Confidence Score: 5/5

Safe to merge — the lane serialization logic is correct, the injectable lock dict cleanly isolates tests from the global state, and the decorator changes are backward-compatible.

All five changed files have straightforward, well-tested changes. The lock acquisition and release path through asyncio.Lock inside run_in_executor is sound, tests verify both serialization behavior and event-loop isolation, and no existing call sites are broken by the additive SkillInfo.lane field or the dual-overload skill decorator.

No files require special attention.

Important Files Changed

Filename Overview
dimos/agents/annotation.py Refactors skill into a parameterized decorator supporting an optional lane keyword; uses @overload typing, guards against empty-string lanes, and stores the lane on __skill_lane__.
dimos/agents/mcp/mcp_server.py Adds lane-based serialization via asyncio.Lock per lane in _handle_tools_call; exposes lane in _meta on tools/list; threads a lane_locks dict through handle_request so tests can inject isolated dicts.
dimos/core/module.py Adds optional `lane: str
dimos/agents/mcp/test_mcp_server.py Adds three new tests: lane metadata in tools/list, same-lane serialization (verifies max_active==1), and isolated lane_locks across separate event loops.
dimos/agents/test_skill_result.py Adds tests for @skill(lane="motion") recording __skill_lane__ and bare @skill recording __skill_lane__ = None.

Sequence Diagram

sequenceDiagram
    participant C as MCP Client
    participant H as handle_request
    participant TC as _handle_tools_call
    participant LL as lane_locks dict
    participant TP as Thread Pool

    C->>H: "tools/call {name:"drive"}"
    H->>TC: req_id, params, skills, rpc_calls, lane_locks
    TC->>TC: lookup rpc_call in rpc_calls
    TC->>TC: "lane = next(s.lane for s in skills if s.func_name=="drive")"
    alt lane is None
        TC->>TP: run_in_executor(rpc_call)
        TP-->>TC: result
    else "lane = "motion""
        TC->>LL: setdefault("motion", asyncio.Lock())
        LL-->>TC: lock
        TC->>TC: async with lock (acquire)
        TC->>TP: run_in_executor(rpc_call)
        Note over TP: second same-lane call waits on lock
        TP-->>TC: result
        TC->>TC: release lock
    end
    TC-->>H: JSON-RPC result
    H-->>C: response
Loading

Reviews (2): Last reviewed commit: "Fix MCP lane lock test isolation" | Re-trigger Greptile

Comment thread dimos/agents/mcp/mcp_server.py
logger.warning("MCP tool not found", tool=name)
return _jsonrpc_result_text(req_id, f"Tool not found: {name}")

lane = next((s.lane for s in skills if s.func_name == name), None)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The O(n) linear scan for the lane duplicates work already done when the rpc_calls dict was built. A {s.func_name: s.lane for s in skills}.get(name) dict comprehension or a pre-built lane map alongside rpc_calls would make this O(1).

Suggested change
lane = next((s.lane for s in skills if s.func_name == name), None)
lane = {s.func_name: s.lane for s in skills}.get(name)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@RitwijParmar
Copy link
Copy Markdown
Author

Addressed the test-isolation concern from the Greptile review.

What changed:

  • handle_request now accepts an optional lane_locks store, while the FastAPI endpoint still defaults to app.state.lane_locks for production behavior.
  • The same-lane serialization test now passes a local lock store.
  • Added a regression test that exercises the same lane name across two separate asyncio.run() event loops with fresh lock stores, so loop-bound asyncio.Lock objects do not leak between tests.

Validation run locally:

  • /Users/ritwij/.cache/codex-runtimes/codex-primary-runtime/dependencies/python/bin/python3 -m compileall dimos/agents/mcp/mcp_server.py dimos/agents/mcp/test_mcp_server.py

I could not run the full pytest target locally because the repo's uv/pixi test environment is not installed on this machine.

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