feat: add zammad_merge_tickets tool via legacy ticket_merge REST endpoint - #312
feat: add zammad_merge_tickets tool via legacy ticket_merge REST endpoint#312iisat wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughChangesTicket merge flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 7 |
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.
2c2fbb1 to
d8268ce
Compare
…oint
Wraps PUT /api/v1/ticket_merge/{source_id}/{target_number} (zammad_py does
not expose merge; the newer PUT /tickets/{id}/merge route 404s on some
instances). Target can be given by display number or internal ID (number
is looked up). Failed merges (HTTP 200 + result='failed') raise ValueError
instead of being reported as success.
Tests: 6 client + 6 model cases; suite 236 passed.
d8268ce to
1314c94
Compare
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`:
- Line 300: Validate target_ticket_number in TicketMergeParams against the
expected ticket-number format, preferably digits only, before it is used by the
ticket merge request. Ensure the validation rejects path separators, traversal
sequences, and other nonconforming values while preserving valid ticket numbers;
alternatively, URL-encode the value at the request construction near the
session.put call.
- Around line 296-298: Make target-ticket lookup failures distinguishable in
mcp_zammad/client.py lines 296-298 by handling
self.api.ticket.find(target_ticket_id) and raising an error that identifies the
target. In mcp_zammad/server.py lines 1227-1233, inspect that error before
calling _handle_ticket_not_found_error and report target-related failures using
target_ticket_id or target_ticket_number; retain source_ticket_id for
source-related failures.
In `@tests/test_client_methods.py`:
- Around line 655-744: Add a test to TestMergeTickets covering target-ticket
lookup failure: configure mock_instance.ticket.find to raise the expected
exception for target_ticket_id, call merge_tickets with that ID, and assert the
exception type and message identify the missing target ticket distinctly from
source-ticket merge failures. Verify the merge HTTP endpoint is not called.
🪄 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 Plus
Run ID: c6255243-b61f-4e62-a3f7-364df2081159
📒 Files selected for processing (6)
README.mdmcp_zammad/client.pymcp_zammad/models.pymcp_zammad/server.pytests/test_client_methods.pytests/test_models.py
| target = self.api.ticket.find(target_ticket_id) | ||
| target_ticket_number = str(target["number"]) | ||
|
|
||
| response = self.api.session.put(f"{self.url}/ticket_merge/{source_ticket_id}/{target_ticket_number}") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate target_ticket_number before it reaches the URL path.
target_ticket_number is only constrained to length 1-50 in TicketMergeParams; it is not restricted to a specific character set. It is interpolated directly into f"{self.url}/ticket_merge/{source_ticket_id}/{target_ticket_number}" with no URL-encoding. A crafted value containing / or .. changes the request path sent through the authenticated session.
Restrict the field to the expected ticket-number format (e.g. digits) in the model, or URL-encode the value with urllib.parse.quote before building the request path.
As per path instructions for mcp_zammad/client.py, "Check for URL validation to prevent SSRF attacks."
🤖 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` at line 300, Validate target_ticket_number in
TicketMergeParams against the expected ticket-number format, preferably digits
only, before it is used by the ticket merge request. Ensure the validation
rejects path separators, traversal sequences, and other nonconforming values
while preserving valid ticket numbers; alternatively, URL-encode the value at
the request construction near the session.put call.
Source: Path instructions
- Distinguish target-ticket lookup failures from source-ticket errors: wrap the ticket.find() call in client.merge_tickets with a ValueError identifying the TARGET ticket, and report it against target_ticket_id in the server tool instead of always blaming source_ticket_id. - Restrict TicketMergeParams.target_ticket_number to digits only (pattern ^\d+$) since the value is interpolated into the ticket_merge URL path without encoding. - Add tests: non-digit target numbers rejected, target lookup failure is distinguishable, and error attribution (source vs target) in the merge tool.
- Distinguish target-ticket lookup failures from source-ticket errors: wrap the ticket.find() call in client.merge_tickets with a ValueError identifying the TARGET ticket, and report it against target_ticket_id in the server tool instead of always blaming source_ticket_id. - Restrict TicketMergeParams.target_ticket_number to digits only (pattern ^\d+$) since the value is interpolated into the ticket_merge URL path without encoding. - Add tests: non-digit target numbers rejected, target lookup failure is distinguishable, and error attribution (source vs target) in the merge tool.
1d47157 to
1d543d5
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Implements #309 — adds a
zammad_merge_ticketstool that merges one Zammad ticket into another, the same operation as the "Merge" button in the Zammad UI.API choice
The tool wraps the legacy endpoint:
Rationale:
zammad_pydoes not expose ticket merge, so the client performs a raw authenticated request reusing the existing session/credentials.PUT /api/v1/tickets/{id}/mergeroute (withtarget_ticket_numberin the body) returns 404 on some released Zammad versions, whileticket_mergeis the route the web UI itself uses.Behavior
target_ticketaccepts the display ticket number; an internal ticket ID can be passed viatarget_ticket_idinstead (its number is looked up first, since the endpoint requires the number).{"result": "failed"}— the tool raisesValueErrorin that case instead of reporting a false success.merged) and that both customers may see the merged conversation. The tool is annotated as non-read-only.Test plan
uv run pytest --cov=mcp_zammad— 234 passed, coverage 88.27% (gate: 86%)uv run ruff format --check mcp_zammad tests/uv run ruff check mcp_zammad tests— cleanuv run mypy mcp_zammad— cleanresult:"failed", HTTP errors, target lookup) + 6 model validation casesmergedCloses #309
Summary by CodeRabbit
New Features
Documentation