Skip to content

fix(mcp): editing any field resets an external MCP server's restart_policy and auto_start #2088

Description

@AlexLiu190625

What happens

PUT /api/mcp/servers/{id} with a payload that touches one unrelated field silently
rewrites two other columns on the shared MCPServer row. On a row stored with
managed="external" and restart_policy="always":

BEFORE: restart_policy='always'  auto_start=True
PUT   : {"description": "a new description"}
AFTER : restart_policy='no'      auto_start=None

The request returns 200. Nothing in the response or the UI says the restart settings
changed.

Mechanism

The route does not write the payload's fields onto the row. It rebuilds the whole
configuration and writes every field of the rebuilt object back, so any column the
round-trip drops comes back as a config default.

  1. _build_server_config (src/xagent/web/api/mcp.py) starts from
    server.to_config_dict() and merges the payload into it.
  2. MCPServer.to_config_dict() (src/xagent/core/tools/core/mcp/model.py) emits
    restart_policy, auto_start and the docker fields inside a
    if self.managed == "internal": block. For an external row it emits none of them,
    so the stored values never reach the config object.
  3. MCPServerConfig (src/xagent/core/tools/core/mcp/data_config.py) then supplies its
    own defaults for the absent keys: restart_policy: str = Field("no", ...) and
    auto_start left as None.
  4. _update_server_from_config (src/xagent/web/api/mcp.py) writes back every name in
    _MCP_SERVER_CONFIGURABLE_FIELDS, and that tuple contains both restart_policy and
    auto_start. The loop is an unconditional setattr per field — there is no
    "only if the payload carried it" guard — so the defaults land on the row.

The same round-trip drops docker_url, volumes and bind_ports for an external row
by the same mechanism.

How to reproduce

  1. Store an MCP server row with managed="external", restart_policy="always" and
    auto_start=True.
  2. PUT /api/mcp/servers/{id} with {"description": "a new description"}.
  3. Read the row back: restart_policy is 'no' and auto_start is NULL.

Expected

A payload that does not name restart_policy or auto_start must not change them. More
generally, the rebuild should not be able to write a column whose stored value it never
read — either the round-trip carries every stored column through, or the write-back is
restricted to the fields the payload actually carried.

Related

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions