Open
Conversation
…ruption - Implement lazy initialization in get_client() to resolve "Zammad client not initialized" errors during stateless HTTP/SSE session teardowns (fixes basher83#202). - Redirect initialization logs to stderr to prevent stdout corruption during stdio transport. - Ensure logging configuration is applied immediately in the default entrypoint.
…dd_article - Update ArticleCreate Pydantic model to include 'to', 'cc', 'subject', and 'content_type' fields. - Update ZammadClient.add_article to accept and pass these email-related fields to Zammad API. - Fix discrepancy between tool docstring and implementation that caused email sending to fail. - Update tests to verify email support and accommodate lazy client initialization.
Contributor
WalkthroughThis PR introduces Docker Compose configuration, configures logging earlier in initialization, implements lazy client instantiation, and extends the article creation API with email-specific fields (subject, to, cc, content_type). Changes span configuration, models, client methods, server initialization, and corresponding test updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 8-10: Replace the plaintext environment variable ZAMMAD_HTTP_TOKEN
with the file-backed secret option supported by the client: stop exporting
ZAMMAD_HTTP_TOKEN in the environment block and instead set
ZAMMAD_HTTP_TOKEN_FILE to the path where the secret will be mounted (e.g.,
/run/secrets/zammad_http_token), add a secrets: entry under the service that
references a Docker secret named zammad_http_token, and define that secret in
the compose file (or mark it external) pointing to a local file (e.g.,
./secrets/zammad_http_token); update any references to ZAMMAD_HTTP_TOKEN in the
compose file to use ZAMMAD_HTTP_TOKEN_FILE so the token is provided via the
secret mount rather than an environment variable.
In `@mcp_zammad/models.py`:
- Around line 325-328: The content_type field currently allows any string and
always causes sanitize_body() to escape the body, breaking text/html and
allowing unsupported values; change content_type to an explicit validated enum
(accept only "text/plain" and "text/html" and default to "text/plain") on the
model (the Field named content_type) and add a validator that rejects/normalizes
other values, then update sanitize_body() to conditionally process the body: if
content_type == "text/plain" escape/encode as before, if content_type ==
"text/html" run an HTML sanitizer (or a safe whitelist/stripper) instead of
escaping, and ensure unsupported values are rejected before reaching the API;
reference the Field content_type and the sanitize_body() function when making
these changes (also apply same validation to the similar fields around lines
333-337).
In `@mcp_zammad/server.py`:
- Around line 823-825: The lazy path in get_client() constructs ZammadClient()
directly, bypassing the startup setup in initialize() that loads .env and
verifies connectivity; instead, call the same setup routine used by initialize()
(or extract that logic into a private helper like _setup_client and invoke it
from both initialize() and get_client()) so the lazy branch loads file-based
config and runs the same auth/connectivity verification before assigning
self.client, and preserve the same error handling/logging behavior.
🪄 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: 3a02436e-41c7-4505-a184-d22c34381cf8
📒 Files selected for processing (6)
docker-compose.ymlmcp_zammad/__main__.pymcp_zammad/client.pymcp_zammad/models.pymcp_zammad/server.pytests/test_server.py
Comment on lines
+325
to
+328
| subject: str | None = Field(default=None, description="Article subject (for emails)") | ||
| to: str | None = Field(default=None, description="Email recipient (for email type)") | ||
| cc: str | None = Field(default=None, description="Email CC recipients") | ||
| content_type: str | None = Field(default=None, description="Content type (text/plain or text/html)") |
Contributor
There was a problem hiding this comment.
content_type support is incomplete at the model boundary.
The new header/content fields are all plain strings right now, and sanitize_body() still escapes every body before it leaves the model. That breaks text/html articles by sending literal tags, while unsupported content_type values still go straight to the API. Validate content_type explicitly and make body escaping conditional on the selected content type.
As per coding guidelines, "Validate all user inputs", "Check that all fields have appropriate defaults and constraints.", and "Verify HTML sanitization is applied where needed."
Also applies to: 333-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp_zammad/models.py` around lines 325 - 328, The content_type field
currently allows any string and always causes sanitize_body() to escape the
body, breaking text/html and allowing unsupported values; change content_type to
an explicit validated enum (accept only "text/plain" and "text/html" and default
to "text/plain") on the model (the Field named content_type) and add a validator
that rejects/normalizes other values, then update sanitize_body() to
conditionally process the body: if content_type == "text/plain" escape/encode as
before, if content_type == "text/html" run an HTML sanitizer (or a safe
whitelist/stripper) instead of escaping, and ensure unsupported values are
rejected before reaching the API; reference the Field content_type and the
sanitize_body() function when making these changes (also apply same validation
to the similar fields around lines 333-337).
Comment on lines
+823
to
+825
| logger.debug("Zammad client not initialized, performing lazy initialization") | ||
| # Lazy initialization for cases where lifespan initialization hasn't finished | ||
| self.client = ZammadClient() |
Contributor
There was a problem hiding this comment.
Lazy get_client() skips the startup setup path.
initialize() loads .env files and eagerly verifies the connection, but the new lazy branch goes straight to ZammadClient(). A request that hits this path before lifespan now ignores file-based config and moves auth/connectivity failures from startup into the request path. Reuse the same setup helper here instead of constructing the client directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp_zammad/server.py` around lines 823 - 825, The lazy path in get_client()
constructs ZammadClient() directly, bypassing the startup setup in initialize()
that loads .env and verifies connectivity; instead, call the same setup routine
used by initialize() (or extract that logic into a private helper like
_setup_client and invoke it from both initialize() and get_client()) so the lazy
branch loads file-based config and runs the same auth/connectivity verification
before assigning self.client, and preserve the same error handling/logging
behavior.
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 of Changes
mcp_zammad/models.py: Added to, cc, subject, and content_type fields to the ArticleCreate Pydantic model. This allows the MCP server to accept these parameters.mcp_zammad/client.py: Updated the add_article method to accept these new parameters and include them in the payload sent to the Zammad API.tests/test_server.py:Summary by CodeRabbit
New Features
Improvements