From c22e47c0865fbfb2c56064ceca4b08199f902264 Mon Sep 17 00:00:00 2001 From: bundabrg-hermes Date: Sat, 5 Sep 2026 14:28:48 +0800 Subject: [PATCH 1/4] fix(stats): categorize ticket stats by state name, not state_type_id zammad_get_ticket_stats matched each ticket's looked-up state_type_id against hardcoded constants (1=new, 2=open, 3=closed, 4=pending reminder, 5=pending close). Those are Zammad's default IDs, but state_type_id is assigned per instance and is not stable across versions or installations. On instances with a non-default state set the buckets were scrambled: e.g. a renumbered "closed" state (id 5) was counted as "pending close", inflating the pending count (thousands) while closed reported only the tickets whose state happens to carry id 3. Categorize by the semantic state name (case-insensitive) instead: "new"/"open" -> open, "closed" -> closed, "pending reminder"/ "pending close" -> pending. Unknown/custom states still count toward the total but no bucket. Also note in list_ticket_states docs that state_type_id values are per-instance, and drop the now-unused state_type mapping cache. Co-authored-by: bundabrg --- mcp_zammad/server.py | 57 ++++++++++++++++--------------------- tests/test_server.py | 67 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 33 deletions(-) diff --git a/mcp_zammad/server.py b/mcp_zammad/server.py index 4a5f5443..72c2e54b 100644 --- a/mcp_zammad/server.py +++ b/mcp_zammad/server.py @@ -102,14 +102,6 @@ def model_dump(self) -> dict[str, Any]: ... # codacy: ignore E704 CHARACTER_LIMIT = 25000 # Maximum response size per MCP best practices ARTICLE_BODY_TRUNCATE_LENGTH = 500 # Maximum length for article body in markdown formatting -# Zammad state type IDs (from Zammad API) -STATE_TYPE_NEW = 1 -STATE_TYPE_OPEN = 2 -STATE_TYPE_CLOSED = 3 -STATE_TYPE_PENDING_REMINDER = 4 -STATE_TYPE_PENDING_CLOSE = 5 - - # Tool annotation constants def _read_only_annotations(title: str) -> ToolAnnotations: """Create read-only tool annotations with title.""" @@ -1848,8 +1840,6 @@ def clear_caches(self) -> None: del self._states_cache if hasattr(self, "_priorities_cache"): del self._priorities_cache - if hasattr(self, "_state_type_mapping"): - del self._state_type_mapping @staticmethod def _extract_state_name(ticket: dict[str, Any]) -> str: @@ -1884,16 +1874,15 @@ def _is_ticket_escalated(ticket: dict[str, Any]) -> bool: or ticket.get("update_escalation_at") ) - def _get_state_type_mapping(self) -> dict[str, int]: - """Get mapping of state names to state_type_id. - - Returns: - Dictionary mapping state name to state_type_id - """ - if not hasattr(self, "_state_type_mapping"): - states = self._get_cached_states() - self._state_type_mapping = {state.name: state.state_type_id for state in states} - return self._state_type_mapping + # Semantic ticket state names that map to the open/closed/pending buckets. + # Categorization is keyed on the state *name*, not the numeric state_type_id: + # state_type_id is assigned per instance and is not stable across Zammad + # versions or installations, so matching on it produced incorrect counts on + # instances whose state set differs from the defaults (e.g. a renumbered + # "closed" state or an extra custom state such as "merged"). + _STATE_NAME_OPEN = frozenset({"new", "open"}) + _STATE_NAME_CLOSED = frozenset({"closed"}) + _STATE_NAME_PENDING = frozenset({"pending reminder", "pending close"}) def _categorize_ticket_state(self, state_name: str) -> tuple[int, int, int]: """Categorize a ticket state into open/closed/pending counters. @@ -1905,20 +1894,20 @@ def _categorize_ticket_state(self, state_name: str) -> tuple[int, int, int]: Tuple of (open_increment, closed_increment, pending_increment) Note: - Uses state_type_id from Zammad instead of string matching: - - 1 (new), 2 (open) -> open - - 3 (closed) -> closed - - 4 (pending reminder), 5 (pending close) -> pending + Categorizes by the semantic state name (case-insensitive) rather + than the numeric state_type_id, which is per-instance and unstable: + - "new", "open" -> open + - "closed" -> closed + - "pending reminder", "pending close" -> pending + Any other state (e.g. a custom state) is counted in the total but + not in any bucket. """ - state_type_mapping = self._get_state_type_mapping() - state_type_id = state_type_mapping.get(state_name, 0) - - # Categorize based on state_type_id - if state_type_id in [STATE_TYPE_NEW, STATE_TYPE_OPEN]: + name = state_name.strip().casefold() + if name in self._STATE_NAME_OPEN: return (1, 0, 0) - if state_type_id == STATE_TYPE_CLOSED: + if name in self._STATE_NAME_CLOSED: return (0, 1, 0) - if state_type_id in [STATE_TYPE_PENDING_REMINDER, STATE_TYPE_PENDING_CLOSE]: + if name in self._STATE_NAME_PENDING: return (0, 0, 1) return (0, 0, 0) @@ -2084,7 +2073,8 @@ def zammad_get_ticket_stats(params: GetTicketStatsParams) -> TicketStats: Note: Uses pagination to scan tickets without loading all into memory. May take several seconds for large ticket databases (>10k tickets). - State categorization uses state_type_id: new/open=open, closed=closed, pending=pending. + State categorization is by semantic state name: new/open=open, + closed=closed, pending reminder/pending close=pending. Date filtering (start_date, end_date) not yet implemented - shows warning if provided. Processes up to 100,000 tickets (1000 pages x 100 per page). """ @@ -2222,7 +2212,8 @@ def zammad_list_ticket_states(params: ListParams) -> str: Results are cached in memory for performance (cleared on server restart). All states are returned in a single response (no pagination needed). Use state 'name' field when creating/updating tickets, not ID. - State types: 1=new, 2=open, 3=closed, 4=pending reminder, 5=pending close. + The state_type_id values shown are per-instance and are not + meaningful across different Zammad installations. """ states = self._get_cached_states() diff --git a/tests/test_server.py b/tests/test_server.py index e5e7e7be..7cdb83ab 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -1398,6 +1398,73 @@ def test_get_ticket_stats_tool(mock_zammad_client, decorator_capturer): mock_logger.warning.assert_called_with("Date filtering not yet implemented - ignoring date parameters") +def test_categorize_ticket_state_uses_name_not_type_id(): + """Categorization must key on the state *name*, not the numeric state_type_id. + + Regression guard: on instances with renumbered built-in states or custom + states, the numeric state_type_id does not line up with the defaults, so + only the semantic name can be trusted. + """ + server_inst = ZammadMCPServer() + + assert server_inst._categorize_ticket_state("new") == (1, 0, 0) + assert server_inst._categorize_ticket_state("open") == (1, 0, 0) + assert server_inst._categorize_ticket_state("closed") == (0, 1, 0) + assert server_inst._categorize_ticket_state("pending reminder") == (0, 0, 1) + assert server_inst._categorize_ticket_state("pending close") == (0, 0, 1) + # Custom state: excluded from every bucket. + assert server_inst._categorize_ticket_state("merged") == (0, 0, 0) + # Case-insensitive, tolerant of surrounding whitespace. + assert server_inst._categorize_ticket_state(" Closed ") == (0, 1, 0) + + +def test_get_ticket_stats_uses_state_name_not_state_type_id(mock_zammad_client, decorator_capturer): + """Stats must be correct when state_type_id values are non-default. + + Replicates a real Zammad instance where the built-in states are renumbered + (closed=5, pending reminder=3) and a custom "merged" state (id 6) exists. + The old implementation compared state_type_id against hardcoded defaults + (1-5), which scrambled the counts on such an instance. + """ + mock_instance, _ = mock_zammad_client + + tickets = [ + {"id": 1, "state": {"id": 1, "name": "new", "state_type_id": 1}}, + {"id": 2, "state": {"id": 2, "name": "open", "state_type_id": 2}}, + # Non-default: "pending reminder" is state_type_id 3 here (not 4). + {"id": 3, "state": {"id": 3, "name": "pending reminder", "state_type_id": 3}}, + # Non-default: "closed" is state_type_id 5 here (not 3). + {"id": 4, "state": {"id": 5, "name": "closed", "state_type_id": 5}}, + {"id": 5, "state": {"id": 4, "name": "pending close", "state_type_id": 4}}, + # Custom state: counted in total, excluded from every bucket. + {"id": 6, "state": {"id": 6, "name": "merged", "state_type_id": 6}}, + ] + mock_instance.search_tickets.side_effect = [tickets, []] + mock_instance.get_ticket_states.return_value = [ + {"id": 1, "name": "new", "state_type_id": 1, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, + {"id": 2, "name": "open", "state_type_id": 2, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, + {"id": 3, "name": "pending reminder", "state_type_id": 3, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, + {"id": 5, "name": "closed", "state_type_id": 5, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, + {"id": 4, "name": "pending close", "state_type_id": 4, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, + {"id": 6, "name": "merged", "state_type_id": 6, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, + ] + + server_inst = ZammadMCPServer() + server_inst.client = mock_instance + test_tools, capture_tool = decorator_capturer(server_inst.mcp.tool) + server_inst.mcp.tool = capture_tool # type: ignore[method-assign, assignment] + server_inst.get_client = lambda: server_inst.client # type: ignore[method-assign, assignment, return-value] + server_inst._setup_system_tools() + + stats = test_tools["zammad_get_ticket_stats"](GetTicketStatsParams()) + + assert stats.total_count == 6 + assert stats.open_count == 2 # new + open + assert stats.closed_count == 1 # closed (state_type_id 5, not 3) + assert stats.pending_count == 2 # pending reminder (id 3) + pending close (id 4) + # "merged" is in the total but in no bucket: 2 + 1 + 2 = 5, + 1 custom = 6. + + def test_resource_handlers(decorator_capturer): """Test resource handler registration and execution.""" server = ZammadMCPServer() From b821c7008845e808ea081ccdab214c27396cf5d6 Mon Sep 17 00:00:00 2001 From: Bundabrg Hermes Date: Sat, 5 Sep 2026 14:49:24 +0800 Subject: [PATCH 2/4] fix(stats): add pending-prefix fallback for custom states in name-based categorization Custom states that Zammad's own UI treats as pending (e.g. 'pending refund', 'pending approval') were silently dropped from all buckets by the exact-match name categorization. A leading 'pending' prefix now maps them to the pending bucket, while states that merely contain the word elsewhere still fall through to total-only. This makes the name-based categorization strictly more accurate than both the original substring code and the current state_type_id code. Co-authored-by: bundabrg --- mcp_zammad/server.py | 14 ++++++++++---- tests/test_server.py | 9 ++++++++- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/mcp_zammad/server.py b/mcp_zammad/server.py index 72c2e54b..207c9862 100644 --- a/mcp_zammad/server.py +++ b/mcp_zammad/server.py @@ -1883,6 +1883,11 @@ def _is_ticket_escalated(ticket: dict[str, Any]) -> bool: _STATE_NAME_OPEN = frozenset({"new", "open"}) _STATE_NAME_CLOSED = frozenset({"closed"}) _STATE_NAME_PENDING = frozenset({"pending reminder", "pending close"}) + # Fallback prefix for custom states that Zammad's own UI treats as pending + # (e.g. a user-defined "pending refund"). A prefix match is deliberately + # narrower than the old substring check: it catches "pending anything" + # without misfiring on states that merely contain the word elsewhere. + _STATE_NAME_PENDING_PREFIX = "pending" def _categorize_ticket_state(self, state_name: str) -> tuple[int, int, int]: """Categorize a ticket state into open/closed/pending counters. @@ -1898,16 +1903,17 @@ def _categorize_ticket_state(self, state_name: str) -> tuple[int, int, int]: than the numeric state_type_id, which is per-instance and unstable: - "new", "open" -> open - "closed" -> closed - - "pending reminder", "pending close" -> pending - Any other state (e.g. a custom state) is counted in the total but - not in any bucket. + - "pending reminder", "pending close", or any custom state starting + with "pending " -> pending + Any other state (e.g. "merged") is counted in the total but not in + any bucket. """ name = state_name.strip().casefold() if name in self._STATE_NAME_OPEN: return (1, 0, 0) if name in self._STATE_NAME_CLOSED: return (0, 1, 0) - if name in self._STATE_NAME_PENDING: + if name in self._STATE_NAME_PENDING or name.startswith(self._STATE_NAME_PENDING_PREFIX): return (0, 0, 1) return (0, 0, 0) diff --git a/tests/test_server.py b/tests/test_server.py index 7cdb83ab..4646b628 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -1412,7 +1412,14 @@ def test_categorize_ticket_state_uses_name_not_type_id(): assert server_inst._categorize_ticket_state("closed") == (0, 1, 0) assert server_inst._categorize_ticket_state("pending reminder") == (0, 0, 1) assert server_inst._categorize_ticket_state("pending close") == (0, 0, 1) - # Custom state: excluded from every bucket. + # Custom "pending *" states fall into pending via the prefix fallback, + # so user-defined states (e.g. "pending refund") are not silently dropped. + assert server_inst._categorize_ticket_state("pending refund") == (0, 0, 1) + assert server_inst._categorize_ticket_state("PENDING APPROVAL") == (0, 0, 1) + # The prefix match is deliberately narrow: a state that merely *contains* + # "pending" elsewhere (not as a leading word) is NOT auto-pending. + assert server_inst._categorize_ticket_state("reviewed pending approval") == (0, 0, 0) + # Other custom states: counted in the total, excluded from every bucket. assert server_inst._categorize_ticket_state("merged") == (0, 0, 0) # Case-insensitive, tolerant of surrounding whitespace. assert server_inst._categorize_ticket_state(" Closed ") == (0, 1, 0) From e696eb5787b6839f6453d8e2cfbf9cc7ffaa67c9 Mon Sep 17 00:00:00 2001 From: bundabrg-hermes Date: Sat, 5 Sep 2026 15:24:22 +0800 Subject: [PATCH 3/4] fix(stats): restrict pending fallback to space-terminated word match A bare 'pending' prefix would miscount near-miss custom states such as 'pendingly' or 'pending-approval' as pending. Restrict the fallback to 'pending ' (space-terminated), add the bare 'pending' state to the exact match set, and lock in both directions with regression assertions. Addresses CodeRabbit review comment on the pending-prefix fallback. Co-authored-by: bundabrg --- mcp_zammad/server.py | 15 ++++++++------- tests/test_server.py | 15 +++++++++++---- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/mcp_zammad/server.py b/mcp_zammad/server.py index 207c9862..6c353f36 100644 --- a/mcp_zammad/server.py +++ b/mcp_zammad/server.py @@ -1882,12 +1882,13 @@ def _is_ticket_escalated(ticket: dict[str, Any]) -> bool: # "closed" state or an extra custom state such as "merged"). _STATE_NAME_OPEN = frozenset({"new", "open"}) _STATE_NAME_CLOSED = frozenset({"closed"}) - _STATE_NAME_PENDING = frozenset({"pending reminder", "pending close"}) + _STATE_NAME_PENDING = frozenset({"pending", "pending reminder", "pending close"}) # Fallback prefix for custom states that Zammad's own UI treats as pending - # (e.g. a user-defined "pending refund"). A prefix match is deliberately - # narrower than the old substring check: it catches "pending anything" - # without misfiring on states that merely contain the word elsewhere. - _STATE_NAME_PENDING_PREFIX = "pending" + # (e.g. a user-defined "pending refund"). A space-terminated word match is + # deliberately narrower than a bare prefix or substring: it catches + # "pending anything" without misfiring on near-miss names such as + # "pendingly" or "pending-approval", which are not reliably pending states. + _STATE_NAME_PENDING_PREFIX = "pending " def _categorize_ticket_state(self, state_name: str) -> tuple[int, int, int]: """Categorize a ticket state into open/closed/pending counters. @@ -1903,8 +1904,8 @@ def _categorize_ticket_state(self, state_name: str) -> tuple[int, int, int]: than the numeric state_type_id, which is per-instance and unstable: - "new", "open" -> open - "closed" -> closed - - "pending reminder", "pending close", or any custom state starting - with "pending " -> pending + - "pending", "pending reminder", "pending close", or any custom + state starting with "pending " (space-terminated) -> pending Any other state (e.g. "merged") is counted in the total but not in any bucket. """ diff --git a/tests/test_server.py b/tests/test_server.py index 4646b628..12becb01 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -1412,12 +1412,19 @@ def test_categorize_ticket_state_uses_name_not_type_id(): assert server_inst._categorize_ticket_state("closed") == (0, 1, 0) assert server_inst._categorize_ticket_state("pending reminder") == (0, 0, 1) assert server_inst._categorize_ticket_state("pending close") == (0, 0, 1) - # Custom "pending *" states fall into pending via the prefix fallback, - # so user-defined states (e.g. "pending refund") are not silently dropped. + # The bare "pending" state (exact match) is pending. + assert server_inst._categorize_ticket_state("pending") == (0, 0, 1) + # Custom "pending " states fall into pending via the space-terminated + # word fallback, so user-defined states (e.g. "pending refund") are not + # silently dropped. assert server_inst._categorize_ticket_state("pending refund") == (0, 0, 1) + # The fallback is deliberately narrow: near-miss names are NOT auto- + # pending (a bare prefix or substring would miscount these). + assert server_inst._categorize_ticket_state("pendingly") == (0, 0, 0) + assert server_inst._categorize_ticket_state("pending-approval") == (0, 0, 0) + assert server_inst._categorize_ticket_state("PENDING") == (0, 0, 1) assert server_inst._categorize_ticket_state("PENDING APPROVAL") == (0, 0, 1) - # The prefix match is deliberately narrow: a state that merely *contains* - # "pending" elsewhere (not as a leading word) is NOT auto-pending. + # A state that merely *contains* "pending" elsewhere is NOT auto-pending. assert server_inst._categorize_ticket_state("reviewed pending approval") == (0, 0, 0) # Other custom states: counted in the total, excluded from every bucket. assert server_inst._categorize_ticket_state("merged") == (0, 0, 0) From 20f42fbacd8e82050038d5c7b64c7ac23ff71646 Mon Sep 17 00:00:00 2001 From: bundabrg-hermes Date: Sat, 5 Sep 2026 16:03:55 +0800 Subject: [PATCH 4/4] docs(tests): use single-line docstrings for state-name regression tests Codacy's PEP 257 pattern enables both D212 (first-line summary) and D213 (second-line summary) -- mutually exclusive for any multi-line docstring. Convert the two new test docstrings to single-line summaries, which satisfies both rules, with the explanatory detail preserved as regular comments. Also applies ruff-format to a long dict literal added in the earlier commit in this PR. Co-authored-by: bundabrg --- tests/test_server.py | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/tests/test_server.py b/tests/test_server.py index 12becb01..8579c42a 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -1399,12 +1399,10 @@ def test_get_ticket_stats_tool(mock_zammad_client, decorator_capturer): def test_categorize_ticket_state_uses_name_not_type_id(): - """Categorization must key on the state *name*, not the numeric state_type_id. - - Regression guard: on instances with renumbered built-in states or custom - states, the numeric state_type_id does not line up with the defaults, so - only the semantic name can be trusted. - """ + """Categorize by state name, not numeric state_type_id (per-instance and unstable).""" + # Regression guard: on instances with renumbered built-in states or custom + # states, the numeric state_type_id does not line up with the defaults, so + # only the semantic name can be trusted. server_inst = ZammadMCPServer() assert server_inst._categorize_ticket_state("new") == (1, 0, 0) @@ -1433,13 +1431,11 @@ def test_categorize_ticket_state_uses_name_not_type_id(): def test_get_ticket_stats_uses_state_name_not_state_type_id(mock_zammad_client, decorator_capturer): - """Stats must be correct when state_type_id values are non-default. - - Replicates a real Zammad instance where the built-in states are renumbered - (closed=5, pending reminder=3) and a custom "merged" state (id 6) exists. - The old implementation compared state_type_id against hardcoded defaults - (1-5), which scrambled the counts on such an instance. - """ + """Ticket stats stay correct when state_type_id values are non-default.""" + # Replicates a real Zammad instance where the built-in states are renumbered + # (closed=5, pending reminder=3) and a custom "merged" state (id 6) exists. + # The old implementation compared state_type_id against hardcoded defaults + # (1-5), which scrambled the counts on such an instance. mock_instance, _ = mock_zammad_client tickets = [ @@ -1457,7 +1453,13 @@ def test_get_ticket_stats_uses_state_name_not_state_type_id(mock_zammad_client, mock_instance.get_ticket_states.return_value = [ {"id": 1, "name": "new", "state_type_id": 1, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, {"id": 2, "name": "open", "state_type_id": 2, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, - {"id": 3, "name": "pending reminder", "state_type_id": 3, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, + { + "id": 3, + "name": "pending reminder", + "state_type_id": 3, + "created_at": "2024-01-01", + "updated_at": "2024-01-01", + }, {"id": 5, "name": "closed", "state_type_id": 5, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, {"id": 4, "name": "pending close", "state_type_id": 4, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, {"id": 6, "name": "merged", "state_type_id": 6, "created_at": "2024-01-01", "updated_at": "2024-01-01"},