fix: add pending_time to update_ticket and surface article attachments

[stephaneberle9]

Summary

Two independent, self-contained fixes to the ticket read/write tools, each with tests. No new dependencies, no config/transport changes.

1. feat: add pending_time to update_ticket for pending states

Problem: Moving a ticket to pending reminder / pending close failed with Missing required value for field 'pending_time'!.
Cause: Zammad requires a pending_time for pending states, but the tool (and the client method) never exposed that field.
Remedy: Thread pending_time through TicketUpdateParams, ZammadClient.update_ticket, and the zammad_update_ticket tool (accepts ISO 8601, serialized for the API). A model validator rejects a pending state submitted without pending_time up front, so the model gets an actionable message instead of a raw API error.

2. fix: surface article attachments when reading tickets

Problem: Attachments/PDFs appeared unreadable — reading a ticket returned "only text", so attachment ids were never known and zammad_download_attachment couldn't be used.
Cause: The read Article model had no attachments field, so the per-article attachment metadata returned by the Zammad API was silently dropped (BaseModel ignores extras) and never rendered.
Remedy: Add an attachments field to Article and list each file (id, filename, size) in the markdown ticket/article output with a pointer to zammad_download_attachment, making the existing download path discoverable.

Testing

• uv run pytest — 231 passed
• uv run ruff format --check / ruff check — clean
• uv run mypy mcp_zammad — clean

Notes

Branched off main; contains only these two fixes (an unrelated OAuth feature is intentionally kept out and will be proposed separately).

Summary by CodeRabbit

• New Features

  • Allow setting a ticket's pending-until time when updating a ticket.
  • Articles now include attachment metadata (id, filename, size) with download links.

• Behavior

  • Updating a ticket rejects "pending" states unless a pending-until timestamp is provided; pending time is forwarded/handled as a proper timestamp.

• Documentation

  • Clarified pending-time requirements with an example.

• Tests

  • Added coverage for pending-time handling and article attachment rendering/sanitization.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: eeb75866-ce15-49ca-8281-99a2e10f5dd0

📥 Commits

Reviewing files that changed from the base of the PR and between 44e1952 and 6350eef.

📒 Files selected for processing (2)

• .codacy.yaml
• mcp_zammad/server.py

---

Walkthrough

Adds scheduling support (pending_time) to ticket updates and surfaces article attachments in ticket markdown; includes model changes, client serialization, server rendering/sanitization, docstring examples, and tests.

Changes

Pending Time and Article Attachment Support

Layer / File(s)	Summary
Data models: attachments and pending_time contracts mcp_zammad/models.py, tests/test_models.py	Defines Attachment model and adds `attachments: list[Attachment]
Client: pending_time serialization mcp_zammad/client.py, tests/test_client_methods.py	Imports datetime and extends ZammadClient.update_ticket(..., pending_time: datetime | str | None). Serializes datetime pending_time values to ISO 8601 in the update payload and forwards string values unchanged. Tests validate both behaviors.
Server: pending_time docs and validation mcp_zammad/server.py, tests/test_server.py	Updates zammad_update_ticket docstring/examples to document pending_time semantics (required for pending reminder/close). Tests assert TicketUpdateParams enforces pending_time for pending states and that the tool forwards parsed datetimes to the client.
Server: attachment rendering and sanitization mcp_zammad/server.py, tests/test_server.py	Adds _sanitize_inline_text and _format_article_attachments(...), integrates attachment lines into _format_ticket_detail_markdown and zammad://ticket/{ticket_id} rendering, and tests rendering, filename sanitization, ignoring non-integer sizes, and omitting empty attachment sections.
Codacy config .codacy.yaml	Expands exclude_paths to include tests/** and test filename globs to avoid pydocstyle conflicts with repository linting rules.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• basher83/Zammad-MCP#103: Both PRs touch mcp_zammad/server.py’s ticket detail/resource rendering—specifically how ticket articles are handled when switching to Pydantic model attribute access—so the main PR’s attachment/pending-time markdown changes build on overlapping ticket/Article formatting logic.
• basher83/Zammad-MCP#211: Both PRs modify the same Zammad “ticket update” pathways by extending mcp_zammad/client.py:update_ticket(...) and the mcp_zammad/models.py request model (TicketUpdateParams) with new optional fields—main PR adds pending_time, retrieved PR adds time_unit.
• basher83/Zammad-MCP#101: Both PRs modify mcp_zammad/models.py’s TicketUpdateParams and the zammad_update_ticket tool interface in mcp_zammad/server.py, so the main PR’s added pending_time handling is tied to the retrieved PR’s Pydantic params refactor.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and directly summarizes the two main changes: adding pending_time support to update_ticket and surfacing article attachments, following conventional commit format.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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 and usage tips.}

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

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

[coderabbitai]

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/server.py`:
- Around line 633-640: The attachment metadata is inserted into markdown
unescaped (see the attachments loop using att_id, filename, size_str and the
lines.append call); sanitize/escape these values before formatting: ensure
att_id is safely stringified (no control chars), escape/encode filename to
neutralize Markdown/control characters (implement or call a helper like
escape_markdown or escape_text), and validate/format size into a safe numeric
string, then use those sanitized variables in the lines.append call instead of
raw att.get(...) values.

In `@tests/test_server.py`:
- Around line 1060-1080: Rename the test and update its docstring to reflect
that the server forwards pending_time as a datetime object (not an ISO string):
change the function name test_update_ticket_forwards_pending_time_as_iso to
something like test_update_ticket_forwards_pending_time_as_datetime and update
the docstring in the test function body to state "The tool forwards pending_time
to the client as a datetime object." Keep the existing assertions that check
kwargs["pending_time"] == datetime(..., tzinfo=timezone.utc) and leave the rest
of the test (ZammadMCPServer setup, params, and call to zammad_update_ticket)
unchanged.

🪄 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: 8cf68499-61b3-45d6-af04-266e419d1ddb

📥 Commits

Reviewing files that changed from the base of the PR and between a4379e3 and bebbc34.

📒 Files selected for processing (6)

• mcp_zammad/client.py
• mcp_zammad/models.py
• mcp_zammad/server.py
• tests/test_client_methods.py
• tests/test_models.py
• tests/test_server.py

[coderabbitai]

♻️ Duplicate comments (1)

mcp_zammad/server.py (1)

651-659: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Sanitize article_id in the attachment section header as well.

article_id is still interpolated raw in the header line, while attachment fields are sanitized. If article payloads come from dicts, this leaves an avoidable markdown/control-text injection path.

🔧 Minimal fix

 def _format_article_attachments(attachments: list[Attachment] | list[dict] | None, article_id: int) -> list[str]:
@@
-    lines = [f"- **Attachments** (download via zammad_download_attachment, article_id={article_id}):"]
+    safe_article_id = _sanitize_inline_text(article_id)
+    lines = [f"- **Attachments** (download via zammad_download_attachment, article_id={safe_article_id}):"]

🤖 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/server.py` around lines 651 - 659, The header line building the
attachments section uses raw article_id while other fields are sanitized; update
the code that constructs lines (the first element assigned to variable lines) to
pass article_id through _sanitize_inline_text (e.g., safe_article_id =
_sanitize_inline_text(article_id)) and use that sanitized value in the header
string so the attachments header is protected from injection just like the
per-attachment fields.

🤖 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.

Duplicate comments:
In `@mcp_zammad/server.py`:
- Around line 651-659: The header line building the attachments section uses raw
article_id while other fields are sanitized; update the code that constructs
lines (the first element assigned to variable lines) to pass article_id through
_sanitize_inline_text (e.g., safe_article_id =
_sanitize_inline_text(article_id)) and use that sanitized value in the header
string so the attachments header is protected from injection just like the
per-attachment fields.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9846f0c5-96b1-4fba-b6d8-95bc5064e240

📥 Commits

Reviewing files that changed from the base of the PR and between bebbc34 and 44e1952.

📒 Files selected for processing (2)

• mcp_zammad/server.py
• tests/test_server.py

[stephaneberle9]

Heads-up: the Codacy config is shadowed (root cause of the D203 failure here)

While fixing the static-analysis failure on this PR, I found the repo has three Codacy config files that contradict each other, and Codacy is reading the wrong one:

File	Format	What it says
.codacy.yaml	minimal	(before this PR) only exclude_paths: plugins/**
.codacy.yml	engines: profile	pydocstyle: enabled: false and excludes tests/**
.codacy/codacy.yaml	codacy-cli v2	tools: list (lizard, pmd, pylint, semgrep, trivy)

When both .codacy.yaml and .codacy.yml exist, Codacy uses .yaml and silently ignores .yml. So the comprehensive intent in .codacy.yml (disable pydocstyle, skip test files) was never actually applied — which is why Codacy ran its default pydocstyle profile against a test file and flagged D203 ("1 blank line before class docstring") on a newly added test class. D203 directly conflicts with D211 ("no blank line"), the convention Ruff enforces project-wide, so it's a false-positive style nit rather than a real defect.

What I did in this PR (minimal fix): mirrored the tests/** exclusion into the active .codacy.yaml so the gate stops flagging test docstrings, with an inline comment documenting the shadowing trap.

Suggested follow-up (out of scope here): consolidate to a single source of truth. Either:

• delete .codacy.yml and fold its engines/exclusions into .codacy.yaml, or
• standardize on the codacy-cli .codacy/codacy.yaml and remove the two root files.

Right now a future edit to .codacy.yml would look effective but do nothing, which is an easy trap to fall into again.