Conversation
Three small but real type bugs in statuspro_mcp_server, surfaced when the LSP (pyright) re-analyzed orders.py after the param-annotation pass and confirmed pre-existing in main. ty (the project's validation checker) misses them today because statuspro_mcp_server/ is in the exclude list — see #56. - schemas.py: `BatchOrderResult.order_id` and `.error` used the positional `Field(None, …)` form, which pyright reads as default=missing — flagging every constructor that omitted the field. Switched to `Field(default=None, …)` so the default is unambiguous. - orders.py: `UpdateOrderStatusRequest(comment=comment, …)` and `BulkStatusUpdateRequest(comment=comment, …)` were passing the tool's `str | None` parameter into attrs fields typed `str | Unset`. Wrapped with `to_unset(comment)` per CLAUDE.md's documented None → UNSET convention. - orders.py: `_bounded_gather[T]` and the inner `bounded[T]` declared a TypeVar that appeared only in the return type, so pyright couldn't infer T at any call site (warning: "TypeVar T appears only once"). Typed the input as `list[Awaitable[T]]` / `Awaitable[T]` so T binds from the caller's coroutines and the return shape carries the element type through. Behaviorally identical; type-correctness only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tool param annotations had drifted: bare `int` / `bool` types on some
mutation tools, `Annotated[int, Field(description=…)]` wrappers on
others, and one outlier description ("Must be true to apply the
change" vs. "Set to true to apply the change") on `confirm`. The MCP
schema FastMCP advertises to clients was inconsistent as a result.
- New `tools/param_types.py` module — mirrors the
`tools/list_coercion.py` convention of collapsing repeated
`Annotated[..., Field(...)]` boilerplate into a single readable
alias at the call site:
- `OrderIdParam = Annotated[int, Field(description="StatusPro order id")]`
- `ConfirmFlag = Annotated[bool, Field(description="Set to true to apply the change")]`
- `orders.py` — `order_id` on every mutation tool (5 sites) now uses
`OrderIdParam`; `confirm` on every mutation tool (4 sites) now uses
`ConfirmFlag`. Normalizes the previously-divergent `update_order_status`
description.
- `statuses.py` — `get_viable_statuses` `order_id` adopts `OrderIdParam`;
drops the now-unused `Annotated` / `Field` imports.
Behavior unchanged. Tool JSON schema descriptions are now uniform
across all mutation tools, and adding a new mutation tool is one
import + one annotation rather than 14 characters of `Annotated[…]`
boilerplate per parameter.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens and standardizes typing for StatusPro MCP tool parameters and fixes a few latent typing mismatches that pyright surfaced in statuspro_mcp_server/, without intended behavior changes.
Changes:
- Fixes Pydantic
Field(...)defaults in shared tool schemas so optional fields are treated as optional by type checkers. - Aligns optional
commenthandling with attrs request models by convertingNonetoUNSETviato_unset(...). - Extracts reusable
Annotated[..., Field(...)]parameter aliases (OrderIdParam,ConfirmFlag) and applies them across tools for consistent JSON schema metadata.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
statuspro_mcp_server/src/statuspro_mcp/tools/statuses.py |
Uses OrderIdParam for consistent tool param typing/schema metadata. |
statuspro_mcp_server/src/statuspro_mcp/tools/schemas.py |
Makes optional schema fields explicitly default=None to satisfy static typing expectations. |
statuspro_mcp_server/src/statuspro_mcp/tools/param_types.py |
Introduces shared Annotated aliases to avoid drift across tool definitions. |
statuspro_mcp_server/src/statuspro_mcp/tools/orders.py |
Binds generic type params properly for async helpers and normalizes order_id/confirm param annotations; converts comment via to_unset. |
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
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
Two-commit PR cleaning up MCP tool parameter typing in
statuspro_mcp_server/. Surfaced during a/techdebt-scanfollowed bycode-modernizerinvocation, where pyright (LSP) caught real type errors thatty(validation) silently misses today (see #56).Commit 1 —
fix(mcp): correct latent type errors pyright surfacedThree small, behavior-preserving type bugs:
tools/schemas.py—BatchOrderResult.order_idand.errorswitched from positionalField(None, …)(pyright reads as required) toField(default=None, …). Fixes "Argument missing for parameterorder_id" at three constructor sites inorders.py.tools/orders.py—UpdateOrderStatusRequest(comment=…)andBulkStatusUpdateRequest(comment=…)were passingstr | Nonefrom the tool param into attrs fields typedstr | Unset. Wrapped withto_unset(comment)per CLAUDE.md's None → UNSET convention.tools/orders.py—_bounded_gather[T]and innerbounded[T]declared a TypeVar that appeared only in the return type. Typed inputs aslist[Awaitable[T]]/Awaitable[T]so T binds at call sites.Commit 2 —
refactor(mcp): extract OrderIdParam and ConfirmFlag tool param aliasesTool param annotations had drifted: bare
int/booltypes on some mutation tools,Annotated[int, Field(description=…)]wrappers on others, and one outlierconfirmdescription. Newtools/param_types.pymirrors thetools/list_coercion.pyconvention of collapsing repeatedAnnotated[…, Field(…)]boilerplate into a single alias.OrderIdParam— used by 6 tools (get_order,get_order_history,update_order_status,add_order_comment,update_order_due_date,get_viable_statuses)ConfirmFlag— used by 4 mutation toolsWhy
Both classes of bug were latent — they'd been incorrect for as long as the code's been there, and
ty check --exclude statuspro_mcp_server/never reported them. Concrete data point added to #56 (comment).Test plan
uv run poe check— ruff format/check, prettier, ty, yamllint, pytest 295 passedto_unset(None)returnsUNSET,to_unset("text")returns"text"(perdomain/converters.py)Annotated[…, Field(…)]at registration time only (cached); no per-call overhead from the alias indirection🤖 Generated with Claude Code