fix!: remove zammad_delete_attachment, Zammad has no such endpoint

[jovamateus26]

Fixes #320

Problem

zammad_delete_attachment fails on every call:

Resource.destroy() takes 2 positional arguments but 4 were given

client.delete_attachment calls api.ticket_article_attachment.destroy(attachment_id, article_id, ticket_id), but TicketArticleAttachment only overrides download() — its destroy() is the inherited Resource.destroy(self, id).

Correcting the arity would not help. Zammad routes exactly one attachment path (config/routes/ticket.rb):

match api_path + '/ticket_attachment/:ticket_id/:article_id/:id', to: 'ticket_articles#attachment', via: :get

GET only. There is no DELETE counterpart at any URL shape, so the tool cannot be made to work against the Zammad REST API. The closest supported operation is deleting the whole article (DELETE /api/v1/ticket_articles/:id). Verified against Zammad 6.5 sources in a running container.

Change

Removes the dead surface rather than shipping a tool that always errors:

• zammad_delete_attachment tool
• ZammadClient.delete_attachment
• DeleteAttachmentParams, DeleteAttachmentResult
• AttachmentDeletionError (had no other caller)
• the README bullet and the "Delete an Attachment" usage section
• tool count in ARCHITECTURE.md: 22 → 21

Upload, listing and download are untouched — those endpoints exist and work.

Tests

The old tests passed because MagicMock accepts a call signature the real class rejects; a spec'd mock would have caught this at the assert. Replaced them with test_no_attachment_deletion_tool_is_registered, which asserts the tool is absent and records why in the docstring, so it does not silently come back.

215 passed, coverage 88.12% (gate 86%). ruff check, ruff format and mypy mcp_zammad clean.

Your call

I marked this fix! because it drops a tool from the public surface — though since it never worked, no functioning integration can depend on it.

If you would rather keep the name registered, I am happy to rewrite this as: keep the tool, raise a clear NotSupportedError explaining the Zammad limitation and pointing at article deletion. Say the word and I will push that instead.

The historical design docs under docs/plans/ are left as-is, since they record what was planned at the time.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[coderabbitai]

Warning

Review limit reached

@jovamateus26, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 59 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: fef1ae26-057c-4698-852c-a23c6e49d73f

📥 Commits

Reviewing files that changed from the base of the PR and between 8873b2e and cec2851.

📒 Files selected for processing (8)

• ARCHITECTURE.md
• README.md
• mcp_zammad/client.py
• mcp_zammad/models.py
• mcp_zammad/server.py
• tests/test_client.py
• tests/test_models.py
• tests/test_server.py

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 minor

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category	Results
Documentation	1 minor

View in Codacy

🟢 Metrics -4 complexity

Metric	Results
Complexity	-4

View in Codacy

_{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.}

[jovamateus26]

Same note as on #321 about the checks:

GitHub Actions are waiting on your approval — Tests and Coverage, Security Scan, Codacy Security Scan and Build and Publish Docker Image are all at action_required (first-time contributor on a fork), so none of them executed. Locally on this branch: 215 passed, coverage 88.12% (gate 86%), ruff and mypy clean.

Codacy's single "new issue" is an Info-level docs nit, and this PR's own numbers make the case better than I can:

Issues
======
+ Solved 12
- Added   1

The one added is tests/test_server.py:2597 — D213 multi-line docstring summary should start at the second line, on the docstring of the new guard test. The 12 solved are the same family of findings (D213, D203, D406, D407, D413) sitting in the code this PR deletes — i.e. code that was already on main.

So Prospector is enforcing a numpy-ish docstring convention that this project doesn't use anywhere, and the "0 new issues" gate charges it to whichever PR touches a line. Net, this PR removes 11 more findings than it adds.

Happy to reword that one docstring into a single line if you want the gate green, or to send a separate PR setting the pydocstyle convention to google so it stops firing on everyone. Your quality gate, your call — say which and I'll do it.