feat: report tool bugs as MCP errors and collapse the 83x tool template - #88
Conversation
…failures Every tool body was the same template: try, optional _check_write(ctx), a run of `if x is not None: params[...] = x`, the call, and `except Exception as e: return _err(e)`. That last line returned a KeyError or TypeError in the body as a successful result with isError unset and no log line, so a broken tool was indistinguishable from a 404 to the caller (#76), and the template itself was 83 copies of the same 8 lines (#75). tool_result, applied under @mcp.tool, now owns that: - GitLabError subclasses (401/403/404, 409/422/429, read-only) take the _err envelope exactly as before; the contract tests in test_tool_contract.py assert the same key sets and wire requests. - Anything else is logged with _log.exception and re-raised as fastmcp ToolError with the type name in the message, which the client sees as isError: true. Two new tests pin both sides through Client(mcp): a respx RuntimeError side effect and a mocked 404. - write=True runs _check_write first; its failure is expected and takes the envelope. _params(**kw) drops None so the 90 unrolled guards become one keyword call per tool. The handful of truthiness guards (`if search:` and friends) now forward an explicit empty value instead of dropping it; the schema's min_length/ge constraints cover the realistic inputs. gitlab_merge_mr_sequence keeps its own GitLabError handler because its envelope has to say which MRs already merged; bugs still fall through to the decorator. #79 rides along: access_level is Literal["guest", "reporter", "developer", "maintainer", "owner"], so the schema rejects a bad value and both hand-rolled checks that answered on the success channel are gone. The existing "bogus level" test now expects the rejection and asserts no request was made. functools.wraps keeps the signature fastmcp introspects; the assembly test still counts 83 tools and every contract row still passes. servers/gitlab.py: 1039 -> 510 statements, coverage 100%. Tests 405 -> 408. Changelog: Unexpected server errors are now reported as MCP tool errors (isError: true) instead of successful results; expected API errors (401/403/404/409/429, read-only, validation) are unchanged.
vish288
left a comment
vish288
left a comment
There was a problem hiding this comment.
Review: would approve — self-approval blocked by GitHub
Schema diff, all 83 tools, before (75ec5c0) vs HEAD: 81 byte-identical. The 2 that changed are gitlab_share_project_with_group and gitlab_share_group_with_group, and the change is exactly #79 — access_level went from a bare string to enum: [guest, reporter, developer, maintainer, owner]. The decorator itself leaked nothing into any schema.
Two behaviour changes, both accepted, both belong in the minor bump:
- Truthiness guards →
_params. The oldif search:silently dropped"",0andFalse— a client literally could not set a boolean flag to false._paramsdrops onlyNone. That is a latent bug fixed, not a regression. access_levelis case-sensitive through the Literal. The old code lower-cased it. Since the schema now advertises the exact values, a schema-reading client sends them correctly; a hand-written"Developer"now fails at validation instead of silently working. Correct trade for MCP, and the reason this is 0.11.0 not 0.10.4.
Correctness. _check_write inside the try, so read-only keeps its envelope. merge_mr_sequence keeps a local except GitLabError because its envelope carries merged_so_far — documented, pinned by the existing read-only test, and bugs still fall through to the decorator. servers/gitlab.py 1039 → 510 statements, 100% covered.
Tests. RuntimeError via respx side_effect → is_error True, class name in content, exc_info record on the logger; mocked 404 → is_error False + hint; the bogus-level test now parametrized over both share tools and asserts no HTTP call was made.
Fixes #75. Fixes #76. Fixes #79. 408 passed, 99% src, gate 98 clears, ruff/format/build clean, CI green 3.10–3.13.
1 participant
What
One decorator,
tool_result, applied under@mcp.tool, replaces thetry / _check_write / if-not-None / except Exceptiontemplate in all 83 tools. Bodies are now the call plus_ok/_paginated.GitLabErrorsubclasses (401/403/404, 409/422/429, read-only) keep today's JSON envelope byte-for-byte; the 166 contract tests from test: per-tool wire contract, real-lifespan assembly test and a 98% coverage gate #87 assert the same wire requests and_errkey sets.ToolError, so the client seesisError: truewith the exception type in the message. Two new tests drive both paths throughClient(mcp)(respxRuntimeErrorside effect; mocked 404).write=Truefolds in_check_write; its failure is expected and takes the envelope._params(**kw)dropsNone, so the 90 unrolledif x is not Noneguards become one keyword call per tool.gitlab_merge_mr_sequencekeeps its ownGitLabErrorhandler because its envelope carriesmerged_so_far(pinned by an existing read-only test); bugs still fall through to the decorator.access_levelisLiteral["guest", "reporter", "developer", "maintainer", "owner"]; both hand-rolled checks that returned on the success channel are deleted.functools.wrapspreserves the signature fastmcp introspects: the assembly test still counts 83 tools and every contract row passes unchanged.Numbers
servers/gitlab.pystatementsservers/gitlab.pycoveragesrc/coveragesrc/linesGates:
uv run pytest --cov --cov-fail-under=98,ruff check,ruff format --check,uv buildall green. Converted one tool first and ran the full suite before the rest; failures never rose.Behaviour notes for the release
Unexpected server errors are now reported as MCP tool errors (
isError: true) instead of successful results; expected API errors (401/403/404/409/429, read-only, validation) are unchanged.Minor: the few truthiness guards (
if search:etc.) now forward an explicit empty value rather than silently dropping it.access_levelis case-sensitive through the schema ("Developer"was previously lower-cased).Fixes #75. Fixes #76. Fixes #79.