feat(kb): add read-only Knowledge Base support#267
Open
snizzleorg wants to merge 18 commits into
Open
Conversation
Adds the read-only slice of Knowledge Base support requested by the maintainer in basher83#200, in a focused PR designed for review: - ZammadAPIError typed exception for KB API failures. - ZammadClient KB read-only methods (direct HTTP, since zammad-py does not wrap KB endpoints): - list_knowledge_bases (with documented 404 -> ID-probe fallback; all other errors propagate) - get_knowledge_base, get_kb_category - get_kb_answer, get_kb_answer_with_content - list_kb_answers (per-answer 404 tolerated and logged; other errors raise) - search_kb_answers (BFS-expands subcategories, case-insensitive title/body match) - Pydantic models for KnowledgeBase / Category / Answer (+ translations) and StrictBaseModel param models for the new tools. - MCP tools (read-only): - zammad_list_knowledge_bases / zammad_get_knowledge_base - zammad_get_kb_category - zammad_list_kb_answers / zammad_search_kb_answers / zamm Adds the read-only slice of Knowledge Base support requestegormaintainer in basher83#200, in a focused PR designed for review: - Zammadga - ZammadAPIError typed exception for KB API failures. urf- ZammadClient KB read-only methods (direct HTTP, sice not wrap KB endpoints): - list_knowledge_bases (with documentedg - list_knowledge_basal all other errors propagate) - get_knowledge_base, get_kb_cis - get_knowledge_base, get_kbes - get_kb_answer, get_kb_answer_with_eg - list_kb_answers (per-answer 404 tolerate K errors raise) - search_kb_answers (BFS-expands subcate D - search_kb_anes
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 read-only Knowledge Base support: typed client errors and direct HTTP KB endpoints, answer extraction and search with BFS category expansion, Pydantic KB models and params, server Markdown formatting plus MCP tools/resources, and comprehensive tests covering error propagation and formatting. ChangesKnowledge Base Read-Only Integration
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
github-actions
Bot
added
type:bug
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 5 minor |
🟢 Metrics 198 complexity
Metric Results Complexity 198
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
added 2 commits
May 13, 2026 13:10
- Restructure new docstrings to satisfy D213/D406/D407/D413/D203/D107 (blank line before class docstrings, summary on second line, sections rewritten as prose). - Add docstring on ZammadAPIError.__init__. - Drop unused ZammadAPIError import from server.py (KB tools rely on exceptions propagating from the client; the symbol is not referenced directly here). - Replace magic HTTP status numbers (204, 404) with module constants (_HTTP_NO_CONTENT, _HTTP_NOT_FOUND) to address PLR2004. - Fix RUF003 (en-dash in client.py KB section header). - Tests: hoist 'mcp_zammad.server' import to module top (PLC0415), drop decorative section comments that ruff flagged as ERA001, sort imports. No behavioral changes.
The vendored llm-anon-core directory belongs to the PII feature on the fork's main branch and is not part of this read-only KB slice.
![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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp_zammad/client.py`:
- Around line 532-549: The _probe_kb_ids fallback currently only checks IDs
1..10 and can miss KBs with higher IDs; change _probe_kb_ids to scan a
larger/robust ID range instead of the fixed range by iterating from 1 upward and
stopping when a configurable max_id is reached or when a threshold of
consecutive 404s (e.g., 50) is observed to avoid unbounded scanning; keep using
self.api.session.get(self._kb_url(kb_id)) and _kb_raise_or_return(response),
append any non-empty dict results to results, and update the docstring to
reflect the new stopping criteria and configuration points.
- Around line 525-526: The code currently treats a 204 or empty-body GET
response as an empty dict ("if response.status_code == 204 or not
response.content: return {}"), which masks API failures; change the logic so
that when a GET returns 204 or has no content you raise a clear, typed exception
(or return a failure result) instead of returning {}. Locate the response
handling block that checks response.status_code/response.content (the existing
"if response.status_code == 204 or not response.content" conditional) and
replace the silent-empty return with raising an HTTP/error (or a custom
exception) that includes the status_code and any error body, and update callers
of this function to handle that exception accordingly.
- Around line 513-524: The code uses response.ok (which doesn't exist on HTTPX
Response) causing AttributeError; update the checks to use response.is_success
instead. In particular, change the conditional in _kb_raise_or_return to use
response.is_success when deciding to parse/raise and update the other occurrence
that references response.ok (the call near line 562) to use response.is_success
as well; keep the raise of ZammadAPIError(response.status_code, response.url,
body) and the existing JSON/text parsing logic unchanged.
In `@tests/test_kb_readonly.py`:
- Around line 32-35: Update the type annotation on the helper function
_make_response to replace the use of Any with a stricter type (object | None) in
its signature so it satisfies ANN401; change the parameter declaration from
json_body: Any = None to json_body: object | None = None and ensure any places
that call _make_response remain compatible (no runtime behavior changes).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dedd4345-2fa3-4e2d-8404-9e08cd3e8496
📒 Files selected for processing (4)
mcp_zammad/client.pymcp_zammad/models.pymcp_zammad/server.pytests/test_kb_readonly.py
added 2 commits
May 13, 2026 13:13
- Replace response.ok with library-agnostic 2xx status_code check via
new _is_2xx() helper. zammad_py currently uses requests where .ok
exists, but this also keeps the code working if it ever migrates to
HTTPX (which exposes .is_success instead of .ok).
- _kb_raise_or_return now raises ZammadAPIError for 204 / empty-body
responses on KB read endpoints instead of returning {}, so unexpected
API states are not silently masked.
- Widen _probe_kb_ids range from 1..10 to 1..200 with an explicit
consecutive-404 break threshold (50). Prevents missing KBs whose IDs
are above 10 (e.g. after deletes/migrations) while still bounding
worst-case work on instances without any KBs.
- Tests:
- replace 'Any' with 'object | None' in _make_response (ANN401)
- update 204 test to expect ZammadAPIError
- replace fixed-range probe test with one that exercises the new
consecutive-miss stop (51 calls expected)
vendor/llm-anon-core is a local checkout used only by feature branches (PII anonymisation on the fork's main); it must not be part of the read-only KB slice. Adds vendor/ to .gitignore to prevent reoccurrence.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp_zammad/client.py`:
- Around line 829-850: The BFS in _expand_category_ids uses a Python list and
list.pop(0) which is O(n); replace the queue with collections.deque for O(1)
pops: import deque from collections, initialize queue = deque(root_ids) (and
update the queue type annotation to deque[int] or collections.deque[int]) and
change queue.pop(0) to queue.popleft(); all other logic (seen/visited handling,
get_kb_category error handling, child_ids loop) remains unchanged.
In `@tests/test_kb_readonly.py`:
- Around line 234-237: Remove the dead-code that checks for and stores
kb_client.get_kb_answer_with_content.__wrapped__ into a temporary variable and
then immediately deletes it; specifically delete the lines creating and deleting
the variable `info` (the hasattr(...) retrieval and the `del info`), leaving the
test to directly exercise extractors without that unused check, and run the
tests to ensure no behavioral changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6fca0652-e056-4520-b76d-a56c543dfcec
📒 Files selected for processing (4)
.gitignoremcp_zammad/client.pymcp_zammad/server.pytests/test_kb_readonly.py
Comment on lines
+829
to
+850
| def _expand_category_ids(self, kb_id: int, root_ids: list[int]) -> list[int]: | ||
| """BFS-expand root category IDs into all descendants via child_ids.""" | ||
| visited: list[int] = [] | ||
| queue: list[int] = list(root_ids) | ||
| seen: set[int] = set() | ||
| while queue: | ||
| cid = queue.pop(0) | ||
| if cid in seen: | ||
| continue | ||
| seen.add(cid) | ||
| visited.append(cid) | ||
| try: | ||
| category = self.get_kb_category(kb_id, cid) | ||
| except ZammadAPIError as exc: | ||
| if exc.status_code == _HTTP_NOT_FOUND: | ||
| logger.warning("KB category %d not found while expanding tree", cid) | ||
| continue | ||
| raise | ||
| for child_id in category.get("child_ids") or []: | ||
| if child_id not in seen: | ||
| queue.append(child_id) | ||
| return visited |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider using collections.deque for the BFS queue.
Using list.pop(0) is O(n) per pop operation. For a BFS queue, collections.deque provides O(1) popleft(). While the category count is typically small, this is a simple improvement for algorithmic correctness.
♻️ Suggested improvement
+from collections import deque
+
def _expand_category_ids(self, kb_id: int, root_ids: list[int]) -> list[int]:
"""BFS-expand root category IDs into all descendants via child_ids."""
visited: list[int] = []
- queue: list[int] = list(root_ids)
+ queue: deque[int] = deque(root_ids)
seen: set[int] = set()
while queue:
- cid = queue.pop(0)
+ cid = queue.popleft()
if cid in seen:
continue
seen.add(cid)🧰 Tools
🪛 Ruff (0.15.12)
[warning] 849-849: Use list.extend to create a transformed list
(PERF401)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp_zammad/client.py` around lines 829 - 850, The BFS in _expand_category_ids
uses a Python list and list.pop(0) which is O(n); replace the queue with
collections.deque for O(1) pops: import deque from collections, initialize queue
= deque(root_ids) (and update the queue type annotation to deque[int] or
collections.deque[int]) and change queue.pop(0) to queue.popleft(); all other
logic (seen/visited handling, get_kb_category error handling, child_ids loop)
remains unchanged.
Comment on lines
+234
to
+237
| info = kb_client.get_kb_answer_with_content.__wrapped__ if hasattr( | ||
| kb_client.get_kb_answer_with_content, "__wrapped__" | ||
| ) else None | ||
| del info # not used; we exercise extractors directly below |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Remove unnecessary dead code.
These lines check for __wrapped__ attribute, store it in info, then immediately delete it. The comment acknowledges it's unused. This can be removed entirely for cleaner test code.
♻️ Suggested cleanup
- info = kb_client.get_kb_answer_with_content.__wrapped__ if hasattr(
- kb_client.get_kb_answer_with_content, "__wrapped__"
- ) else None
- del info # not used; we exercise extractors directly below📝 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
| info = kb_client.get_kb_answer_with_content.__wrapped__ if hasattr( | |
| kb_client.get_kb_answer_with_content, "__wrapped__" | |
| ) else None | |
| del info # not used; we exercise extractors directly below |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_kb_readonly.py` around lines 234 - 237, Remove the dead-code that
checks for and stores kb_client.get_kb_answer_with_content.__wrapped__ into a
temporary variable and then immediately deletes it; specifically delete the
lines creating and deleting the variable `info` (the hasattr(...) retrieval and
the `del info`), leaving the test to directly exercise extractors without that
unused check, and run the tests to ensure no behavioral changes.
added 3 commits
May 13, 2026 13:21
- ZammadAPIError class (mcp_zammad/client.py:31): remove leading blank line and put summary on the first line (D211/D212). - _kb_raise_or_return docstring (mcp_zammad/client.py:528): keep multi-line layout but move summary to the second line (D213). - Test module docstring (tests/test_kb_readonly.py:1): summary on the first line (D212). - TestToolFailureSemantics class (tests/test_kb_readonly.py:370): remove leading blank line and put summary on the first line (D211/D212). Notes: - D203 (1 blank line required before class docstring) directly conflicts with D211 in pydocstyle; we satisfy D211 here and accept that D203 may surface as a low-priority informational finding. - No behavioural changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
mcp_zammad/client.py (1)
553-573:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let the probe abort before it reaches the first live KB.
This still breaks on instances where the first surviving knowledge base ID is
51+: the fallback exits after 50 initial404s and reports an empty system even though KBs exist.Suggested fix
- Some Zammad versions return 404 on GET /knowledge_bases even when KBs - exist, so we probe individual IDs. Iteration starts at 1 and stops - once either ``_KB_PROBE_MAX_ID`` is reached or after - ``_KB_PROBE_MAX_CONSECUTIVE_MISSES`` consecutive 404 responses, which - bounds the work even on instances with sparse / high IDs. + Some Zammad versions return 404 on GET /knowledge_bases even when KBs + exist, so we probe individual IDs. Iteration starts at 1 and runs + through ``_KB_PROBE_MAX_ID`` so the fallback stays bounded without + missing KBs behind long stretches of deleted IDs. @@ - consecutive_misses = 0 for kb_id in range(1, _KB_PROBE_MAX_ID + 1): response = self.api.session.get(self._kb_url(kb_id)) if response.status_code == _HTTP_NOT_FOUND: - consecutive_misses += 1 - if consecutive_misses >= _KB_PROBE_MAX_CONSECUTIVE_MISSES: - break continue - consecutive_misses = 0 data = self._kb_raise_or_return(response)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp_zammad/client.py` around lines 553 - 573, The probe in _probe_kb_ids can abort after _KB_PROBE_MAX_CONSECUTIVE_MISSES initial 404s and miss a first live KB with a high ID; change the logic so the consecutive-misses cutoff only applies after at least one successful KB fetch has been observed. Concretely, in the _probe_kb_ids method (around the self.api.session.get(self._kb_url(kb_id)) call) introduce a flag like seen_any_success (or require resetting consecutive_misses) so that you do not break on consecutive 404s until you've seen a 2xx response; on success append the parsed KB to results and reset consecutive_misses to 0, and on non-404 non-2xx still raise ZammadAPIError as before. This uses _KB_PROBE_MAX_CONSECUTIVE_MISSES, _KB_PROBE_MAX_ID, self._kb_url and the existing response handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp_zammad/client.py`:
- Around line 709-718: The _body_from_translation_assets function currently only
checks bodies for the provided translation_ids and returns "" when
translation_ids is empty or stale; update it to mirror the title/content helpers
by falling back to scanning all entries in the translations mapping for a legacy
body when no body was found via translation_ids. Specifically, keep the existing
loop over translation_ids and returning self._strip_html(body) when found, and
if that yields nothing, iterate translations.values() (or translations.items())
to locate a content_attributes.body in any legacy translation and return its
stripped HTML; otherwise return "" — reference _body_from_translation_assets and
the analogous title/content helper logic when implementing the fallback.
- Around line 548-551: The code currently calls dict(data) on any non-list JSON
which will raise generic TypeError/ValueError for null/scalar payloads; update
the response handling in the method that does response.json() (the block using
variables response and data) to explicitly accept dicts and lists only and raise
a ZammadAPIError for any other 2xx payload shape (include context like status
code and the raw body/representation in the error message). Specifically:
replace the dict(data) fallback with an isinstance(data, dict) branch that
returns data, and in the final else raise ZammadAPIError("Unexpected 2xx
response payload", status=response.status_code, body=data) or similar to
preserve the ZammadAPIError contract.
---
Duplicate comments:
In `@mcp_zammad/client.py`:
- Around line 553-573: The probe in _probe_kb_ids can abort after
_KB_PROBE_MAX_CONSECUTIVE_MISSES initial 404s and miss a first live KB with a
high ID; change the logic so the consecutive-misses cutoff only applies after at
least one successful KB fetch has been observed. Concretely, in the
_probe_kb_ids method (around the self.api.session.get(self._kb_url(kb_id)) call)
introduce a flag like seen_any_success (or require resetting consecutive_misses)
so that you do not break on consecutive 404s until you've seen a 2xx response;
on success append the parsed KB to results and reset consecutive_misses to 0,
and on non-404 non-2xx still raise ZammadAPIError as before. This uses
_KB_PROBE_MAX_CONSECUTIVE_MISSES, _KB_PROBE_MAX_ID, self._kb_url and the
existing response handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c1954fb-2058-41e5-a846-9442cebad6bd
📒 Files selected for processing (1)
mcp_zammad/client.py
Comment on lines
+548
to
+551
| data = response.json() | ||
| if isinstance(data, list): | ||
| return data | ||
| return dict(data) |
Contributor
There was a problem hiding this comment.
Raise a typed error for unexpected 2xx payload shapes.
dict(data) will throw a generic TypeError/ValueError for JSON null or scalar bodies, so KB callers lose the ZammadAPIError contract on malformed success responses.
Suggested fix
data = response.json()
if isinstance(data, list):
return data
- return dict(data)
+ if isinstance(data, dict):
+ return data
+ raise ZammadAPIError(
+ response.status_code,
+ response.url,
+ f"Unexpected KB response shape: {type(data).__name__}",
+ )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp_zammad/client.py` around lines 548 - 551, The code currently calls
dict(data) on any non-list JSON which will raise generic TypeError/ValueError
for null/scalar payloads; update the response handling in the method that does
response.json() (the block using variables response and data) to explicitly
accept dicts and lists only and raise a ZammadAPIError for any other 2xx payload
shape (include context like status code and the raw body/representation in the
error message). Specifically: replace the dict(data) fallback with an
isinstance(data, dict) branch that returns data, and in the final else raise
ZammadAPIError("Unexpected 2xx response payload", status=response.status_code,
body=data) or similar to preserve the ZammadAPIError contract.
Comment on lines
+709
to
+718
| def _body_from_translation_assets( | ||
| self, translations: dict[str, Any], translation_ids: list[int] | ||
| ) -> str: | ||
| """Extract plain-text body from translation content_attributes (legacy).""" | ||
| for tid in translation_ids: | ||
| t = translations.get(str(tid)) or {} | ||
| body = (t.get("content_attributes") or {}).get("body") or "" | ||
| if body: | ||
| return self._strip_html(body) | ||
| return "" |
Contributor
There was a problem hiding this comment.
Keep the legacy body fallback consistent with the title/content helpers.
When translation_ids is empty or stale, this path returns "" even if a legacy translation body exists. That makes those answers searchable by title only.
Suggested fix
def _body_from_translation_assets(
self, translations: dict[str, Any], translation_ids: list[int]
) -> str:
"""Extract plain-text body from translation content_attributes (legacy)."""
for tid in translation_ids:
t = translations.get(str(tid)) or {}
body = (t.get("content_attributes") or {}).get("body") or ""
if body:
return self._strip_html(body)
- return ""
+ first_body = (
+ (next(iter(translations.values()), {}) or {}).get("content_attributes") or {}
+ ).get("body") or ""
+ return self._strip_html(first_body) if first_body else ""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp_zammad/client.py` around lines 709 - 718, The
_body_from_translation_assets function currently only checks bodies for the
provided translation_ids and returns "" when translation_ids is empty or stale;
update it to mirror the title/content helpers by falling back to scanning all
entries in the translations mapping for a legacy body when no body was found via
translation_ids. Specifically, keep the existing loop over translation_ids and
returning self._strip_html(body) when found, and if that yields nothing, iterate
translations.values() (or translations.items()) to locate a
content_attributes.body in any legacy translation and return its stripped HTML;
otherwise return "" — reference _body_from_translation_assets and the analogous
title/content helper logic when implementing the fallback.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/test_kb_readonly.py (1)
234-237: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove dead code.
These lines check for
__wrapped__, store it ininfo, then immediately delete it. The comment confirms it's unused. This was flagged in a previous review and marked as addressed, but the code is still present.♻️ Suggested cleanup
- info = kb_client.get_kb_answer_with_content.__wrapped__ if hasattr( - kb_client.get_kb_answer_with_content, "__wrapped__" - ) else None - del info # not used; we exercise extractors directly below answer = kb_client._extract_kb_answer_from_payload(payload, 7)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_kb_readonly.py` around lines 234 - 237, Remove the dead-code that captures and immediately deletes the wrapper of kb_client.get_kb_answer_with_content: delete the lines that assign to the temporary variable info via hasattr(..., "__wrapped__") and the subsequent del info, since info is unused and the comment indicates the extractors are exercised directly; locate the code around kb_client.get_kb_answer_with_content and remove the info assignment and deletion to clean up the test.mcp_zammad/client.py (2)
712-717:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep legacy body extraction aligned with the other translation fallbacks.
When
translation_idsis empty or stale, this returns""even iftranslationsstill contains a legacycontent_attributes.body. Those answers become searchable by title only.🔧 Suggested fix
for tid in translation_ids: t = translations.get(str(tid)) or {} body = (t.get("content_attributes") or {}).get("body") or "" if body: return self._strip_html(body) + for translation in translations.values(): + body = ((translation or {}).get("content_attributes") or {}).get("body") or "" + if body: + return self._strip_html(body) return ""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp_zammad/client.py` around lines 712 - 717, The current loop over translation_ids returns "" when translation_ids is empty or stale even though legacy bodies may exist in the translations mapping; update the extraction logic in the function that uses translation_ids/translations to, after the existing loop, iterate over translations.values() (or the dict items) and check each entry's (entry.get("content_attributes") or {}).get("body") and return self._strip_html(body) on first found legacy body before returning ""; ensure you still call self._strip_html and preserve the original priority of translation_ids over legacy entries.
547-550:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
ZammadAPIErrorfor malformed 2xx KB responses.A success body that is invalid JSON,
null, or a scalar still leaksValueError/TypeErrorfrom this helper instead of the typed API error the new KB layer is meant to provide.🔧 Suggested fix
- data = response.json() + try: + data = response.json() + except ValueError as exc: + raise ZammadAPIError(response.status_code, response.url, response.text) from exc if isinstance(data, list): return data - return dict(data) + if isinstance(data, dict): + return data + raise ZammadAPIError( + response.status_code, + response.url, + f"Unexpected KB response shape: {type(data).__name__}", + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp_zammad/client.py` around lines 547 - 550, The helper currently calls response.json() and lets JSON parsing errors or unexpected scalar/null responses bubble up as ValueError/TypeError; instead, wrap the JSON decoding in a try/except that catches ValueError/TypeError around response.json(), and if an exception occurs or the decoded data is not a list or a mapping, raise a ZammadAPIError (including context such as response.status_code and the raw body/decoded value) rather than returning or leaking the original exception; update the logic around the data variable handling (the block that checks isinstance(data, list) / return dict(data)) to validate types and raise ZammadAPIError for malformed 2xx KB responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp_zammad/client.py`:
- Around line 575-577: The loop in list_knowledge_bases currently calls
_kb_raise_or_return(response) and silently skips non-dict or empty dict
payloads, which can hide malformed per-ID GET /knowledge_bases/{id} responses;
instead, ensure malformed or unexpected 200 payloads raise an exception. Update
the code around list_knowledge_bases where results.append is done to validate
the returned data from _kb_raise_or_return(response): if the return is not a
non-empty dict (or does not match the expected KB schema), raise a clear error
(e.g., ValueError or a specific ApiError) including the raw response content and
the id, rather than ignoring it, or change _kb_raise_or_return to perform this
validation and raise for malformed payloads so results only ever receives valid
KB objects.
In `@tests/test_kb_readonly.py`:
- Around line 1-9: The module-level docstring in tests/test_kb_readonly.py has a
blank line after the opening triple quotes which violates pydocstyle D212; edit
the top docstring so the summary line starts immediately after the opening
quotes (no leading blank line) and keep the rest of the paragraph/description
below it, ensuring the first line is the brief summary and there is a blank line
before any extended description if present.
- Around line 371-372: Remove the stray blank line between the class declaration
and its docstring for class TestToolFailureSemantics so the docstring
immediately follows the line "class TestToolFailureSemantics:"; edit the class
definition to place the triple-quoted docstring directly below the class header
with no intervening blank line to satisfy pydocstyle D211.
---
Duplicate comments:
In `@mcp_zammad/client.py`:
- Around line 712-717: The current loop over translation_ids returns "" when
translation_ids is empty or stale even though legacy bodies may exist in the
translations mapping; update the extraction logic in the function that uses
translation_ids/translations to, after the existing loop, iterate over
translations.values() (or the dict items) and check each entry's
(entry.get("content_attributes") or {}).get("body") and return
self._strip_html(body) on first found legacy body before returning ""; ensure
you still call self._strip_html and preserve the original priority of
translation_ids over legacy entries.
- Around line 547-550: The helper currently calls response.json() and lets JSON
parsing errors or unexpected scalar/null responses bubble up as
ValueError/TypeError; instead, wrap the JSON decoding in a try/except that
catches ValueError/TypeError around response.json(), and if an exception occurs
or the decoded data is not a list or a mapping, raise a ZammadAPIError
(including context such as response.status_code and the raw body/decoded value)
rather than returning or leaking the original exception; update the logic around
the data variable handling (the block that checks isinstance(data, list) /
return dict(data)) to validate types and raise ZammadAPIError for malformed 2xx
KB responses.
In `@tests/test_kb_readonly.py`:
- Around line 234-237: Remove the dead-code that captures and immediately
deletes the wrapper of kb_client.get_kb_answer_with_content: delete the lines
that assign to the temporary variable info via hasattr(..., "__wrapped__") and
the subsequent del info, since info is unused and the comment indicates the
extractors are exercised directly; locate the code around
kb_client.get_kb_answer_with_content and remove the info assignment and deletion
to clean up the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc5d6583-93ff-420d-8d1c-9cf6cf41380d
📒 Files selected for processing (2)
mcp_zammad/client.pytests/test_kb_readonly.py
Comment on lines
+575
to
+577
| data = self._kb_raise_or_return(response) | ||
| if isinstance(data, dict) and data: | ||
| results.append(data) |
Contributor
There was a problem hiding this comment.
Don’t silently drop malformed per-ID KB probe responses.
GET /knowledge_bases/{id} should either yield one KB object or fail loudly. Right now unexpected 200 payloads are treated as “not found”, so list_knowledge_bases() can return an incomplete result set without any error.
🔧 Suggested fix
data = self._kb_raise_or_return(response)
- if isinstance(data, dict) and data:
- results.append(data)
+ if not isinstance(data, dict) or not data:
+ raise ZammadAPIError(
+ response.status_code,
+ response.url,
+ data if data else "Empty knowledge_base payload during probe",
+ )
+ results.append(data)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp_zammad/client.py` around lines 575 - 577, The loop in
list_knowledge_bases currently calls _kb_raise_or_return(response) and silently
skips non-dict or empty dict payloads, which can hide malformed per-ID GET
/knowledge_bases/{id} responses; instead, ensure malformed or unexpected 200
payloads raise an exception. Update the code around list_knowledge_bases where
results.append is done to validate the returned data from
_kb_raise_or_return(response): if the return is not a non-empty dict (or does
not match the expected KB schema), raise a clear error (e.g., ValueError or a
specific ApiError) including the raw response content and the id, rather than
ignoring it, or change _kb_raise_or_return to perform this validation and raise
for malformed payloads so results only ever receives valid KB objects.
Author
|
For mcp_zammad/client.py class ZammadAPIError and for the module/class docstrings in tests/test_kb_readonly.py, one Codacy run requires D211 + D212, then the next run requires D203 + D213 (and vice versa). It’s impossible to satisfy both at the same time. Could you please deconflict the pydocstyle configuration used by Codacy (disable one of each conflicting pair: D203 vs D211, D212 vs D213) or align it with the project’s intended style? |
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.
Summary
Splits the parked PR #200 as recommended by the maintainer. This is slice 1: read-only Knowledge Base discovery only. No writes, no attachments, no docs changes. Designed to be small enough to review in one pass.
Rebased on current main. All 251 tests pass.
What's included
zammad-pydoes not wrap KB endpoints):404 → ID-probecompatibility fallback; all other error statuses (401/403/5xx/empty/unexpected shape) raise ZammadAPIError.404is tolerated and logged (a listed answer ID may have been deleted in a race); all other errors raise.zammad://kb/{kb_id},zammad://kb/{kb_id}/category/{category_id},zammad://kb/{kb_id}/answer/{answer_id}Maintainer concerns directly addressed
Out of scope (will follow as separate slices)
How to test
Result:
251 passed.Closes the read-only portion of #200; #200 itself can stay parked while the remaining slices land.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores