Open
Conversation
Add 17 MCP tools, 3 resources, client methods, Pydantic models,
and tests for Zammad Knowledge Base API.
- client.py: 17 new KB methods using direct HTTP (zammad_py has
no KB support); handles compound payloads, list/dict responses,
and status transitions (publish/internalize/archive/unarchive)
- models.py: 16 new KB Pydantic models (response + param); HTML
escaping, base64 validation, and path-traversal sanitization
- server.py: _setup_kb_tools() + _setup_kb_resources() wired into
_setup_tools()/_setup_resources(); KB-specific markdown formatters
- tests/test_kb.py: 68 tests covering client methods, model
validation, formatters, and server tools (243 total, all passing)
- README.md / ARCHITECTURE.md: updated with KB tools, resources,
client methods, and model hierarchy
Resources added:
zammad://kb/{kb_id}
zammad://kb/{kb_id}/category/{category_id}
zammad://kb/{kb_id}/answer/{answer_id}
Requires knowledge_base.read
Add 17 MCP tools, 3 resources, client m
… GET /knowledge_bases returns 404 Some Zammad versions/configurations return 404 on the list endpoint even when knowledge bases exist. Fall back to probing IDs 1-10 individually, stopping at the first 404. Adds regression test for the fallback path.
…structure
Zammad's GET /knowledge_bases/{id}/answers/{id} response wraps the
answer data under assets.KnowledgeBaseAnswer.{id} rather than at
the top level. _extract_kb_answer_from_payload now checks
payload[assets][KnowledgeBaseAnswer] first, falling back to the
legacy top-level key and flat dict for backward compatibility.
This fixes zammad_list_kb_answers showing all answers as 'draft'
since published_at/internal_at/archived_at were unreachable in
the raw compound payload.
…b_category Users (and LLMs) don't know translation IDs upfront. When translation_id is not provided and a title/body update is requested, automatically fetch the answer/category first and use its first translation_id. This fixes 404 errors when updating answers/categories without knowing the internal translation ID in advance.
- _kb_raise_or_return now includes HTTP status, Zammad error body, and request URL in the exception message instead of just raising raise_for_status() with no context - _handle_api_error passes the full error detail through for 404/403/401 instead of swallowing it with a generic message - Fixes update_kb_answer/update_kb_category returning opaque 404 errors
…swer Zammad's KB answer PATCH endpoint requires category_id even when not changing the category - omitting it returns: 'Couldn't find KnowledgeBase::Category without an ID' Auto-fetch category_id (and translation_id) from the answer in a single GET request when either is missing, avoiding two round-trips.
KB attachments are served via /api/v1/attachments/{id}, not the
ticket attachment endpoint. Using zammad_download_attachment for
KB attachments fails with a permission error.
- client: download_kb_attachment(attachment_id) -> (bytes, content_type)
- server: zammad_download_kb_attachment(attachment_id) returns JSON
with base64-encoded content, content_type, and size
- Docstring explicitly warns not to use zammad_download_attachment
for KB attachments
Matches the naming convention of other KB tools and accepts kb_id/answer_id/attachment_id params that Claude naturally infers from context - avoiding 'tool not found' errors when Claude passes all three IDs.
- zammad_download_kb_answer_attachment: add save_path param to write file directly to disk, returning only metadata (no base64 in context) - zammad_add_kb_answer_attachment: add file_path param to read file from disk (mime_type auto-detected), avoiding base64 in tool call - Both old base64 paths remain as fallback when path params omitted - mimetypes import moved to top-level imports
Returns raw bytes as BlobResourceContents via MCP resource protocol, avoiding base64 in tool response context. Claude Desktop can read it with: use resource zammad://kb-attachment/97727
Claude Desktop cannot autonomously trigger resources/read calls, making this resource dead code.
- Add dedicated KBAnswerAttachmentDownloadParams with clear save_path field description explaining both modes (disk vs base64) - Remove save_path from KBAnswerAttachmentDeleteParams (wrong model) - Update download tool docstring with Mode 1 / Mode 2 guidance so Claude picks the right approach based on file size and use case
- list_kb_answers now injects _title from KnowledgeBaseAnswerTranslation assets so answer titles appear without a separate fetch - _format_kb_answers_list_markdown shows title instead of bare ID - Add _extract_kb_answer_title helper on ZammadClient - Add search_kb_answers client method (case-insensitive title substring match across all categories or scoped to one) - Add SearchKBAnswersParams model - Add zammad_search_kb_answers MCP tool
- _extract_kb_answer_body extracts plain text from HTML body in KnowledgeBaseAnswerTranslation assets - list_kb_answers injects _body into each answer dict - search_kb_answers matches query against both title and body - html/re imports moved to top-level in client.py
Zammad does not include body content in the default GET answer response.
The body requires ?include_contents={translation_id} on the request.
- get_kb_answer now makes a second request with include_contents when
translation_ids are present, returning KnowledgeBaseAnswerTranslationContent
- _extract_kb_answer_body reads from KnowledgeBaseAnswerTranslationContent
(primary) with fallback to content_attributes.body (older style)
- search_kb_answers now actually matches body text
- Updated tests to reflect the two-request pattern
- _format_kb_answer_markdown now accepts title and body params - Heading uses answer title instead of bare ID when available - Body content displayed under ## Content section - Both tool and resource call sites pass title/body extracted from payload
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds full Knowledge Base (KB) support: new KB HTTP client endpoints, extensive Pydantic KB models and params, server MCP tools/resources for KB CRUD, publishing/archiving and attachments, README/ARCHITECTURE docs updates, and a comprehensive KB test suite. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
- _format_kb_search_results_markdown: extracted from zammad_search_kb_answers - _build_kb_attachment_result: extracted from zammad_download_kb_answer_attachment - _resolve_attachment_upload_params: extracted from zammad_add_kb_answer_attachment - _resolve_kb_answer_update_ids: extracted from update_kb_answer (client) Reduces flagged methods from 51-74 lines to well under 50
Author
SummaryFull Knowledge Base CRUD support for Zammad MCP, including answer search, attachment upload/download with file path support, and body content retrieval. New Tools (19 total)KB Navigation
KB Answers
KB Attachments
New Resources (3)
Key Implementation Details
Files Changed
Testinguv run pytest tests/ -q --ignore=tests/integration
# 246 passed |
added 2 commits
March 9, 2026 17:38
- _probe_kb_ids: extracted from list_knowledge_bases (CC 10 -> 4) - _answers_matching_query: extracted from search_kb_answers (CC 10 -> 4) - _first_translation_field: extracted from _extract_kb_answer_title (CC 9 -> 3) - _strip_html, _body_from_content_assets, _body_from_translation_assets: split from _extract_kb_answer_body (CC ~10 -> 3 each) - _resolve_kb_answer_update_ids: simplified boolean logic (CC 12 -> 6)
added 2 commits
March 9, 2026 17:42
- Split _setup_kb_tools into _setup_kb_category_tools, _setup_kb_answer_tools, _setup_kb_attachment_tools (MC0001 CC=67 -> ~20 each) - Extract _format_kb_answer_optional_sections from _format_kb_answer_markdown (CC=9 -> 4) - Extract _fill_ids_from_answer from _resolve_kb_answer_update_ids (CC=11 -> 5)
- _setup_kb_info_tools: list_kbs, get_kb (2 tools, CC ~4) - _setup_kb_category_tools: get/create/update/delete category (4 tools, CC ~6) - _setup_kb_answer_read_tools: list/search/get answers (3 tools, CC ~5) - _setup_kb_answer_write_tools: create/update/delete answers (3 tools, CC ~5) - _setup_kb_answer_status_tools: publish/internalize/archive/unarchive (4 tools, CC ~6) - _setup_kb_attachment_tools: add/delete/download attachments (3 tools, CC ~5) All sub-methods stay well under Codacy MC0001 limit of 8
![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: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ARCHITECTURE.md`:
- Around line 52-53: Update the ARCHITECTURE.md inventory to match the
implementation: change the tools count from "33" to "39" to reflect the actual
number registered in mcp_zammad/server.py, and add the missing client methods
`search_kb_answers()` and `download_kb_attachment()` to the Knowledge Base
method list (the block referenced around lines 91-112) so the documented public
surface mirrors mcp_zammad/client.py and server.py exactly.
In `@mcp_zammad/client.py`:
- Around line 450-477: Change list_knowledge_bases() so it only falls back to
_probe_kb_ids() when the /knowledge_bases request returns 404; for any other
non-200 status (e.g., 401/403/500) propagate or raise the error instead of
treating it as "no KBs". In _probe_kb_ids(), stop breaking on the first 404 —
continue probing the full range (1..10), skipping missing IDs but still
collecting any 200 responses; keep appending only when r.status_code == 200 and
r.content. Target the list_knowledge_bases and _probe_kb_ids methods to make
these changes.
In `@mcp_zammad/server.py`:
- Around line 964-990: The _resolve_attachment_upload_params helper currently
opens params.file_path directly; fix it by enforcing an environment-configured
allowlist directory (read from an env var) and validating the supplied path
before any open(): compute an absolute path and then realpath to collapse
symlinks, ensure the resulting path is inside the allowlist directory (e.g.,
path.startswith(allowed_dir_realpath)), verify it is a regular file and readable
(os.path.isfile / os.access), and raise a clear error if any check fails; keep
the rest of _resolve_attachment_upload_params behavior unchanged and apply the
same validation wherever zammad_add_kb_answer_attachment accepts file_path.
- Around line 3102-3128: The zammad_download_kb_answer_attachment handler
currently writes attacker-controlled bytes to arbitrary host paths and inlines
unbounded base64, so update it to validate and constrain writes: read a
configured DOWNLOAD_ROOT (env var) and resolve params.save_path using
os.path.abspath and ensure os.path.commonpath([DOWNLOAD_ROOT,
resolved_save_path]) == DOWNLOAD_ROOT (reject otherwise), sanitize filenames and
create parent dirs only under DOWNLOAD_ROOT; enforce a maximum inline size
threshold (e.g., MAX_INLINE_BYTES from env) so if len(content) >
MAX_INLINE_BYTES require a safe save_path or else reject, and also
accept/validate an explicit params.max_bytes override but cap it to a hard upper
limit before building the result with _build_kb_attachment_result; use
get_client/download_kb_attachment unchanged but perform these checks before
writing or base64-ing.
- Around line 2531-3133: The handlers like zammad_list_knowledge_bases,
zammad_get_knowledge_base, zammad_get_kb_category, zammad_create_kb_category,
zammad_update_kb_category, zammad_delete_kb_category, zammad_list_kb_answers,
zammad_search_kb_answers, zammad_get_kb_answer, zammad_create_kb_answer,
zammad_update_kb_answer, zammad_delete_kb_answer, zammad_publish_kb_answer,
zammad_internalize_kb_answer, zammad_archive_kb_answer,
zammad_unarchive_kb_answer, zammad_add_kb_answer_attachment,
zammad_delete_kb_answer_attachment, and zammad_download_kb_answer_attachment
currently return _handle_api_error(...) from their except blocks; change each
except block so failures raise an MCP error instead of returning a success
string (e.g., raise self.mcp.error(...) or the project’s MCPError type) with the
same error message/context produced by _handle_api_error; keep the original
context string and preserve JSON/markdown formatting logic, only replacing the
final return _handle_api_error(...) with raising the MCP error so callers
receive a protocol-level error instead of a normal tool result.
In `@tests/test_kb.py`:
- Around line 478-509: Update the attachment test suite to cover the new
disk-backed workflows: add tests that call client.add_kb_answer_attachment with
file_path (not just data) to verify the client reads the file, auto-detects MIME
type, and creates target directories when missing (use temp dirs/files and
assert session.post payload or file reads as appropriate), and add tests for
downloading attachments that verify saving to a provided file_path and the
base64-fallback behavior when the response contains inline data (exercise
client.download/save behavior); also include error cases (e.g., missing
file_path raises or download HTTP errors) alongside the existing
test_add_kb_answer_attachment and test_delete_kb_answer_attachment to ensure
add_kb_answer_attachment and delete_kb_answer_attachment are fully covered.
- Around line 535-542: Update the test_kb.py unit test
test_kb_answer_attachment_add_sanitizes_filename to also cover Windows-style
backslash traversal by creating KBAnswerAttachmentAddParams with
filename="..\\..\\etc\\passwd" and asserting that params.filename contains no
backslashes and equals "passwd"; locate the test function
test_kb_answer_attachment_add_sanitizes_filename and add the additional case
using KBAnswerAttachmentAddParams and checks against params.filename to ensure
both "/" and "\" traversal are sanitized.
- Around line 409-433: Add a new test in tests/test_kb.py that exercises
update_kb_answer without supplying translation_id or category_id so the code
path that calls get_kb_answer is exercised; mock
mock_zammad_api.return_value.get_kb_answer to return a dict containing the
existing category_id and translation id (e.g. id/translation id values), call
client.update_kb_answer(kb_id=1, answer_id=100, title="New Title", body="New
body") and then inspect mock_instance.session.patch.call_args.kwargs["json"] to
assert that payload["category_id"] equals the resolved category id and
payload["translations_attributes"][0]["id"] equals the resolved translation id
and that the title/content_attributes.body are set accordingly; reuse
_make_mock_response, mock_zammad_api and KB_BASE_URL setup from the other tests.
- Around line 708-739: The tests use vague assertions like assert "Error" in
result and assert "not found" in result.lower(), so update
test_list_knowledge_bases_error and test_get_knowledge_base_not_found to assert
the exact MCP error response shape and text your handlers return for
zammad_list_knowledge_bases and zammad_get_knowledge_base (e.g., exact error
dict or exact error string including the upstream exception message), and mirror
this for any mutating-tool error tests; locate the checks in the test functions
that call fn(params=ListKnowledgeBasesParams()) and
fn(params=GetKnowledgeBaseParams(kb_id=...)) and replace the loose substring
assertions with equality/assertion against the concrete MCP error format your
codebase standard uses.
- Around line 520-525: The test currently uses a broad pytest.raises(Exception)
which can hide unrelated errors; change the assertion to expect Pydantic's
ValidationError instead: import ValidationError from pydantic and replace
pytest.raises(Exception, ...) with pytest.raises(ValidationError, match="Invalid
base64") in test_kb_answer_attachment_add_invalid_base64 (ref:
KBAnswerAttachmentAddParams) and the other test block at lines 559–567 so the
tests only pass on proper Pydantic validation failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c2c8e29-8ed2-428c-9ca4-943562a4f8e2
📒 Files selected for processing (6)
ARCHITECTURE.mdREADME.mdmcp_zammad/client.pymcp_zammad/models.pymcp_zammad/server.pytests/test_kb.py
Comment on lines
+2531
to
+3133
| def _setup_kb_tools(self) -> None: # noqa: PLR0915 | ||
| """Register Knowledge Base tools.""" | ||
|
|
||
| @self.mcp.tool(annotations=_read_only_annotations("List Knowledge Bases")) | ||
| def zammad_list_knowledge_bases(params: ListKnowledgeBasesParams) -> str: | ||
| """List all knowledge bases available in Zammad. | ||
|
|
||
| Args: | ||
| params (ListKnowledgeBasesParams): Parameters containing: | ||
| - response_format (ResponseFormat): Output format (default: MARKDOWN) | ||
|
|
||
| Returns: | ||
| str: Formatted list of knowledge bases. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.reader or knowledge_base.editor permission. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| kbs = client.list_knowledge_bases() | ||
| if params.response_format == ResponseFormat.JSON: | ||
| result = json.dumps({"items": kbs, "count": len(kbs)}, indent=2, default=str) | ||
| else: | ||
| lines = ["# Knowledge Bases", "", f"Found {len(kbs)} knowledge base(s)", ""] | ||
| for kb in kbs: | ||
| lines.append(f"## KB ID: {kb.get('id', 'N/A')}") | ||
| lines.append(f"- **Active**: {kb.get('active', False)}") | ||
| if kb.get("custom_address"): | ||
| lines.append(f"- **Address**: {kb['custom_address']}") | ||
| cat_ids = kb.get("category_ids") or [] | ||
| lines.append(f"- **Root Categories**: {len(cat_ids)}") | ||
| lines.append("") | ||
| result = "\n".join(lines) | ||
| return truncate_response(result) | ||
| except Exception as e: | ||
| return _handle_api_error(e, context="listing knowledge bases") | ||
|
|
||
| @self.mcp.tool(annotations=_read_only_annotations("Get Knowledge Base")) | ||
| def zammad_get_knowledge_base(params: GetKnowledgeBaseParams) -> str: | ||
| """Get details of a specific knowledge base by ID. | ||
|
|
||
| Args: | ||
| params (GetKnowledgeBaseParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - response_format (ResponseFormat): Output format (default: MARKDOWN) | ||
|
|
||
| Returns: | ||
| str: Knowledge base details including category/answer IDs. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.reader or knowledge_base.editor permission. | ||
| Use zammad_list_knowledge_bases to discover available KB IDs. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| kb = client.get_knowledge_base(params.kb_id) | ||
| if params.response_format == ResponseFormat.JSON: | ||
| result = json.dumps(kb, indent=2, default=str) | ||
| else: | ||
| result = _format_kb_markdown(kb) | ||
| return truncate_response(result) | ||
| except Exception as e: | ||
| return _handle_api_error(e, context=f"retrieving knowledge base {params.kb_id}") | ||
|
|
||
| @self.mcp.tool(annotations=_read_only_annotations("Get KB Category")) | ||
| def zammad_get_kb_category(params: GetKBCategoryParams) -> str: | ||
| """Get a knowledge base category by ID. | ||
|
|
||
| Args: | ||
| params (GetKBCategoryParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - category_id (int): Category ID (required) | ||
| - response_format (ResponseFormat): Output format (default: MARKDOWN) | ||
|
|
||
| Returns: | ||
| str: Category details including child category IDs and answer IDs. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.reader or knowledge_base.editor permission. | ||
| Use answer_ids to find answers within this category. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| category = client.get_kb_category(params.kb_id, params.category_id) | ||
| if params.response_format == ResponseFormat.JSON: | ||
| result = json.dumps(category, indent=2, default=str) | ||
| else: | ||
| result = _format_kb_category_markdown(category) | ||
| return truncate_response(result) | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"retrieving KB category {params.category_id} in KB {params.kb_id}" | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_write_annotations("Create KB Category")) | ||
| def zammad_create_kb_category(params: CreateKBCategoryParams) -> str: | ||
| """Create a new category in a knowledge base. | ||
|
|
||
| Args: | ||
| params (CreateKBCategoryParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - title (str): Category title (required) | ||
| - kb_locale_id (int): Locale ID for the title translation (required) | ||
| - parent_id (int | None): Parent category ID (None = root category) | ||
| - category_icon (str | None): FontAwesome icon code (e.g. 'f115') | ||
|
|
||
| Returns: | ||
| str: Created category details as JSON or markdown. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.editor permission. | ||
| Use zammad_get_knowledge_base to find kb_locale_id values. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| category = client.create_kb_category( | ||
| kb_id=params.kb_id, | ||
| title=params.title, | ||
| kb_locale_id=params.kb_locale_id, | ||
| parent_id=params.parent_id, | ||
| category_icon=params.category_icon, | ||
| ) | ||
| return truncate_response(json.dumps(category, indent=2, default=str)) | ||
| except Exception as e: | ||
| return _handle_api_error(e, context=f"creating KB category in KB {params.kb_id}") | ||
|
|
||
| @self.mcp.tool(annotations=_write_annotations("Update KB Category")) | ||
| def zammad_update_kb_category(params: UpdateKBCategoryParams) -> str: | ||
| """Update an existing knowledge base category. | ||
|
|
||
| Args: | ||
| params (UpdateKBCategoryParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - category_id (int): Category ID to update (required) | ||
| - title (str | None): New category title | ||
| - translation_id (int | None): Translation ID to update (use when changing title) | ||
| - parent_id (int | None): New parent category ID | ||
| - category_icon (str | None): New FontAwesome icon code | ||
|
|
||
| Returns: | ||
| str: Updated category details as JSON. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.editor permission. | ||
| Provide translation_id alongside title to update an existing translation. | ||
| Use zammad_get_kb_category to find translation_ids. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| category = client.update_kb_category( | ||
| kb_id=params.kb_id, | ||
| category_id=params.category_id, | ||
| title=params.title, | ||
| translation_id=params.translation_id, | ||
| parent_id=params.parent_id, | ||
| category_icon=params.category_icon, | ||
| ) | ||
| return truncate_response(json.dumps(category, indent=2, default=str)) | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"updating KB category {params.category_id} in KB {params.kb_id}" | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_destructive_write_annotations("Delete KB Category")) | ||
| def zammad_delete_kb_category(params: DeleteKBCategoryParams) -> str: | ||
| """Delete a knowledge base category. | ||
|
|
||
| Args: | ||
| params (DeleteKBCategoryParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - category_id (int): Category ID to delete (required) | ||
|
|
||
| Returns: | ||
| str: Confirmation message. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.editor permission. | ||
| WARNING: This permanently deletes the category and may affect child categories. | ||
| Categories containing answers may need to be emptied first. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| client.delete_kb_category(params.kb_id, params.category_id) | ||
| return f"KB category {params.category_id} deleted from knowledge base {params.kb_id}." | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"deleting KB category {params.category_id} in KB {params.kb_id}" | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_read_only_annotations("List KB Answers")) | ||
| def zammad_list_kb_answers(params: ListKBAnswersParams) -> str: | ||
| """List all answers in a knowledge base category. | ||
|
|
||
| Args: | ||
| params (ListKBAnswersParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - category_id (int): Category ID (required) | ||
| - response_format (ResponseFormat): Output format (default: MARKDOWN) | ||
|
|
||
| Returns: | ||
| str: List of answers with status and basic metadata. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.reader or knowledge_base.editor permission. | ||
| Uses zammad_get_kb_category to discover answer_ids, then fetches each. | ||
| Large categories may be slow due to individual answer fetches. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| answers = client.list_kb_answers(params.kb_id, params.category_id) | ||
| if params.response_format == ResponseFormat.JSON: | ||
| result = json.dumps( | ||
| {"items": answers, "count": len(answers), "kb_id": params.kb_id, "category_id": params.category_id}, | ||
| indent=2, | ||
| default=str, | ||
| ) | ||
| else: | ||
| result = _format_kb_answers_list_markdown(answers, params.kb_id, params.category_id) | ||
| return truncate_response(result) | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"listing KB answers in category {params.category_id} of KB {params.kb_id}" | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_read_only_annotations("Search KB Answers")) | ||
| def zammad_search_kb_answers(params: SearchKBAnswersParams) -> str: | ||
| """Search knowledge base answers by title or body content. | ||
|
|
||
| Performs a case-insensitive substring search against answer titles | ||
| and body text across all categories in the KB (or within a specific | ||
| category). Use this to find answers without knowing the category or ID. | ||
|
|
||
| Args: | ||
| params (SearchKBAnswersParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - query (str): Search string matched against answer titles (required) | ||
| - category_id (int | None): Limit to this category (optional) | ||
| - response_format (ResponseFormat): Output format (default: MARKDOWN) | ||
|
|
||
| Returns: | ||
| str: Matching answers with ID, title, status, and category ID. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.reader or knowledge_base.editor permission. | ||
| Searches titles only, not answer body content. | ||
| For large KBs this may be slow (fetches each answer individually). | ||
| Use category_id to scope the search and improve speed. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| results = client.search_kb_answers( | ||
| kb_id=params.kb_id, | ||
| query=params.query, | ||
| category_id=params.category_id, | ||
| ) | ||
| if params.response_format == ResponseFormat.JSON: | ||
| result = json.dumps( | ||
| {"items": results, "count": len(results), "kb_id": params.kb_id, "query": params.query}, | ||
| indent=2, | ||
| default=str, | ||
| ) | ||
| else: | ||
| result = _format_kb_search_results_markdown(results, params.query, params.kb_id) | ||
| return truncate_response(result) | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"searching KB answers in KB {params.kb_id} for '{params.query}'" | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_read_only_annotations("Get KB Answer")) | ||
| def zammad_get_kb_answer(params: GetKBAnswerParams) -> str: | ||
| """Get a knowledge base answer by ID. | ||
|
|
||
| Args: | ||
| params (GetKBAnswerParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - answer_id (int): Answer ID (required) | ||
| - response_format (ResponseFormat): Output format (default: MARKDOWN) | ||
|
|
||
| Returns: | ||
| str: Answer details including status, translation IDs, and attachments. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.reader or knowledge_base.editor permission. | ||
| The API returns a compound payload; JSON format exposes the full payload. | ||
| Answer body/title are stored in translations; use translation_ids to look them up. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| payload = client.get_kb_answer(params.kb_id, params.answer_id) | ||
| if params.response_format == ResponseFormat.JSON: | ||
| result = json.dumps(payload, indent=2, default=str) | ||
| else: | ||
| answer = client._extract_kb_answer_from_payload(payload, params.answer_id) or payload | ||
| title = client._extract_kb_answer_title(payload, answer) | ||
| body = client._extract_kb_answer_body(payload, answer) | ||
| result = _format_kb_answer_markdown(answer, title=title, body=body) | ||
| return truncate_response(result) | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"retrieving KB answer {params.answer_id} in KB {params.kb_id}" | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_write_annotations("Create KB Answer")) | ||
| def zammad_create_kb_answer(params: CreateKBAnswerParams) -> str: | ||
| """Create a new answer in a knowledge base category. | ||
|
|
||
| Args: | ||
| params (CreateKBAnswerParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - category_id (int): Category to place the answer in (required) | ||
| - title (str): Answer title (required) | ||
| - body (str): Answer body content, HTML or plain text (default: "") | ||
| - kb_locale_id (int): Locale ID for this translation (required) | ||
|
|
||
| Returns: | ||
| str: Created answer details as JSON (compound payload). | ||
|
|
||
| Note: | ||
| Requires knowledge_base.editor permission. | ||
| The answer is created in 'draft' status. Use zammad_publish_kb_answer | ||
| or zammad_internalize_kb_answer to make it visible. | ||
| Use zammad_get_knowledge_base to find kb_locale_id values. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| payload = client.create_kb_answer( | ||
| kb_id=params.kb_id, | ||
| category_id=params.category_id, | ||
| title=params.title, | ||
| body=params.body, | ||
| kb_locale_id=params.kb_locale_id, | ||
| ) | ||
| return truncate_response(json.dumps(payload, indent=2, default=str)) | ||
| except Exception as e: | ||
| return _handle_api_error(e, context=f"creating KB answer in KB {params.kb_id}") | ||
|
|
||
| @self.mcp.tool(annotations=_write_annotations("Update KB Answer")) | ||
| def zammad_update_kb_answer(params: UpdateKBAnswerParams) -> str: | ||
| """Update an existing knowledge base answer. | ||
|
|
||
| Args: | ||
| params (UpdateKBAnswerParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - answer_id (int): Answer ID to update (required) | ||
| - title (str | None): New answer title | ||
| - translation_id (int | None): Translation ID to update (use with title/body) | ||
| - body (str | None): New answer body content | ||
| - category_id (int | None): Move answer to a different category | ||
|
|
||
| Returns: | ||
| str: Updated answer details as JSON (compound payload). | ||
|
|
||
| Note: | ||
| Requires knowledge_base.editor permission. | ||
| Provide translation_id alongside title/body to update an existing translation. | ||
| Use zammad_get_kb_answer to find translation_ids. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| payload = client.update_kb_answer( | ||
| kb_id=params.kb_id, | ||
| answer_id=params.answer_id, | ||
| title=params.title, | ||
| translation_id=params.translation_id, | ||
| body=params.body, | ||
| category_id=params.category_id, | ||
| ) | ||
| return truncate_response(json.dumps(payload, indent=2, default=str)) | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"updating KB answer {params.answer_id} in KB {params.kb_id}" | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_destructive_write_annotations("Delete KB Answer")) | ||
| def zammad_delete_kb_answer(params: DeleteKBAnswerParams) -> str: | ||
| """Delete a knowledge base answer permanently. | ||
|
|
||
| Args: | ||
| params (DeleteKBAnswerParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - answer_id (int): Answer ID to delete (required) | ||
|
|
||
| Returns: | ||
| str: Confirmation message. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.editor permission. | ||
| WARNING: This permanently deletes the answer and all its translations/attachments. | ||
| Consider archiving (zammad_archive_kb_answer) instead of deleting. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| client.delete_kb_answer(params.kb_id, params.answer_id) | ||
| return f"KB answer {params.answer_id} deleted from knowledge base {params.kb_id}." | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"deleting KB answer {params.answer_id} in KB {params.kb_id}" | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_idempotent_write_annotations("Publish KB Answer")) | ||
| def zammad_publish_kb_answer(params: KBAnswerPublishParams) -> str: | ||
| """Publish a knowledge base answer publicly. | ||
|
|
||
| Args: | ||
| params (KBAnswerPublishParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - answer_id (int): Answer ID (required) | ||
|
|
||
| Returns: | ||
| str: Updated answer payload as JSON. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.editor permission. | ||
| Makes the answer publicly visible. Sets published_at timestamp. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| payload = client.publish_kb_answer(params.kb_id, params.answer_id) | ||
| return truncate_response(json.dumps(payload, indent=2, default=str)) | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"publishing KB answer {params.answer_id} in KB {params.kb_id}" | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_idempotent_write_annotations("Internalize KB Answer")) | ||
| def zammad_internalize_kb_answer(params: KBAnswerPublishParams) -> str: | ||
| """Make a knowledge base answer internal (visible to agents only). | ||
|
|
||
| Args: | ||
| params (KBAnswerPublishParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - answer_id (int): Answer ID (required) | ||
|
|
||
| Returns: | ||
| str: Updated answer payload as JSON. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.editor permission. | ||
| Sets internal_at timestamp; hides the answer from public KB portal. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| payload = client.internalize_kb_answer(params.kb_id, params.answer_id) | ||
| return truncate_response(json.dumps(payload, indent=2, default=str)) | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"internalizing KB answer {params.answer_id} in KB {params.kb_id}" | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_idempotent_write_annotations("Archive KB Answer")) | ||
| def zammad_archive_kb_answer(params: KBAnswerPublishParams) -> str: | ||
| """Archive a knowledge base answer (hide without deleting). | ||
|
|
||
| Args: | ||
| params (KBAnswerPublishParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - answer_id (int): Answer ID (required) | ||
|
|
||
| Returns: | ||
| str: Updated answer payload as JSON. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.editor permission. | ||
| Sets archived_at timestamp; answer is hidden but recoverable. | ||
| Use zammad_unarchive_kb_answer to restore. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| payload = client.archive_kb_answer(params.kb_id, params.answer_id) | ||
| return truncate_response(json.dumps(payload, indent=2, default=str)) | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"archiving KB answer {params.answer_id} in KB {params.kb_id}" | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_idempotent_write_annotations("Unarchive KB Answer")) | ||
| def zammad_unarchive_kb_answer(params: KBAnswerPublishParams) -> str: | ||
| """Unarchive a previously archived knowledge base answer. | ||
|
|
||
| Args: | ||
| params (KBAnswerPublishParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - answer_id (int): Answer ID (required) | ||
|
|
||
| Returns: | ||
| str: Updated answer payload as JSON. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.editor permission. | ||
| Clears archived_at; answer returns to draft state. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| payload = client.unarchive_kb_answer(params.kb_id, params.answer_id) | ||
| return truncate_response(json.dumps(payload, indent=2, default=str)) | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"unarchiving KB answer {params.answer_id} in KB {params.kb_id}" | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_write_annotations("Add KB Answer Attachment")) | ||
| def zammad_add_kb_answer_attachment(params: KBAnswerAttachmentAddParams) -> str: | ||
| """Add an attachment to a knowledge base answer. | ||
|
|
||
| Args: | ||
| params (KBAnswerAttachmentAddParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - answer_id (int): Answer ID (required) | ||
| - file_path (str): Absolute path to file on disk (preferred). | ||
| Reads the file directly - no base64 required in the call. | ||
| filename and mime_type are inferred from the path if not set. | ||
| - filename (str): Filename (required when using data, optional with file_path) | ||
| - data (str): Base64-encoded file content (use file_path instead) | ||
| - mime_type (str): MIME type (default: application/octet-stream) | ||
|
|
||
| Returns: | ||
| str: Updated answer payload as JSON with attachment metadata. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.editor permission. | ||
| Prefer file_path over data to avoid bloating context with base64. | ||
| Attachment IDs are returned in the payload for later deletion. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| filename, data, mime_type = _resolve_attachment_upload_params(params) | ||
| payload = client.add_kb_answer_attachment( | ||
| kb_id=params.kb_id, | ||
| answer_id=params.answer_id, | ||
| filename=filename, | ||
| data=data, | ||
| mime_type=mime_type, | ||
| ) | ||
| return truncate_response(json.dumps(payload, indent=2, default=str)) | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"adding attachment to KB answer {params.answer_id} in KB {params.kb_id}" | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_destructive_write_annotations("Delete KB Answer Attachment")) | ||
| def zammad_delete_kb_answer_attachment(params: KBAnswerAttachmentDeleteParams) -> str: | ||
| """Delete an attachment from a knowledge base answer. | ||
|
|
||
| Args: | ||
| params (KBAnswerAttachmentDeleteParams): Parameters containing: | ||
| - kb_id (int): Knowledge base ID (required) | ||
| - answer_id (int): Answer ID (required) | ||
| - attachment_id (int): Attachment ID to delete (required) | ||
|
|
||
| Returns: | ||
| str: Confirmation message. | ||
|
|
||
| Note: | ||
| Requires knowledge_base.editor permission. | ||
| WARNING: Attachment deletion is permanent and cannot be undone. | ||
| Use zammad_get_kb_answer to find attachment IDs. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| client.delete_kb_answer_attachment(params.kb_id, params.answer_id, params.attachment_id) | ||
| return ( | ||
| f"Attachment {params.attachment_id} deleted from KB answer {params.answer_id} " | ||
| f"in knowledge base {params.kb_id}." | ||
| ) | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, | ||
| context=f"deleting attachment {params.attachment_id} from KB answer {params.answer_id} in KB {params.kb_id}", | ||
| ) | ||
|
|
||
| @self.mcp.tool(annotations=_read_only_annotations("Download KB Answer Attachment")) | ||
| def zammad_download_kb_answer_attachment( | ||
| params: KBAnswerAttachmentDownloadParams, | ||
| ) -> str: | ||
| """Download an attachment from a knowledge base answer. | ||
|
|
||
| Two modes: | ||
| - save_path provided: writes file to disk, returns metadata only (no base64 in context). | ||
| Recommended for large/binary files. Example: save_path=/Users/you/Downloads/file.pdf | ||
| - save_path omitted: returns base64-encoded content in JSON response. | ||
| Use for small files (<50KB) Claude needs to read directly. | ||
|
|
||
| Note: use attachment IDs from zammad_get_kb_answer. | ||
| Do NOT use zammad_download_attachment (ticket attachments only). | ||
| Requires knowledge_base.reader or knowledge_base.editor permission. | ||
| """ | ||
| client = self.get_client() | ||
| try: | ||
| content, content_type = client.download_kb_attachment(params.attachment_id) | ||
| save_path: str | None = None | ||
| if params.save_path: | ||
| save_path = os.path.abspath(params.save_path) | ||
| os.makedirs(os.path.dirname(save_path), exist_ok=True) | ||
| with open(save_path, "wb") as f: | ||
| f.write(content) | ||
| result = _build_kb_attachment_result(content, content_type, params, save_path) | ||
| return truncate_response(result) | ||
| except Exception as e: | ||
| return _handle_api_error( | ||
| e, context=f"downloading attachment {params.attachment_id} from KB answer {params.answer_id}" | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
Return KB tool failures as MCP errors, not success text.
The except Exception: return _handle_api_error(...) pattern repeats throughout _setup_kb_tools. In JSON mode, callers get a successful tool result containing prose rather than JSON, and write failures are indistinguishable from success at the protocol level. Raise an MCP error here instead of converting failures into a normal string response.
As per coding guidelines, "Handle errors gracefully with proper MCP error responses" and "Verify proper error handling with MCP-specific error responses."
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 2531-2531: mcp_zammad/server.py#L2531
ZammadMCPServer._setup_kb_tools is too complex (67) (MC0001)
🪛 Ruff (0.15.4)
[warning] 2565-2565: Do not catch blind exception: Exception
(BLE001)
[warning] 2592-2592: Do not catch blind exception: Exception
(BLE001)
[warning] 2620-2620: Do not catch blind exception: Exception
(BLE001)
[warning] 2654-2654: Do not catch blind exception: Exception
(BLE001)
[warning] 2689-2689: Do not catch blind exception: Exception
(BLE001)
[warning] 2714-2714: Consider moving this statement to an else block
(TRY300)
[warning] 2715-2715: Do not catch blind exception: Exception
(BLE001)
[warning] 2750-2750: Do not catch blind exception: Exception
(BLE001)
[warning] 2795-2795: Do not catch blind exception: Exception
(BLE001)
[warning] 2829-2829: Do not catch blind exception: Exception
(BLE001)
[warning] 2865-2865: Do not catch blind exception: Exception
(BLE001)
[warning] 2900-2900: Do not catch blind exception: Exception
(BLE001)
[warning] 2925-2925: Consider moving this statement to an else block
(TRY300)
[warning] 2926-2926: Do not catch blind exception: Exception
(BLE001)
[warning] 2951-2951: Do not catch blind exception: Exception
(BLE001)
[warning] 2976-2976: Do not catch blind exception: Exception
(BLE001)
[warning] 3002-3002: Do not catch blind exception: Exception
(BLE001)
[warning] 3027-3027: Do not catch blind exception: Exception
(BLE001)
[warning] 3066-3066: Do not catch blind exception: Exception
(BLE001)
[warning] 3092-3095: Consider moving this statement to an else block
(TRY300)
[warning] 3096-3096: Do not catch blind exception: Exception
(BLE001)
[warning] 3129-3129: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp_zammad/server.py` around lines 2531 - 3133, The handlers like
zammad_list_knowledge_bases, zammad_get_knowledge_base, zammad_get_kb_category,
zammad_create_kb_category, zammad_update_kb_category, zammad_delete_kb_category,
zammad_list_kb_answers, zammad_search_kb_answers, zammad_get_kb_answer,
zammad_create_kb_answer, zammad_update_kb_answer, zammad_delete_kb_answer,
zammad_publish_kb_answer, zammad_internalize_kb_answer,
zammad_archive_kb_answer, zammad_unarchive_kb_answer,
zammad_add_kb_answer_attachment, zammad_delete_kb_answer_attachment, and
zammad_download_kb_answer_attachment currently return _handle_api_error(...)
from their except blocks; change each except block so failures raise an MCP
error instead of returning a success string (e.g., raise self.mcp.error(...) or
the project’s MCPError type) with the same error message/context produced by
_handle_api_error; keep the original context string and preserve JSON/markdown
formatting logic, only replacing the final return _handle_api_error(...) with
raising the MCP error so callers receive a protocol-level error instead of a
normal tool result.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcp_zammad/client.py`:
- Around line 817-830: The _fill_ids_from_answer method should make the boolean
updating_text parameter keyword-only to improve call-site clarity: change the
signature of _fill_ids_from_answer to accept updating_text as a keyword-only
argument (e.g., introduce a bare '*' before updating_text) while keeping types
and return the same, then update all call sites that pass updating_text
positionally to use updating_text=<bool> so callers are explicit; ensure no
behavioral changes and run tests to validate.
- Around line 436-442: Introduce a dedicated ZammadAPIError exception class and
use it instead of raising a plain Exception in the response handling path (the
code around _kb_raise_or_return / the try/except that parses response.json()).
Implement ZammadAPIError with attributes status_code, url, and body and a
descriptive message, then replace the raise Exception(...) with raise
ZammadAPIError(response.status_code, response.url, body) so callers can catch
and inspect structured API errors.
In `@mcp_zammad/server.py`:
- Around line 2793-2796: Update the docstring that currently claims "Searches
titles only, not answer body content." to reflect the actual behavior: both
title and answer body are searched; reference the implementation in client.py
(_answers_matching_query) which checks both title.lower() and body.lower(). Edit
the docstring in server.py (the endpoint or function containing that comment) to
remove the incorrect restriction and mention that both titles and answer bodies
are searched and that for large KBs the operation may still be slow and can be
scoped with category_id.
- Around line 3344-3357: The resource is using private client methods
(_extract_kb_answer_from_payload, _extract_kb_answer_title,
_extract_kb_answer_body); add a public method on the client, e.g.
get_kb_answer_with_content(kb_id: int, answer_id: int) -> dict, that internally
calls get_kb_answer(...) and returns the processed answer dict including keys
like "answer", "title", and "body"; then update get_kb_answer_resource to call
client.get_kb_answer_with_content(int(kb_id), int(answer_id)) and use the
returned dict to call _format_kb_answer_markdown, leaving the existing exception
handling in place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2966d78-ba7d-476a-ae5e-226ec71d3bf8
📒 Files selected for processing (2)
mcp_zammad/client.pymcp_zammad/server.py
Comment on lines
+3344
to
+3357
| @self.mcp.resource("zammad://kb/{kb_id}/answer/{answer_id}") | ||
| def get_kb_answer_resource(kb_id: str, answer_id: str) -> str: | ||
| """Get a knowledge base answer as a resource.""" | ||
| client = self.get_client() | ||
| try: | ||
| payload = client.get_kb_answer(int(kb_id), int(answer_id)) | ||
| answer = client._extract_kb_answer_from_payload(payload, int(answer_id)) or payload | ||
| title = client._extract_kb_answer_title(payload, answer) | ||
| body = client._extract_kb_answer_body(payload, answer) | ||
| return _format_kb_answer_markdown(answer, title=title, body=body) | ||
| except (requests.exceptions.RequestException, ValueError, ValidationError) as e: | ||
| return _handle_api_error( | ||
| e, context=f"retrieving KB answer {answer_id} in KB {kb_id}" | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Resources call private client methods.
The KB answer resource calls client._extract_kb_answer_from_payload, client._extract_kb_answer_title, and client._extract_kb_answer_body. While functional, accessing private methods creates coupling that could break on future refactors. Consider exposing a public get_kb_answer_with_content() method that returns the processed answer dict.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp_zammad/server.py` around lines 3344 - 3357, The resource is using private
client methods (_extract_kb_answer_from_payload, _extract_kb_answer_title,
_extract_kb_answer_body); add a public method on the client, e.g.
get_kb_answer_with_content(kb_id: int, answer_id: int) -> dict, that internally
calls get_kb_answer(...) and returns the processed answer dict including keys
like "answer", "title", and "body"; then update get_kb_answer_resource to call
client.get_kb_answer_with_content(int(kb_id), int(answer_id)) and use the
returned dict to call _format_kb_answer_markdown, leaving the existing exception
handling in place.
ARCHITECTURE.md: - Update tools count 33 -> 39 - Add search_kb_answers() and download_kb_attachment() to client methods list client.py: - list_knowledge_bases: only fall back to _probe_kb_ids on 404; raise on other error statuses (401/403/500) - _probe_kb_ids: continue on 404 instead of break to discover non-contiguous IDs server.py: - Add _validate_path_within_root helper with KB_UPLOAD_ROOT / KB_DOWNLOAD_ROOT env var enforcement and symlink-safe realpath checks - _resolve_attachment_upload_params: validate upload path, check isfile/readable - zammad_download_kb_answer_attachment: validate download path, enforce KB_MAX_INLINE_BYTES limit for inline base64 responses - Add _KB_UPLOAD_ROOT, _KB_DOWNLOAD_ROOT, _KB_MAX_INLINE_BYTES module constants models.py: - sanitize_filename: normalize Windows backslashes before basename extraction to ARCHITECTURE.md: - Update tools count 33 -> 39 - Add search_kb_answers() and download_kb_attachment() to client methods list client.py: - list_knowledge_bases: oncep- Update tools va- Add search_kb_answers() an_a client.py: - list_knowledge_bases: only fall back to _probe_kb_ids on 404; _au- list_knes other error statuses (401/403/500) - _probe_kb_ids: continue on 404 i_a- _probe_kb_ids: continue on 404 in d server.py: - Add _validate_path_within_root helper with KB_UPLOAD_ROOT / KB_DOx t- Add _vakn env var enforcement and symlink-safe realpath cheobe - Tighten error assertio- _resolve_attachment_upload && git push
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
mcp_zammad/client.py (2)
822-835: 🧹 Nitpick | 🔵 TrivialMake
updating_textkeyword-only for clarity.As noted in a previous review, boolean positional arguments reduce readability at call sites. Making it keyword-only clarifies intent.
def _fill_ids_from_answer( self, answer: dict[str, Any], category_id: int | None, translation_id: int | None, + *, updating_text: bool, ) -> tuple[int | None, int | None]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp_zammad/client.py` around lines 822 - 835, Change the _fill_ids_from_answer function so the updating_text parameter is keyword-only (e.g., add a lone '*' before it in the signature of _fill_ids_from_answer) and then update every call site to pass updating_text=... instead of relying on positional boolean arguments; keep the same behavior and types, only enforce keyword usage to improve call-site clarity.
435-448: 🧹 Nitpick | 🔵 TrivialConsider a custom exception class for API errors.
As noted in a previous review, raising a generic
Exceptionmakes it harder for callers to distinguish Zammad API errors from other failures. A dedicatedZammadAPIErrorwould improve error handling downstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp_zammad/client.py` around lines 435 - 448, Replace the generic Exception raised in the response handling block with a custom exception class (e.g., ZammadAPIError) so callers can catch API-specific failures; add a ZammadAPIError class (subclassing Exception) that accepts message, status_code, url, and body, then in the block where you currently raise Exception(f"HTTP {response.status_code} from Zammad: {body} (URL: {response.url})") raise ZammadAPIError with those details (and attach response.json()/response.text output as the body), and update any imports/usages to reference ZammadAPIError instead of Exception so downstream code can handle Zammad API errors explicitly.tests/test_kb.py (1)
522-556: 🧹 Nitpick | 🔵 TrivialPartial coverage of disk-backed workflows.
The tests for
_resolve_attachment_upload_params(file path upload) anddownload_kb_attachment(save to disk) cover the underlying helpers well. However, the server tool tests (test_add_kb_answer_attachmentat line 958) only exercise the base64 data path, not thefile_pathparameter flow through the full tool invocation.Consider adding a server tool test that provides
file_pathto verify the end-to-end flow including path validation and MIME detection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_kb.py` around lines 522 - 556, Add an end-to-end server tool test that passes a real file_path through the server tool (similar to the existing test_add_kb_answer_attachment) to exercise the file_path code path: create a tmp_path file (e.g., "document.pdf") with sample bytes, set the tool input to include file_path (not data), invoke the server tool function used by test_add_kb_answer_attachment, and assert that the call to the API used base64-encoded file content and that the mime type returned/detected is "application/pdf"; reuse/mock mock_zammad_api and the client/download_kb_attachment/_resolve_attachment_upload_params behavior so the full path validation and MIME detection flow is covered end-to-end.mcp_zammad/server.py (1)
973-995:⚠️ Potential issue | 🔴 CriticalDon’t leave host filesystem access enabled when no root is configured.
root=Noneturns_validate_path_within_root()into a no-op, so bothfile_pathuploads andsave_pathdownloads still allow arbitrary host reads/writes whenever the operator forgets one env var. This should fail closed for remote MCP tools.As per coding guidelines, "Validate all user inputs" and "Use environment variables for configuration".🔒 Suggested fix
def _validate_path_within_root(path: str, root: str | None, label: str) -> str: + if root is None: + raise ValueError( + f"{label.title()} path access is disabled until KB_{label.upper()}_ROOT is configured." + ) resolved = os.path.realpath(os.path.abspath(path)) - if root is not None: - root_resolved = os.path.realpath(root) - if not resolved.startswith(root_resolved + os.sep) and resolved != root_resolved: - raise ValueError( - f"{label} path '{resolved}' is outside the configured root '{root_resolved}'. " - f"Set {label.upper().replace(' ', '_')}_ROOT env var to allow a different directory." - ) + root_resolved = os.path.realpath(root) + if os.path.commonpath([root_resolved, resolved]) != root_resolved: + raise ValueError( + f"{label} path '{resolved}' is outside the configured root '{root_resolved}'." + ) return resolvedAlso applies to: 1010-1016, 3185-3191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp_zammad/server.py` around lines 973 - 995, The current _validate_path_within_root(path, root, label) silently allows access when root is None; change it to fail closed by rejecting requests unless an explicit root is configured: if root is None raise ValueError (with a clear message referencing the missing {label.upper().replace(' ', '_')}_ROOT env var) rather than treating None as unrestricted, so callers of _validate_path_within_root (and any duplicate usage of this function) cannot read/write arbitrary host paths when the operator forgets to set the root.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcp_zammad/client.py`:
- Around line 665-683: search_kb_answers currently iterates and fetches every
answer via _answers_matching_query which can cause many API calls for large KBs;
update the search_kb_answers docstring to warn about potential performance
issues for large knowledge bases and either (preferred) add an optional
max_results: int | None parameter to search_kb_answers (and propagate it to
_answers_matching_query) to stop iteration once the limit is reached, or at
minimum document the behavior and recommend callers to paginate/limit; reference
search_kb_answers, _answers_matching_query, and get_knowledge_base when making
these changes.
In `@mcp_zammad/server.py`:
- Around line 968-970: The module-level constants _KB_UPLOAD_ROOT,
_KB_DOWNLOAD_ROOT, and _KB_MAX_INLINE_BYTES are being read from environment
before initialize() calls load_dotenv(), so .env-only values are ignored; change
the code to defer reading these values until after load_dotenv() (either by
moving their initialization into initialize() after load_dotenv() or by
converting them to functions/properties that call os.environ.get() when first
used), and ensure initialize() (which calls load_dotenv()) runs before any use;
update the other duplicated definitions (the ones referenced by the reviewer)
similarly to read environment values post-load_dotenv() and validate/convert
KB_MAX_INLINE_BYTES to int with proper fallback.
- Around line 50-52: Incoming KB handlers are currently passing raw dicts into
formatters; instead, parse and validate those payloads with the imported
Pydantic models (KnowledgeBase, KnowledgeBaseCategory, KnowledgeBaseAnswer)
before any formatting. Replace uses of raw payloads by calling
model.parse_obj(...) or model(**payload) for each corresponding payload, catch
pydantic.ValidationError to log details and return an appropriate error
response, and then pass the validated model instances into the existing
formatting/serialization functions; also add/ensure type hints on the handler
signatures to accept dict[str, Any] and return the proper response type so MyPy
knows the validation step occurred.
In `@tests/test_kb.py`:
- Around line 749-754: Add a return type annotation to the helper method
_get_tool to indicate it returns a callable; update the signature of def
_get_tool(self, server: ZammadMCPServer, name: str) to include -> Callable[...,
str] (or -> Callable if you prefer a less specific signature) and ensure you
import Callable from collections.abc at the top of the test file so the type
name resolves.
- Around line 59-64: The helper function _make_client currently accepts a
mock_api parameter that is not referenced but is required to activate the
ZammadAPI patch when creating a ZammadClient; update the _make_client function
to include a concise comment explaining that mock_api is intentionally unused
and exists solely to ensure the ZammadAPI patch/fixture is applied before
instantiation of ZammadClient (reference _make_client, mock_api, and
ZammadClient) so future readers understand the design.
---
Duplicate comments:
In `@mcp_zammad/client.py`:
- Around line 822-835: Change the _fill_ids_from_answer function so the
updating_text parameter is keyword-only (e.g., add a lone '*' before it in the
signature of _fill_ids_from_answer) and then update every call site to pass
updating_text=... instead of relying on positional boolean arguments; keep the
same behavior and types, only enforce keyword usage to improve call-site
clarity.
- Around line 435-448: Replace the generic Exception raised in the response
handling block with a custom exception class (e.g., ZammadAPIError) so callers
can catch API-specific failures; add a ZammadAPIError class (subclassing
Exception) that accepts message, status_code, url, and body, then in the block
where you currently raise Exception(f"HTTP {response.status_code} from Zammad:
{body} (URL: {response.url})") raise ZammadAPIError with those details (and
attach response.json()/response.text output as the body), and update any
imports/usages to reference ZammadAPIError instead of Exception so downstream
code can handle Zammad API errors explicitly.
In `@mcp_zammad/server.py`:
- Around line 973-995: The current _validate_path_within_root(path, root, label)
silently allows access when root is None; change it to fail closed by rejecting
requests unless an explicit root is configured: if root is None raise ValueError
(with a clear message referencing the missing {label.upper().replace(' ',
'_')}_ROOT env var) rather than treating None as unrestricted, so callers of
_validate_path_within_root (and any duplicate usage of this function) cannot
read/write arbitrary host paths when the operator forgets to set the root.
In `@tests/test_kb.py`:
- Around line 522-556: Add an end-to-end server tool test that passes a real
file_path through the server tool (similar to the existing
test_add_kb_answer_attachment) to exercise the file_path code path: create a
tmp_path file (e.g., "document.pdf") with sample bytes, set the tool input to
include file_path (not data), invoke the server tool function used by
test_add_kb_answer_attachment, and assert that the call to the API used
base64-encoded file content and that the mime type returned/detected is
"application/pdf"; reuse/mock mock_zammad_api and the
client/download_kb_attachment/_resolve_attachment_upload_params behavior so the
full path validation and MIME detection flow is covered end-to-end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f90da57-685f-4130-ae46-1f40ae263352
📒 Files selected for processing (5)
ARCHITECTURE.mdmcp_zammad/client.pymcp_zammad/models.pymcp_zammad/server.pytests/test_kb.py
Comment on lines
+665
to
+683
| def search_kb_answers( | ||
| self, kb_id: int, query: str, category_id: int | None = None | ||
| ) -> list[dict[str, Any]]: | ||
| """Search KB answers by title or body content (case-insensitive substring match). | ||
|
|
||
| Searches across all categories in the KB, or within a specific category. | ||
| Each result includes '_title' and '_body' keys extracted from translations. | ||
|
|
||
| Args: | ||
| kb_id: Knowledge base ID | ||
| query: Search string (case-insensitive, matched against title) | ||
| category_id: If provided, limit search to this category | ||
|
|
||
| Returns: | ||
| List of matching answer dicts with '_title' injected. | ||
| """ | ||
| kb = self.get_knowledge_base(kb_id) | ||
| category_ids = [category_id] if category_id is not None else (kb.get("category_ids") or []) | ||
| return self._answers_matching_query(kb_id, category_ids, query.lower()) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Performance consideration for large knowledge bases.
The search implementation iterates through all answers in all categories, fetching each answer individually. For knowledge bases with many categories/answers, this could result in significant API call overhead.
This is acceptable given Zammad's KB API doesn't provide server-side search, but consider adding a note in the docstring about potential performance implications for large KBs, or adding a max_results parameter to limit iterations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp_zammad/client.py` around lines 665 - 683, search_kb_answers currently
iterates and fetches every answer via _answers_matching_query which can cause
many API calls for large KBs; update the search_kb_answers docstring to warn
about potential performance issues for large knowledge bases and either
(preferred) add an optional max_results: int | None parameter to
search_kb_answers (and propagate it to _answers_matching_query) to stop
iteration once the limit is reached, or at minimum document the behavior and
recommend callers to paginate/limit; reference search_kb_answers,
_answers_matching_query, and get_knowledge_base when making these changes.
Comment on lines
+50
to
+52
| KnowledgeBase, | ||
| KnowledgeBaseAnswer, | ||
| KnowledgeBaseCategory, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate KB payloads with the new models before formatting.
The new KB paths consume raw dict[str, Any] through formatting even though KnowledgeBase, KnowledgeBaseCategory, and KnowledgeBaseAnswer are already imported. That skips schema validation on external API responses and weakens the new surface under strict MyPy.
♻️ Suggested direction
- kb = client.get_knowledge_base(params.kb_id)
+ kb = KnowledgeBase(**client.get_knowledge_base(params.kb_id))
if params.response_format == ResponseFormat.JSON:
- result = json.dumps(kb, indent=2, default=str)
+ result = json.dumps(kb.model_dump(), indent=2, default=str)
else:
result = _format_kb_markdown(kb)-def _format_kb_markdown(kb: dict[str, Any]) -> str:
+def _format_kb_markdown(kb: KnowledgeBase) -> str:Also applies to: 792-882, 2598-2600, 2634-2638, 2665-2669, 2874-2881
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp_zammad/server.py` around lines 50 - 52, Incoming KB handlers are
currently passing raw dicts into formatters; instead, parse and validate those
payloads with the imported Pydantic models (KnowledgeBase,
KnowledgeBaseCategory, KnowledgeBaseAnswer) before any formatting. Replace uses
of raw payloads by calling model.parse_obj(...) or model(**payload) for each
corresponding payload, catch pydantic.ValidationError to log details and return
an appropriate error response, and then pass the validated model instances into
the existing formatting/serialization functions; also add/ensure type hints on
the handler signatures to accept dict[str, Any] and return the proper response
type so MyPy knows the validation step occurred.
server.py: - _validate_path_within_root: fail-closed when root=None (deny access unless KB_UPLOAD_ROOT / KB_DOWNLOAD_ROOT env var is explicitly configured) - Use os.path.commonpath instead of startswith for path containment check - Replace module-level _KB_* constants with _kb_upload_root(), _kb_download_root(), _kb_max_inline_bytes() functions so env vars are read after load_dotenv() - Fix stale _KB_MAX_INLINE_BYTES reference in inline-size error message - Fix docstring: zammad_search_kb_answers searches titles AND body, not titles only client.py: - Add ZammadAPIError(Exception) with status_code, url, body attributes; replace generic raise Exception(...) in _kb_raise_or_return - Make _fill_ids_from_answer updating_text parameter keyword-only (*,) - Update _resolve_kb_answer_update_ids call site to use updating_text=... - Add get_kb_answer_with_content() public method wrapp server.py: - _validate_path_within_root: fail-closed when root=None (deny access unless KB_UPLOAD_ROOT / KB_DOWNLOAD_ROOT enveso- _valida c KB_UPLOAD_ROOT / KB_DOWNLOAD_ROOT env var is explicitly configured) - Usean- Use os.path.commonpath instead of startswith for path contaiom colle- Replace module-level _KB_* constants with _kb_upload_root(), _kb_downlpl _kb_max_inline_bytes() functions so env vars are read after load_dotenv() - Fix an- Fix stale _KB_MAX_INLINE_BYTES reference in inline-size error message - e - Fix docstring: zammad_search_kb_answers searches titles AND body, noat client.py: - Add ZammadAPIError(Excool test exercising file_path -> disk read -> M- Add Zamti replace generic raise Exception(...) in _kb_raise_or_return - Make _pu- Make _fill_ids_from_ans
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
mcp_zammad/client.py (1)
458-490:⚠️ Potential issue | 🟠 Major
list_knowledge_bases()still falls back on paths that are not a real 404 fallback.A
200with an empty body still drops into_probe_kb_ids(), and_probe_kb_ids()silently skips non-200 probe failures. That can turn a broken list/probe response into an incomplete or empty KB list instead of a real error.As per coding guidelines, "mcp_zammad/client.py: Review Zammad API integration for correct endpoint usage."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp_zammad/client.py` around lines 458 - 490, list_knowledge_bases currently treats a 200 response with empty content as a signal to fallback to _probe_kb_ids and _probe_kb_ids silently ignores non-200 probe errors, which can hide real failures; update list_knowledge_bases to treat a 200 with empty/invalid body as an error (call response.raise_for_status() or raise a ValueError) instead of falling back, and modify _probe_kb_ids so it only ignores 404 responses but collects or raises on any other non-200 status (e.g., track the first non-404 error from self.api.session.get(self._kb_url(kb_id)) and raise it after probing or propagate via response.raise_for_status()), ensuring valid JSON is validated before appending to results.mcp_zammad/server.py (1)
2599-3223:⚠️ Potential issue | 🟠 MajorRaise MCP errors instead of returning error strings from KB tools.
Every handler in this block turns failures into a normal string result, and Lines 3210-3217 do the same for the inline-size guard. That makes failed reads/writes indistinguishable from success at the protocol layer.
As per coding guidelines, "Handle errors gracefully with proper MCP error responses" and "Verify proper error handling with MCP-specific error responses."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp_zammad/server.py` around lines 2599 - 3223, The KB handlers currently return error strings (e.g. in zammad_download_kb_answer_attachment's inline-size guard and all except/except blocks) which makes failures indistinguishable from success; change each error path to raise an MCP-level error instead of returning a string by replacing the returned JSON/error-string responses and the returns inside except blocks with a proper MCP error raise (use the MCP error API your service exposes — e.g. raise an MCPError or call the MCP error helper) so callers receive a protocol-level error; focus edits in zammad_download_kb_answer_attachment (replace the inline-size json return that uses _kb_max_inline_bytes() and the final return inside the except) and update every other tool function in this diff (e.g. zammad_list_knowledge_bases, zammad_get_knowledge_base, zammad_get_kb_category, zammad_create_kb_category, zammad_update_kb_category, zammad_delete_kb_category, zammad_list_kb_answers, zammad_search_kb_answers, zammad_get_kb_answer, zammad_create_kb_answer, zammad_update_kb_answer, zammad_delete_kb_answer, zammad_publish_kb_answer, zammad_internalize_kb_answer, zammad_archive_kb_answer, zammad_unarchive_kb_answer, zammad_add_kb_answer_attachment, zammad_delete_kb_answer_attachment) to raise the MCP error with contextual text (same contexts used in _handle_api_error) rather than returning error strings so all failures surface as MCP errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcp_zammad/client.py`:
- Around line 624-629: The code currently ignores failures fetching the
translation body and falls back to returning payload, which hides 401/403/500
errors; in the block that handles translation_ids (around variables
translation_ids, response2, _kb_raise_or_return, payload) change the logic so
that if the include_contents GET returns non-ok you call
self._kb_raise_or_return(response2) (so the error is raised/handled) instead of
silently returning payload; keep returning payload only when no translation_ids
exist.
- Around line 665-675: The loop that fetches answers currently swallows all
exceptions (around get_kb_answer and _extract_kb_answer_from_payload), hiding
auth/network/server errors; change the broad "except Exception" to only tolerate
missing-answer/parsing cases and explicit 404 NotFound responses: catch parsing
errors (e.g., KeyError, IndexError, ValueError) and, if your HTTP client raises
requests.exceptions.HTTPError or returns a response with status_code, handle
only 404 by logging a warning and continuing, but re-raise or let other HTTP
errors (401, 500, connection errors) propagate; apply the same change to the
similar block at 683-691 so only per-answer "not found"/parse misses are
suppressed while auth/network/server failures surface.
In `@mcp_zammad/server.py`:
- Around line 3412-3415: The KB answer endpoint currently returns the full
rendered body from client.get_kb_answer_with_content which can exceed response
budgets; before calling _format_kb_answer_markdown, run the returned body (and
optionally the answer text) through truncate_response(...) to limit size. Update
the code path that uses client.get_kb_answer_with_content to replace
result["body"] (and result["answer"] if applicable) with
truncate_response(result["body"]) and pass those truncated values into
_format_kb_answer_markdown so the resource never returns an unbounded body.
- Around line 3389-3419: The three resource handlers get_kb_resource,
get_kb_category_resource, and get_kb_answer_resource catch RequestException,
ValueError, and ValidationError but not the new ZammadAPIError raised by
mcp_zammad.client; update each except clause to also catch ZammadAPIError
(importing it if needed) so HTTP 4xx/5xx errors from client.get_knowledge_base,
client.get_kb_category, and client.get_kb_answer_with_content are routed to
_handle_api_error with the existing context strings.
---
Duplicate comments:
In `@mcp_zammad/client.py`:
- Around line 458-490: list_knowledge_bases currently treats a 200 response with
empty content as a signal to fallback to _probe_kb_ids and _probe_kb_ids
silently ignores non-200 probe errors, which can hide real failures; update
list_knowledge_bases to treat a 200 with empty/invalid body as an error (call
response.raise_for_status() or raise a ValueError) instead of falling back, and
modify _probe_kb_ids so it only ignores 404 responses but collects or raises on
any other non-200 status (e.g., track the first non-404 error from
self.api.session.get(self._kb_url(kb_id)) and raise it after probing or
propagate via response.raise_for_status()), ensuring valid JSON is validated
before appending to results.
In `@mcp_zammad/server.py`:
- Around line 2599-3223: The KB handlers currently return error strings (e.g. in
zammad_download_kb_answer_attachment's inline-size guard and all except/except
blocks) which makes failures indistinguishable from success; change each error
path to raise an MCP-level error instead of returning a string by replacing the
returned JSON/error-string responses and the returns inside except blocks with a
proper MCP error raise (use the MCP error API your service exposes — e.g. raise
an MCPError or call the MCP error helper) so callers receive a protocol-level
error; focus edits in zammad_download_kb_answer_attachment (replace the
inline-size json return that uses _kb_max_inline_bytes() and the final return
inside the except) and update every other tool function in this diff (e.g.
zammad_list_knowledge_bases, zammad_get_knowledge_base, zammad_get_kb_category,
zammad_create_kb_category, zammad_update_kb_category, zammad_delete_kb_category,
zammad_list_kb_answers, zammad_search_kb_answers, zammad_get_kb_answer,
zammad_create_kb_answer, zammad_update_kb_answer, zammad_delete_kb_answer,
zammad_publish_kb_answer, zammad_internalize_kb_answer,
zammad_archive_kb_answer, zammad_unarchive_kb_answer,
zammad_add_kb_answer_attachment, zammad_delete_kb_answer_attachment) to raise
the MCP error with contextual text (same contexts used in _handle_api_error)
rather than returning error strings so all failures surface as MCP errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e27d7e46-2fa4-4e34-a0c8-a7ccfd9fa22f
📒 Files selected for processing (3)
mcp_zammad/client.pymcp_zammad/server.pytests/test_kb.py
client.py: - list_knowledge_bases: treat 200 with empty body as ValueError instead of silently falling back to _probe_kb_ids; restructure to check 404 first, then raise_for_status on other non-ok, then validate response body shape - _probe_kb_ids: raise on non-404 HTTP errors (401/403/500) via raise_for_status so auth and server failures surface instead of being silently skipped - get_kb_answer: call _kb_raise_or_return(response2) unconditionally instead of only when response2.ok, so 401/403/500 on include_contents fetch is raised rather than silently returning payload with no body content - list_kb_answers: narrow broad 'except Exception' to (KeyError, IndexError, ValueError) for parse errors and ZammadAPIError for 404-only HTTP errors; other HTTP errors (401/500) now propagate - _answers_matching_query: same narrowing as list_kb_answers server.py: - Import ZammadA client.py: - list_knowledge_bases: treat 200 with empty body as ValueError instead of silently falling back to _probe_kb_ids;mma- list_kn t silently falling back to _probe_kb_ids; restructure to check 404 first,_e raise_for_status on other non-ok, then validate response body shape - _prob_r- _probe_kb_ids: raise on non-404 HTTP errors (401/403/500) via raisbo so auth and server failures surface instead of being silently skipped - get_kai- get_kb_answer: call _kb_raise_or_return(response2) unconditionally iid only when response2.ok, so 401/403/500 on include_contents fetch is raised nc rather than silently returning payload with no body content - list_kb_anss - lies ZammadAPIError instead of returning bodyless payload - ValueError) for parse errors and ZammadAPIError for 404-only HTTP errors;so other HTTP errors (401/500) now propagate - _answers_matching_query: samt - _an
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
mcp_zammad/server.py (2)
2630-2631:⚠️ Potential issue | 🟠 MajorRaise KB tool failures as MCP errors.
The repeated
except Exception: return _handle_api_error(...)pattern turns failed KB operations into successful tool results, so callers cannot distinguish an actual tool failure from a normal string payload. Please raise an MCP/FastMCP error after building the same message instead of returning it as tool output.As per coding guidelines, "Handle errors gracefully with proper MCP error responses" and "Verify proper error handling with MCP-specific error responses."
Also applies to: 2657-2658, 2688-2691, 2722-2723, 2757-2760, 2783-2786, 2821-2824, 2866-2869, 2900-2903, 2939-2940, 2974-2977, 3000-3002, 3028-3031, 3053-3056, 3079-3082, 3104-3107, 3146-3149, 3176-3180, 3220-3223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp_zammad/server.py` around lines 2630 - 2631, The except blocks that currently do "except Exception as e: return _handle_api_error(e, context=...)" should not return a tool payload; instead build the same error message with _handle_api_error (or format the message string) and raise an MCP-specific exception (e.g., FastMCPError or MCPError) so callers see a failure. Replace the return statement in each handler (e.g., the instance in server.py around the "listing knowledge bases" catch and the other listed locations) with code that constructs the error message via _handle_api_error(...) (or its message) and then raises FastMCPError(message) (or raise MCPError(message)) so the failure propagates rather than being returned as a successful tool result.
3390-3391:⚠️ Potential issue | 🟠 MajorTruncate the rendered KB resources before returning them.
get_kb_resource()andget_kb_category_resource()never calltruncate_response(), andget_kb_answer_resource()only truncatesbodybefore formatting. Large category/answer ID lists or attachment metadata can still push the final resource over the MCP response budget.As per coding guidelines, "Review for MCP protocol compliance and FastMCP best practices."✂️ Suggested change
- kb = client.get_knowledge_base(int(kb_id)) - return _format_kb_markdown(kb) + kb = client.get_knowledge_base(int(kb_id)) + return truncate_response(_format_kb_markdown(kb)) ... - category = client.get_kb_category(int(kb_id), int(category_id)) - return _format_kb_category_markdown(category) + category = client.get_kb_category(int(kb_id), int(category_id)) + return truncate_response(_format_kb_category_markdown(category)) ... - return _format_kb_answer_markdown( - result["answer"], title=result["title"], body=body - ) + return truncate_response( + _format_kb_answer_markdown( + result["answer"], title=result["title"], body=body + ) + )Also applies to: 3400-3401, 3412-3416
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp_zammad/server.py` around lines 3390 - 3391, get_kb_resource and get_kb_category_resource return potentially large rendered KB dicts without applying truncate_response and get_kb_answer_resource only truncates the body; update these functions (get_kb_resource, get_kb_category_resource, get_kb_answer_resource) to call truncate_response on the final rendered resource (the value returned by _format_kb_markdown or the assembled answer/category dict) before returning to ensure attachments, id lists and metadata are size-limited; preserve current formatting logic (e.g., _format_kb_markdown) and call truncate_response on the completed object/string right before the return in each function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcp_zammad/client.py`:
- Around line 729-731: The KB-wide search currently only uses
knowledge_base["category_ids"] (roots) and misses nested categories; update the
logic in the method that calls _answers_matching_query so that when category_id
is None you expand the root category IDs into a full list of descendant category
IDs by traversing each category's child_ids (e.g., BFS/DFS using the existing
get_category/get_knowledge_base accessors or direct kb lookup of child_ids) and
pass that flattened list to _answers_matching_query(kb_id, category_ids,
query.lower()) so answers in nested categories are included.
---
Duplicate comments:
In `@mcp_zammad/server.py`:
- Around line 2630-2631: The except blocks that currently do "except Exception
as e: return _handle_api_error(e, context=...)" should not return a tool
payload; instead build the same error message with _handle_api_error (or format
the message string) and raise an MCP-specific exception (e.g., FastMCPError or
MCPError) so callers see a failure. Replace the return statement in each handler
(e.g., the instance in server.py around the "listing knowledge bases" catch and
the other listed locations) with code that constructs the error message via
_handle_api_error(...) (or its message) and then raises FastMCPError(message)
(or raise MCPError(message)) so the failure propagates rather than being
returned as a successful tool result.
- Around line 3390-3391: get_kb_resource and get_kb_category_resource return
potentially large rendered KB dicts without applying truncate_response and
get_kb_answer_resource only truncates the body; update these functions
(get_kb_resource, get_kb_category_resource, get_kb_answer_resource) to call
truncate_response on the final rendered resource (the value returned by
_format_kb_markdown or the assembled answer/category dict) before returning to
ensure attachments, id lists and metadata are size-limited; preserve current
formatting logic (e.g., _format_kb_markdown) and call truncate_response on the
completed object/string right before the return in each function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 202a754d-f642-4168-803a-ff77fa1a3176
📒 Files selected for processing (3)
mcp_zammad/client.pymcp_zammad/server.pytests/test_kb.py
client.py: - Add _expand_category_ids() BFS helper that traverses child_ids to build a flat list of all descendant category IDs from a set of roots; tolerates ZammadAPIError/KeyError/ValueError on individual categories so one missing category does not abort the whole expansion - search_kb_answers: use _expand_category_ids() so nested subcategories are included in search (previously only root category_ids from the KB were used, missing answers in any subcategory) server.py: - get_kb_resource: wrap _format_kb_markdown() with truncate_response() before returning to prevent unbounded response size - get_kb_category_resource: wrap _format_kb_category_markdown() with truncate_response() before returning tests/test_kb.py: - test_expand_category_ids_includes_descendants: verifies BFS traversal through child_ids produces [root, child] list - test_search_kb_answers_include client.py: - Add _expand_category_ids() BFS helper that traverses child_ids to build a flat list of all descendant category I by- Add _exb_ a flat list of all
…low 8 Extract _answer_matches_query and _collect_category_answers helpers so _answers_matching_query is a simple loop with no branches (CC=1). _collect_category_answers contains the try/except logic (CC=4). Fixes Codacy MC0001 warning on _answers_matching_query.
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
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.
my take at adding support for the knowledge base. This was generated using windsurf AI mainly.
Summary by CodeRabbit
New Features
Documentation
Tests