fix(prompts): accept string arguments for ticket_id - #310
Conversation
MCP prompt arguments are always transmitted as strings; PromptArgument has no type field. Annotating ticket_id as int made FastMCP's _convert_string_arguments coerce the incoming value via pydantic, so any non-numeric string raised PromptError and the prompt could not be rendered. Clients that materialize MCP prompts into slash commands discover their arguments by rendering them with placeholder values. opencode, for example, issues prompts/get with "$1"/"$2", which fails: PromptError: Could not convert argument 'ticket_id' with value '$1' to expected type <class 'int'> This made analyze_ticket and draft_response unusable in those clients, while escalation_summary worked because group is annotated str | None. Annotate ticket_id as str. Both prompts only interpolate it into the returned f-string, so this is behaviour-preserving; the docstrings already document that callers must pass the internal database ID. Add a parametrized regression test that renders all three prompts with non-numeric arguments. It fails on analyze_ticket and draft_response before this change and passes for escalation_summary throughout.
|
Warning Review limit reached
Next review available in: 48 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 (1)
WalkthroughThe MCP ChangesPrompt string argument support
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 1 minor |
🟢 Metrics 0 complexity
Metric Results Complexity 0
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@tests/test_server.py`:
- Around line 235-256: Add tests covering the remaining untested Python paths to
raise overall coverage from 88.42% to at least 90%. Extend the existing async
prompt tests around mcp.render_prompt and inspect related server functions for
uncovered branches, preserving current behavior while exercising each missing
path.
- Line 237: Update the parametrization names near the `"name,arguments"` entry
to use a tuple of individual names, `("name", "arguments")`, so the test
satisfies Ruff PT006 while preserving the existing parameter order.
🪄 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: 38b0733d-2032-42ee-ba02-51548d7b40fb
📒 Files selected for processing (2)
mcp_zammad/server.pytests/test_server.py
| @pytest.mark.asyncio | ||
| @pytest.mark.parametrize( | ||
| "name,arguments", | ||
| [ | ||
| ("analyze_ticket", {"ticket_id": "$1"}), | ||
| ("draft_response", {"ticket_id": "$1", "tone": "$2"}), | ||
| ("escalation_summary", {"group": "$1"}), | ||
| ], | ||
| ) | ||
| async def test_prompts_render_with_non_numeric_arguments(name: str, arguments: dict[str, str]) -> None: | ||
| """Prompt arguments arrive as strings, so they must not be coerced to int. | ||
|
|
||
| Clients that turn MCP prompts into slash commands render them with placeholder | ||
| values such as "$1" to discover their arguments. Annotating ticket_id as int made | ||
| FastMCP raise PromptError on those placeholders, so the prompts were unusable. | ||
| """ | ||
| result = await mcp.render_prompt(name, arguments) | ||
|
|
||
| rendered = result.messages[0].content.text # type: ignore[union-attr] | ||
| assert "$1" in rendered | ||
|
|
||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Raise total coverage to the required threshold.
The supplied PR results report 88.42% coverage, below the repository’s 90% minimum. Add coverage for the remaining Python paths before merging.
As per coding guidelines, maintain test coverage at 90%+ for all Python code.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 237-237: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
🤖 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_server.py` around lines 235 - 256, Add tests covering the
remaining untested Python paths to raise overall coverage from 88.42% to at
least 90%. Extend the existing async prompt tests around mcp.render_prompt and
inspect related server functions for uncovered branches, preserving current
behavior while exercising each missing path.
Source: Coding guidelines
Addresses ruff PT006. The repo's ruff config does not select PT, so this was not flagged locally, but the tuple form is clearer and costs nothing.
|
Thanks for the review. Addressed one, pushing back on the other. PT006 — fixed in e700308. Worth noting it wasn't flagged locally because Coverage to 90% — I'd argue this is out of scope here. I measured both sides on the same machine:
This PR is coverage-neutral: it adds three tests and no source lines. The prompt bodies it touches were already exercised by The 90% figure also doesn't match what the repo enforces — If you do want the 90% threshold enforced, bumping |
analyze_ticket and draft_response annotate ticket_id as int. MCP prompt arguments are always strings on the wire, so FastMCP coerces them via pydantic and rejects opencode's "$1" placeholder with PromptError, leaving both prompts unusable. escalation_summary is unaffected (group: str | None), which confirms the cause. Point at zoispag/Zammad-MCP@fix/prompt-string-arguments until basher83/Zammad-MCP#310 lands, then revert to basher83. This pin was applied once before and lost during later opencode config edits; committing it so it survives checkouts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Problem
analyze_ticketanddraft_responseannotateticket_idasint:MCP prompt arguments are always transmitted as strings —
PromptArgumenthas no type field. FastMCP's_convert_string_arguments(fastmcp/prompts/function_prompt.py) therefore coerces the incoming value to the annotation via pydantic, and any non-numeric string raisesPromptErrorinstead of rendering.Clients that materialize MCP prompts into slash commands discover their arguments by rendering them with placeholder values. opencode issues
prompts/getwith"$1"/"$2", which fails:The result is that both prompts are unusable in such clients.
escalation_summaryis unaffected becausegroupis annotatedstr | None, which is a useful control: it renders the same"$1"fine.For context on how noisy this is in practice — my local logs carry 896 occurrences of this error, two per session across 448 sessions.
Fix
Annotate
ticket_idasstr. Both functions only interpolate it into the returned f-string — no arithmetic, no comparison — so this is behaviour-preserving. The docstrings already document that callers must pass the internal database ID rather than the display number, and that guidance is unchanged.Tests
Added a parametrized regression test rendering all three prompts through the public
mcp.render_promptAPI with non-numeric arguments.Verified it genuinely covers the bug — with the
server.pychange reverted:escalation_summarypasses both before and after, confirming the test is not trivially green.Also updated
test_prompt_handlersto passticket_idas a string, matching the new signature.Full suite on this branch:
uv run ruff format,uv run ruff check, anduv run mypy mcp_zammadare all clean.Summary by CodeRabbit
Bug Fixes
Tests