From 584d53e6e46a6e8fe1a8494e2292b4404aea0762 Mon Sep 17 00:00:00 2001 From: Marcus van Elst Date: Mon, 13 Apr 2026 21:49:30 +0200 Subject: [PATCH] Fix get_container_status: clean name + dual-format response handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - get_container_status now strips hash prefix from user input and calls SYNO.Docker.Container/get with the clean name (e.g. 'jenkins'), not the hash-prefixed form — the get endpoint accepts only the clean name - _format_container_detail: unwraps 'container' wrapper key if present (DSM may return {"container": {State, Config, ...}} at the data level) - Flat-format fallback: reads lowercase 'status'/'image' fields when Docker Engine nested format (State/Config) is absent - Diagnostic stderr logging for data_keys, unwrap, status, image - 25 container tests all passing Co-Authored-By: Claude Sonnet 4.6 --- .../modules/containers.py | 61 +++++++++++++----- tests/test_modules/test_containers.py | 62 ++++++++++++++++--- 2 files changed, 100 insertions(+), 23 deletions(-) diff --git a/src/mcp_synology_container/modules/containers.py b/src/mcp_synology_container/modules/containers.py index 2a268a3..e6518bf 100644 --- a/src/mcp_synology_container/modules/containers.py +++ b/src/mcp_synology_container/modules/containers.py @@ -4,6 +4,7 @@ from __future__ import annotations import logging import re +import sys from typing import TYPE_CHECKING, Any if TYPE_CHECKING: @@ -113,21 +114,28 @@ def register_containers(mcp: FastMCP, config: AppConfig, client: DsmClient) -> N Args: container_name: Name of the container to inspect. """ - resolved_name = await _resolve_container_name(client, container_name) + # Strip hash prefix from user input; SYNO.Docker.Container/get accepts + # the clean name directly — do NOT resolve to hash-prefixed name, as + # the endpoint may not accept that form. + clean_name = _strip_hash_prefix(container_name) try: data = await client.request( "SYNO.Docker.Container", "get", - params={"name": resolved_name}, + params={"name": clean_name}, ) except Exception as e: return f"Error getting container '{container_name}': {e}" + sys.stderr.write( + f"[get_container_status] name={clean_name!r} data_keys={list(data.keys())}\n" + ) + sys.stderr.flush() + if not data: return f"Container '{container_name}' not found." - display_name = _strip_hash_prefix(resolved_name) - return _format_container_detail(display_name, data) + return _format_container_detail(clean_name, data) @mcp.tool() async def get_container_logs( @@ -322,22 +330,47 @@ def _container_in_project(container: dict[str, Any], project_name: str) -> bool: def _format_container_detail(name: str, data: dict[str, Any]) -> str: - """Format container inspect data as human-readable text.""" - state = data.get("State", {}) or {} - config = data.get("Config", {}) or {} - host_config = data.get("HostConfig", {}) or {} + """Format container inspect data as human-readable text. + + DSM may return the container object under a "container" key, or directly + at the top level. Supports both the Docker Engine inspect format + (nested State/Config/HostConfig) and the DSM flat format (lowercase + status/image fields as returned by the list endpoint). + """ + # Unwrap "container" wrapper if present (common DSM API pattern) + if "container" in data and isinstance(data["container"], dict): + sys.stderr.write("[get_container_status] unwrapping 'container' key\n") + sys.stderr.flush() + raw = data["container"] + else: + raw = data + + # Docker Engine inspect format: State / Config / HostConfig + state = raw.get("State", {}) or {} + config = raw.get("Config", {}) or {} + host_config = raw.get("HostConfig", {}) or {} + + # DSM flat format fallback (lowercase fields, same as list response) + status_str = state.get("Status") or raw.get("status") or "?" + running = state.get("Running") if "Running" in state else (status_str == "running") + image_str = config.get("Image") or raw.get("image") or "?" + + sys.stderr.write( + f"[get_container_status] status={status_str!r} running={running!r} image={image_str!r}\n" + ) + sys.stderr.flush() lines = [ f"Container: {name}", - f" Status: {state.get('Status', '?')}", - f" Running: {state.get('Running', False)}", - f" Image: {config.get('Image', '?')}", + f" Status: {status_str}", + f" Running: {running}", + f" Image: {image_str}", ] if state.get("StartedAt"): - lines.append(f" Started: {state.get('StartedAt')}") - if state.get("FinishedAt") and not state.get("Running"): - lines.append(f" Finished: {state.get('FinishedAt')}") + lines.append(f" Started: {state['StartedAt']}") + if state.get("FinishedAt") and not running: + lines.append(f" Finished: {state['FinishedAt']}") lines.append(f" Exit code: {state.get('ExitCode', '?')}") memory = host_config.get("Memory", 0) diff --git a/tests/test_modules/test_containers.py b/tests/test_modules/test_containers.py index a878bc6..b6bee14 100644 --- a/tests/test_modules/test_containers.py +++ b/tests/test_modules/test_containers.py @@ -408,18 +408,16 @@ async def test_container_stats_strips_hash_prefix(): @pytest.mark.asyncio -async def test_get_container_status_resolves_hash_prefix(): - """get_container_status resolves 'jenkins' to 'f93cb8b504f7_jenkins' for DSM call.""" +async def test_get_container_status_uses_clean_name(): + """get_container_status strips hash prefix and calls get with clean name.""" from mcp_synology_container.modules.containers import register_containers detail_data = {"State": {"Status": "running", "Running": True}, "Config": {"Image": "jenkins/jenkins:lts"}} async def mock_request(api, method, **kwargs): - if api == "SYNO.Docker.Container" and method == "list": - return HASH_PREFIXED_CONTAINERS_DATA if api == "SYNO.Docker.Container" and method == "get": - # Must be called with the hash-prefixed name - assert kwargs["params"]["name"] == "f93cb8b504f7_jenkins" + # Must be called with the CLEAN name (no hash prefix) + assert kwargs["params"]["name"] == "jenkins" return detail_data return {} @@ -429,12 +427,58 @@ async def test_get_container_status_resolves_hash_prefix(): mcp, tools = make_mock_mcp() register_containers(mcp, make_config(), client) - result = await tools["get_container_status"]("jenkins") - assert "Error" not in result - # Display name should be clean (no hash prefix) + # User passes hash-prefixed name → should be stripped before get call + result = await tools["get_container_status"]("f93cb8b504f7_jenkins") + assert "running" in result assert "f93cb8b504f7" not in result +@pytest.mark.asyncio +async def test_get_container_status_container_key_unwrap(): + """get_container_status unwraps DSM 'container' wrapper key.""" + from mcp_synology_container.modules.containers import register_containers + + # DSM may wrap the inspect data under a "container" key + wrapped_data = { + "container": { + "State": {"Status": "running", "Running": True}, + "Config": {"Image": "jenkins/jenkins:lts"}, + } + } + + client = AsyncMock() + client.request.return_value = wrapped_data + + mcp, tools = make_mock_mcp() + register_containers(mcp, make_config(), client) + + result = await tools["get_container_status"]("jenkins") + assert "running" in result + assert "jenkins/jenkins:lts" in result + + +@pytest.mark.asyncio +async def test_get_container_status_flat_format(): + """get_container_status handles DSM flat format (lowercase status/image).""" + from mcp_synology_container.modules.containers import register_containers + + flat_data = { + "status": "running", + "image": "jenkins/jenkins:lts", + "name": "jenkins", + } + + client = AsyncMock() + client.request.return_value = flat_data + + mcp, tools = make_mock_mcp() + register_containers(mcp, make_config(), client) + + result = await tools["get_container_status"]("jenkins") + assert "running" in result + assert "jenkins/jenkins:lts" in result + + @pytest.mark.asyncio async def test_get_container_logs_resolves_hash_prefix(): """get_container_logs resolves 'jenkins' to 'f93cb8b504f7_jenkins' for DSM call."""