feat: add tag listing and retrieval tools #174
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add zammad_list_tags to list all system tags (requires admin.tag permission) - Add zammad_get_ticket_tags to get tags for a specific ticket - Add list_tags() client method using direct HTTP via zammad_py session - Enables workflows like finding untagged tickets Commit contains agentically generated code (Agent: Claude Code, Model: claude-opus-4-5-20250514)
Contributor
WalkthroughAdds tag-inspection features: a client method to list system tags, a Pydantic params model for ticket-tag queries, two MCP server tools (list all tags and get tags for a ticket) with Markdown/JSON output, and tests plus README/CHANGELOG documentation updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@mcp_zammad/server.py`:
- Line 2034: The noqa comment on the function definition _setup_system_tools is
unnecessary because PLR0915 isn't enabled; remove the trailing " # noqa:
PLR0915" from the def _setup_system_tools(self) -> None: declaration so the
function signature no longer contains an unused noqa directive.
- Around line 2424-2428: The bare Exception catch in the try-except block around
the get_ticket_tags method call needs to be more specific to satisfy the static
analysis check. Review the exception handling patterns used elsewhere in the
file at the referenced lines and replace the generic Exception type with the
same specific exception types that are appropriate for Zammad API client
operations. Ensure the _handle_ticket_not_found_error handler continues to be
called with the caught exception.
In `@tests/test_server.py`:
- Around line 690-708: Rename and rewrite the test so it exercises the MCP tool
zammad_list_tags instead of calling the raw client.list_tags; use
decorator_capturer to intercept tools (as in test_get_ticket_stats_tool), set
server_inst.client = mock_instance and server_inst.get_client = lambda:
server_inst.client, replace server_inst.mcp.tool with the captured tool, call
server_inst._setup_system_tools(), then invoke test_tools["zammad_list_tags"]
with a ListParams(response_format=ResponseFormat.MARKDOWN) and assert
markdown-specific output (e.g., header "# Tag List", "Found 3 tag(s)", and
"**urgent**") and that mock_instance.list_tags.assert_called_once().
- Around line 711-723: The test test_list_tags_tool_empty currently calls the
mocked client's list_tags directly instead of invoking the zammad_list_tags
tool; update the test to call the tool entry point (e.g., zammad_list_tags or
the server method that runs the tool) on the ZammadMCPServer instance after
assigning server_inst.client = mock_instance so the tool logic is exercised,
keep the mock_instance.list_tags.return_value = [], assert the tool's returned
markdown/output contains the expected message like "Found 0 tag(s)" (or
equivalent tool-specific output), and still verify
mock_instance.list_tags.assert_called_once() to ensure the client was used.
- Around line 726-741: Add a test that exercises the zammad_get_ticket_tags tool
(not just the client): instantiate ZammadMCPServer, assign mock_zammad_client[0]
to server_inst.client, create a GetTicketTagsParams with ticket_id=123, set
mock_instance.get_ticket_tags.return_value to ["urgent","billing","follow-up"],
call server_inst.zammad_get_ticket_tags(params) and assert the
returned/formatted output contains those tags and that
mock_instance.get_ticket_tags was called with 123; also add a separate subtest
where mock_instance.get_ticket_tags raises the TicketNotFoundError (or whatever
exception the client raises) and assert
server_inst._handle_ticket_not_found_error is invoked (use a spy/mocked method)
to cover the error-handling path.
- Around line 744-756: The test currently asserts client behavior but needs to
verify the tool's empty-tag message: change the call from
server_inst.get_client().get_ticket_tags(456) to
server_inst.get_tool().get_ticket_tags(456) (or otherwise invoke the tool-layer
method on ZammadMCPServer) and assert the result equals the exact message
"Ticket `#456` has no tags."; keep mock_instance.get_ticket_tags.return_value = []
and the mock_instance.get_ticket_tags.assert_called_once_with(456) verification.
| ) | ||
|
|
||
| def _setup_system_tools(self) -> None: | ||
| def _setup_system_tools(self) -> None: # noqa: PLR0915 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove the unused noqa directive.
Static analysis correctly identifies that PLR0915 is not an enabled rule. The noqa comment serves no purpose.
🧹 Remove unused noqa directive
- def _setup_system_tools(self) -> None: # noqa: PLR0915
+ def _setup_system_tools(self) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _setup_system_tools(self) -> None: # noqa: PLR0915 | |
| def _setup_system_tools(self) -> None: |
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 2034-2034: mcp_zammad/server.py#L2034
ZammadMCPServer._setup_system_tools is too complex (18) (MC0001)
🪛 Ruff (0.14.13)
2034-2034: Unused noqa directive (non-enabled: PLR0915)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In `@mcp_zammad/server.py` at line 2034, The noqa comment on the function
definition _setup_system_tools is unnecessary because PLR0915 isn't enabled;
remove the trailing " # noqa: PLR0915" from the def _setup_system_tools(self)
-> None: declaration so the function signature no longer contains an unused noqa
directive.
tests/test_server.py
Outdated
Comment on lines
726
to
741
| def test_get_ticket_tags_tool(mock_zammad_client): | ||
| """Test zammad_get_ticket_tags returns tags for a ticket.""" | ||
| mock_instance, _ = mock_zammad_client | ||
|
|
||
| mock_instance.get_ticket_tags.return_value = ["urgent", "billing", "follow-up"] | ||
|
|
||
| server_inst = ZammadMCPServer() | ||
| server_inst.client = mock_instance | ||
|
|
||
| result = server_inst.get_client().get_ticket_tags(123) | ||
|
|
||
| assert len(result) == 3 | ||
| assert "urgent" in result | ||
| assert "billing" in result | ||
| assert "follow-up" in result | ||
| mock_instance.get_ticket_tags.assert_called_once_with(123) |
Contributor
There was a problem hiding this comment.
Test covers client method but not the zammad_get_ticket_tags tool.
The test verifies get_ticket_tags(123) on the client returns the expected list, but doesn't test the actual tool which uses GetTicketTagsParams and produces formatted output. The tool also has error handling via _handle_ticket_not_found_error that isn't exercised.
🤖 Prompt for AI Agents
In `@tests/test_server.py` around lines 726 - 741, Add a test that exercises the
zammad_get_ticket_tags tool (not just the client): instantiate ZammadMCPServer,
assign mock_zammad_client[0] to server_inst.client, create a GetTicketTagsParams
with ticket_id=123, set mock_instance.get_ticket_tags.return_value to
["urgent","billing","follow-up"], call
server_inst.zammad_get_ticket_tags(params) and assert the returned/formatted
output contains those tags and that mock_instance.get_ticket_tags was called
with 123; also add a separate subtest where mock_instance.get_ticket_tags raises
the TicketNotFoundError (or whatever exception the client raises) and assert
server_inst._handle_ticket_not_found_error is invoked (use a spy/mocked method)
to cover the error-handling path.
- Use specific exception types (RequestException, ValueError) instead of generic Exception in zammad_get_ticket_tags error handling - Refactor tag tool tests to use decorator_capturer pattern, testing the actual MCP tool layer instead of raw client methods - Tests now verify formatted output (markdown headers, messages) Commit contains agentically generated code (Agent: Claude Code, Model: claude-opus-4-5-20250514)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@mcp_zammad/server.py`:
- Around line 2346-2369: The JSON returned by the list-tags branch (inside the
response_format == ResponseFormat.JSON block) uses a custom schema and lacks the
standard list metadata (offset/next_page/next_offset/_meta) and deterministic
ordering; update the code that builds result after client.list_tags() to sort
tags consistently (e.g., by name or id) and return the standard list schema used
by other endpoints: include items, total, count, page, per_page, has_more plus
offset, next_page, next_offset and a _meta object with the same fields; ensure
this change is applied where params.response_format == ResponseFormat.JSON and
uses get_client() / list_tags() outputs so clients receive the canonical list
response shape.
♻️ Duplicate comments (1)
mcp_zammad/server.py (1)
2034-2034: Remove the unusednoqadirective.
PLR0915isn’t enabled, so the suppression is redundant and flagged by Ruff.🧹 Proposed fix
- def _setup_system_tools(self) -> None: # noqa: PLR0915 + def _setup_system_tools(self) -> None:
Comment on lines
+2346
to
+2369
| client = self.get_client() | ||
| tags = client.list_tags() | ||
|
|
||
| # Format response | ||
| if params.response_format == ResponseFormat.JSON: | ||
| result = json.dumps( | ||
| { | ||
| "items": tags, | ||
| "total": len(tags), | ||
| "count": len(tags), | ||
| "page": 1, | ||
| "per_page": len(tags), | ||
| "has_more": False, | ||
| }, | ||
| indent=2, | ||
| ) | ||
| else: | ||
| lines = ["# Tag List", "", f"Found {len(tags)} tag(s)", ""] | ||
| for tag in tags: | ||
| name = tag.get("name", "Unknown") | ||
| tag_id = tag.get("id", "?") | ||
| count = tag.get("count", 0) | ||
| lines.append(f"- **{name}** (ID: {tag_id}, used {count} times)") | ||
| result = "\n".join(lines) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Align list-tags JSON schema with other list tools.
This JSON response omits offset/next_page/next_offset/_meta, which makes it inconsistent with other list endpoints and can surprise clients expecting the standard list schema. Consider reusing the same metadata shape and sorting for stable output.
♻️ Suggested adjustment
- tags = client.list_tags()
+ tags = client.list_tags()
+ sorted_tags = sorted(tags, key=lambda t: t.get("id", 0))
# Format response
if params.response_format == ResponseFormat.JSON:
result = json.dumps(
{
- "items": tags,
- "total": len(tags),
- "count": len(tags),
+ "items": sorted_tags,
+ "total": len(sorted_tags),
+ "count": len(sorted_tags),
"page": 1,
- "per_page": len(tags),
+ "per_page": len(sorted_tags),
+ "offset": 0,
"has_more": False,
+ "next_page": None,
+ "next_offset": None,
+ "_meta": {},
},
indent=2,
)
else:
- lines = ["# Tag List", "", f"Found {len(tags)} tag(s)", ""]
- for tag in tags:
+ lines = ["# Tag List", "", f"Found {len(sorted_tags)} tag(s)", ""]
+ for tag in sorted_tags:
name = tag.get("name", "Unknown")
tag_id = tag.get("id", "?")
count = tag.get("count", 0)
lines.append(f"- **{name}** (ID: {tag_id}, used {count} times)")
result = "\n".join(lines)🤖 Prompt for AI Agents
In `@mcp_zammad/server.py` around lines 2346 - 2369, The JSON returned by the
list-tags branch (inside the response_format == ResponseFormat.JSON block) uses
a custom schema and lacks the standard list metadata
(offset/next_page/next_offset/_meta) and deterministic ordering; update the code
that builds result after client.list_tags() to sort tags consistently (e.g., by
name or id) and return the standard list schema used by other endpoints: include
items, total, count, page, per_page, has_more plus offset, next_page,
next_offset and a _meta object with the same fields; ensure this change is
applied where params.response_format == ResponseFormat.JSON and uses
get_client() / list_tags() outputs so clients receive the canonical list
response shape.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
zammad_list_tagstool to list all tags defined in the systemzammad_get_ticket_tagstool to get tags for a specific ticketlist_tags()client method using direct HTTP via zammad_py sessionUse Case
Finding untagged tickets and suggesting appropriate tags based on content requires:
Current tools don't support this workflow - search results don't include tags, and there's no way to list
available tags.
Implementation Details
zammad_list_tagscalls/api/v1/tag_listendpoint (requiresadmin.tagpermission)zammad_get_ticket_tagsuses existingclient.get_ticket_tags()method (requiresticket.agentoradmin.tag)Test Plan
zammad_list_tagsreturns all system tags with id, name, and countzammad_list_tagshandles empty tag listzammad_list_tagsreturns 403 when lacking admin.tag permissionzammad_get_ticket_tagsreturns tags for tagged ticketzammad_get_ticket_tagsreturns empty list for untagged ticketReferences
Commit contains agentically generated code (Agent: Claude Code, Model: claude-opus-4-5-20250514)
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.