Skip to content

feat(mcp): migrate to python-sdk v2 (protocol 2026-07-28) - #1433

Merged
cbcoutinho merged 9 commits into
masterfrom
feat/mcp-sdk-v2
Sep 5, 2026
Merged

feat(mcp): migrate to python-sdk v2 (protocol 2026-07-28)#1433
cbcoutinho merged 9 commits into
masterfrom
feat/mcp-sdk-v2

Conversation

@cbcoutinho

@cbcoutinho cbcoutinho commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Migrates to mcp python-sdk 2.1.1 (protocol 2026-07-28), from 1.29.

Closes the work planned on Deck card 946 (board 46, "MCP 2026-07-28 Spec
Adoption"), which blocks every other card on that board.

Deviation from the card's plan — read this first

The card plans 4 stacked PRs (spike / prep-on-v1 / mechanical bump /
semantics). This is one PR. Phase 1 ("prep on v1") is no longer reachable:
the get_context() replacement needs ServerRequestContext, which does not
exist in 1.x, so there is nothing forward-compatible left to land first. And
splitting the bump from the semantics would make the bottom PR knowingly
broken — without the ToolError conversions, tools silently stop explaining
themselves to the model.

Happy to restructure into a stack if you'd rather review it that way; say so and
I'll split it. The semantic changes are the four bullets under Behavioural
below, each with a comment at the site.

Mechanical (the bulk of the diff)

Change Sites
FastMCPMCPServer, mcp.server.fastmcp*mcp.server.mcpserver* 34
McpError(ErrorData(...))MCPError(code, message) 141
.isError.is_error, .structuredContent, .uriTemplate, .inputSchema ~310
ToolAnnotations(readOnlyHint=…)read_only_hint= ~380
streamablehttp_clientstreamable_http_client + httpx2.AsyncClient 3 files
RequestContextServerRequestContext / ClientRequestContext 2 files

The ToolAnnotations kwargs were not required — camelCase still constructs,
verified at runtime — but renaming them clears ~200 ty warnings, so ty is
usable again. NextcloudFastMCPNextcloudMCPServer, since it was named
after a class that no longer exists.

Behavioural — the actual risk

1. request_ctx and get_context() are both gone. v2 injects Context
into tools and template resources only. That left capability gating in
list_tools() and every static @mcp.resource() (nc://capabilities,
notes://settings, cookbook://version) with no route to a context — and
because both callers fail open by design, this would have silently disabled
capability gating
rather than crashing. New nextcloud_mcp_server/request_context.py
republishes the context from a middleware, which is where v2 puts per-message
interception.

2. Server.request_handlers is gone — the tool-outcome metrics and
client-fleet instrumentation were patching that dict. Both are now a middleware;
instrument_call_tool_outcomes(mcp) keeps its signature.

3. MCPError from a tool is now a JSON-RPC error, not is_error=True.
The client raises it, so the model never sees the message. All ~140 of our
raise sites are "Note 5 not found" / "Nextcloud access not provisioned" —
failures the model should read and react to, which is precisely what ToolError
means in v2. Mapped back at the call_tool boundary rather than changing the
wire contract for 140 messages.

4. UnexpectedToolError withholds the original message entirely (Error executing tool <name>, nothing more). 21 raise ValueError sites in tool bodies
"calendar_name is required when search_all_calendars is False", comment
length limits — were reaching the model as text in 1.x and would now reach it as
nothing. Converted to ToolError. A live integration test caught this one.

Also: Streamable HTTP now caps POST bodies at 4 MiB, answering 413 before
parsing, while WEBDAV_WRITE_MAX_MB defaults to 50 MB — max_request_body_size
is now derived from that same setting so the size refusal stays where it can
explain itself. transport_security moved off the constructor onto
streamable_http_app() (see the note appended to
docs/MCP-1.23-DNS-REBINDING-FIX.md). ctx.elicit() raises
NoBackChannelError at 2026-07-28, so the fallback gained a dedicated branch
and a mcp_elicitation_total{reason="no_back_channel"} label. Deck resource
deprecation notices moved out of ctx.warning() — dropped on 2026-era
connections per SEP-2577 — into descriptions clients actually see.

Test coverage

New:

  • tests/unit/test_request_context_bridge.py — the middleware bridge, driven
    through a real Client(server) (which negotiates 2026-07-28). Calling the
    methods directly would pass while the served path silently failed open.
  • tests/unit/test_tool_errors_stay_tool_results.py — the card's acceptance
    criterion: a Nextcloud 404 and a raised MCPError both land in
    CallToolResult.is_error with a usable message, asserted from the client side.
  • tests/smoke/test_protocol_2026.py — closes a coverage gap this port
    surfaced: every existing fixture drives a hand-rolled ClientSession +
    initialize(), which negotiates a 2025-era version, so nothing exercised
    2026-07-28 over HTTP. Uses Client(url) (mode="auto").

The existing ClientSession fixtures deliberately stay on the legacy path —
that is what real clients still speak, and both eras need to keep working.

No new API surface, so no new contract tests; the existing Pact lanes should be
unaffected and are the check on that.

Verification status — please read

Run locally before I was asked to move validation to CI:

  • ruff, ruff format, ty (201 → 0 diagnostics)
  • pytest -m unit — 3727 passed
  • pytest -m contract — 19 passed
  • pytest -m smoke — passed against a live stack over real Streamable HTTP
  • nc://capabilities static resource confirmed working live in single-user
    BasicAuth — the piece with no route to a Context under v2

Not yet run anywhere: -m "integration and not oauth" got to ~78% with all
failures traced to profiles I hadn't started (ports 8003/8004, keycloak, ldap)
rather than regressions — but I did not finish confirming that, so treat it as
unverified. -m oauth, -m ldap, -m login_flow_ldap, -m keycloak have not
run at all. The three new test files above have never been executed — CI is
their first run.

Auth is the surface I'd least want to take on faith here: elicitation losing its
back-channel, and AuthenticatedUser / auth_context_var still being
SDK-internal (unchanged in v2, but only the OAuth lanes prove it).

The card's open item — now closed

Progressive-consent behaviour under a 2026-07-28 client is documented and
deliberate — verify the degradation empirically, don't assume it.

tests/unit/test_elicitation_no_back_channel.py does this, and passes in CI. It
drives the real present_login_url path behind a Client(server) connection
that negotiates 2026-07-28, and asserts the metric reason is specifically
no_back_channel rather than merely that some fallback fired — reason="error"
would mean we were guessing at the cause. It also pins that registering an
elicitation_callback does not rescue it, since that is the obvious wrong fix.

Confirmed, not assumed: the SDK does raise NoBackChannelError, our handler
catches that specific exception, and the tool degrades instead of failing.
Whether losing the clickable prompt is acceptable is a product call — restoring
an interactive one needs the resolver / InputRequiredResult route, which is
Deck card #526.


This PR was generated with the help of AI, and reviewed by a Human

🤖 Generated with Claude Code

https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd

@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown

Claude finished @cbcoutinho's task in 3m 9s —— View job


Review: migrate to python-sdk v2 (protocol 2026-07-28)

  • Read PR description, comments, and repo CLAUDE.md
  • Inspect diff (git diff origin/master...HEAD, 9 commits)
  • Review behavioral changes: request-context bridge, MCPError/ToolError boundary, elicitation no-back-channel, derived max request body size
  • Review mechanical rename sweep for correctness
  • Check test coverage for the new behavior
  • Post final review feedback

Summary

This is a large, high-risk migration (mcp 1.29 → 2.1.1, protocol 2026-07-28) handled with unusual care. I read the four behavioral changes in detail plus spot-checked the mechanical rename, and I don't have any blocking findings. The four non-blocking review notes referenced in the PR thread (integration/OAuth/LDAP/Keycloak lanes, SonarCloud complexity, the mcp version range, BaseResponse/capability-gating conformance) all appear to have been addressed already, per the author's 2026-09-04/09-05 comments and the current CI state (Pact lane green; full 27-lane matrix reported green three times on a1b6b225).

What I verified

1. errors.py / the call_tool exception boundary — the three-clause except UnexpectedToolErrorexcept ToolErrorexcept MCPError ordering is correct given UnexpectedToolError subclasses ToolError, and the comment explaining why the order is load-bearing is genuinely useful for the next person touching this. friendly_tool_error falling back to _cause_message when there's no HTTP-specific rendering is a sensible layering.

2. request_context.py / the middleware bridge — republishing ServerRequestContext via a contextvar set in NextcloudMCPServer's middleware, for the three callers (list_tools, static @mcp.resource(), enforce_capability) that the SDK gives no Context to, is the right shape for a stopgap the SDK itself removed. current_context() raising LookupError outside a served request (rather than silently returning something bogus) is the correct failure mode, and test_request_context_bridge.py specifically tests it through a real Client(server) dispatch rather than calling the methods directly — which matters here, since a direct call would pass while silently skipping the middleware.

3. _max_request_body_size() (app.py) — the 4/3 base64-inflation factor plus a flat 1 MiB envelope slack, floored at the SDK's 4 MiB default, is exercised well in test_max_request_body_size.py (default 50 MB case, the 0/None/small-value floor case, and a scaling monotonicity case). I checked the arithmetic against the test's own assertions and it holds.

4. Elicitation NoBackChannelError handling_run_elicit's catch order (NoBackChannelError before the generic Exception) attributes the fallback to the right metric label, and test_elicitation_no_back_channel.py proves it against a real Client(server) connection negotiating 2026-07-28 rather than a mock — including the "does registering a callback rescue it" negative case, which is exactly the kind of thing someone would try as a wrong fix later.

5. Mechanical sweep completeness — I grepped the full source and test tree for mcp.server.fastmcp/FastMCP, camelCase ToolAnnotations kwargs (readOnlyHint=, etc.), McpError/ErrorData, and .isError — all zero hits outside history. The rename is clean; nothing was missed beyond what the two straggler commits already fixed.

6. sharing.py / test_sharing_link_expiry.py — the RuntimeError/ValueErrorToolError conversions and the corresponding test updates (pytest.raises(ToolError, ...)) are consistent with the boundary change and correctly explained inline.

Non-blocking observations

  • The mcp[cli] (>=2.1,<3) range (pyproject.toml) is already flagged in the PR thread as a deliberate, unresolved decision for the repo owner — worth an explicit yes/no before merge given this SDK has now broken assumptions twice, but that's already surfaced and isn't something I'd add to.
  • NextcloudMCPServer.__init__ appends _publish_request_context to self.middleware before instrument_call_tool_outcomes(mcp) appends its own middleware in app.py. That ordering happens to be correct (context must be published before anything downstream might want it), but it's an implicit ordering dependency across two files — a one-line comment on instrument_call_tool_outcomes noting it must be registered after the context-publishing middleware (or asserting it defensively) would make that harder to break by accident in a future refactor. Not blocking; today's registration order is correct and tested.

CI status at time of review

The latest push triggered fresh CI (Pact contract tests already green; the full Tests workflow and this review were still queued/in-progress when I looked). Given the author's prior comment that the full 27-lane matrix passed three times on the previous commit (a1b6b225), and this push only added 01fe0bd (a small, self-contained test addition for _max_request_body_size), I'd expect the same result, but I did not wait for it to complete.

Conclusion

No blocking issues found. The behavioral risk surface called out in the PR description is each backed by a targeted test that exercises the real SDK dispatch path rather than mocking around it, which is the right way to de-risk an SDK major-version migration like this.

@cbcoutinho

Copy link
Copy Markdown
Owner Author

Status update — the "never executed" caveat above is now stale

When this PR was opened, three new test files had never run anywhere. They have
now run in CI, and one of them found a real bug in itself:

  • tests/unit/test_request_context_bridge.py
  • tests/unit/test_tool_errors_stay_tool_results.py
  • tests/unit/test_elicitation_no_back_channel.pyfailed first, fixed in
    3308a49. Its tool declared ctx with no type annotation, so the SDK treated
    it as a required tool argument instead of injecting it
    (ctx Field required [type=missing]). Nothing else in the unit suite was
    affected.
  • tests/smoke/test_protocol_2026.py runs in the integration lanes, still going.

The card's last open item is now closed for real

Progressive-consent behaviour under a 2026-07-28 client is documented and
deliberate — verify the degradation empirically, don't assume it.

test_elicitation_no_back_channel.py now passes in CI, and it asserts the metric
label is specifically reason="no_back_channel" rather than merely that some
fallback happened. That distinction is the whole point: reason="error" would
mean we were guessing at the cause. So this is now confirmed on a real
2026-07-28 connection — the SDK does raise NoBackChannelError, our handler
catches that specific exception, and the tool degrades instead of failing.

It also pins that setting an elicitation_callback does not rescue it, since
that is the obvious wrong fix.

I also rewrote one assertion in that file that was vacuous: it checked that a
login URL appeared in the tool result, but the test's own tool had put it there.
Getting the URL in front of the user is the caller's contract —
server/auth_tools.py builds the URL-bearing message on every non-accepted
branch — not something this helper guarantees, so the test now scopes itself to
the degradation and says so.

SonarCloud

Gate OK on all five conditions (new maintainability rating A). It reports 5
CRITICAL python:S3776 cognitive-complexity findings on
configure_{notes,deck,cookbook,mail,semantic}_tools — these are pre-existing:
those are 400-line registration functions on master too, and the rename sweep
touched lines inside them, which re-attributes them as new code. Not refactoring
them here; five functions at complexity 45–153 is a large unrelated change that
would bury a 106-file migration. Happy to open a follow-up card if you want them
split up.

CI so far

Pass: linting, SonarCloud, unit-test, both Pact lanes, package-smoke
(ubuntu + windows). The 18 integration lanes are re-running after the fixup
push — on the previous run keycloak/nc32, keycloak/nc33, ldap/nc32,
ldap/nc34 and docling had all passed, which covers the OAuth/external-IdP and
the GH #980 DAV-principal regression guards.

cbcoutinho added a commit that referenced this pull request Sep 4, 2026
Review catch on #1433. My audit of "exceptions whose text stops reaching the
model" grepped for `raise ValueError` and so missed this one: `_build_link_response`
raises `RuntimeError` when an OCS public-link payload carries no url, and
`nc_share_create_public_link` calls it with no enclosing handler.
`friendly_tool_error` only rewrites httpx errors, so under 2.x the model would be
told the call failed and nothing else.

Same class as the 21 ValueError sites, same fix. Swept the tool modules for every
other non-httpx raise: only `semantic.py`'s TypeError, which is caught by the
`except Exception` that re-raises as MCPError, so its message survives.

Also corrects two docstrings this PR had already invalidated (`Raises: ValueError`
on the expiry helper and its caller), and a comment in semantic.py that still
described v1 delivery — an MCPError from a tool is now mapped back to ToolError
at our boundary rather than shipping as a protocol error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
cbcoutinho added a commit that referenced this pull request Sep 4, 2026
Review catch on #1433. My sed matched `.isError` as attribute access, so the
same names spelled as *strings* inside getattr/hasattr were left behind — and
both degrade quietly rather than erroring:

- test_mail_greenmail.py: `getattr(result, "isError", False)` now always returns
  the default, so _tool_payload stops raising on a failed tool and instead
  json.loads() the error text. All 32 call sites in that file lose the loud
  AssertionError its docstring promises; a CSRF rejection — the thing the suite
  exists to surface (#965) — would have shown up as an opaque JSONDecodeError.
- test_error_propagation.py (x3): `hasattr(response, "structuredContent")` is
  now always False, so the structured-content branch is dead and the three tests
  silently fall through to the is_error assertion, dropping the
  "structured content carries success=False" coverage they were written for.

Neither would have failed CI: the mail lane needs the mail profile, and the
error_propagation tests still pass down the else path.

The `"mimeType"` keys in server/notes.py are deliberately left camelCase — they
are dict keys in hand-built resource payloads, i.e. wire format, which v2 did not
rename.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
cbcoutinho added a commit that referenced this pull request Sep 4, 2026
Review suggestion on #1433. The order depends on the SDK's exception hierarchy,
which a reader would otherwise have to go check: UnexpectedToolError subclasses
ToolError, so the specific clause must come first or every crash takes the
"message is already the author's" path and ships the SDK's contentless text.
MCPError is a separate tree (Exception directly), so its position is free.

Verified against the installed SDK rather than assumed:
UnexpectedToolError -> ToolError -> MCPServerError -> Exception, and
issubclass(MCPError, ToolError) is False.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
cbcoutinho added a commit that referenced this pull request Sep 4, 2026
Review suggestion on #1433: _max_request_body_size() is pure arithmetic gating
user-visible behaviour, and nothing covered it. Get the base64 factor wrong and
a nc_webdav_write_file call under the advertised 50 MB starts failing at the
transport with an opaque 413 instead of that tool's explanatory ToolError —
silently, since no test would notice.

Covers the three cases that matter: the 50 MB default leaving room for the
base64 wire form, a small-or-unset setting never tightening below the SDK's
4 MiB floor (0/None mean "no app-level cap", not "no bytes"), and the limit
actually scaling with the setting.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
@cbcoutinho

Copy link
Copy Markdown
Owner Author

Answering the four non-blocking notes — no code changes needed for any of them, so this PR stays at a1b6b225.

1. Integration/OAuth/LDAP/Keycloak lanes — now green, all of them. This was the right thing to hold on; it was the surface I flagged as least verified. Final state on a1b6b225: 27 SUCCESS, 1 skipped (can-i-deploy, shadow mode per ADR-029), MERGEABLE / CLEAN. That includes all six deployment lanes × nc32/33/34 — single-user, multi-user-basic, login-flow, login-flow-ldap, ldap, keycloak — plus both Pact lanes and package-smoke on ubuntu + windows.

Worth recording that holding on those lanes found a real regression: login-flow ×3 and single-user ×2 failed with a contentless Error executing tool <name>. Two were scope denials — the model was told a call was refused but not that it needed notes.write. Root cause was wider than the raise ValueError sweep in the original commit: ScopeAuthorizationError subclasses Exception (it is also raised on HTTP routes, where ToolError would be the wrong type), and client/talk.py raises plain ValueError. Fixed once at the call_tool boundary in 95cface rather than by converting exception types, since that list is open-ended.

2. SonarCloud complexity — addressed, not deferred. #1437 is stacked on this PR and clears all five: SonarCloud now reports "No issues found", with new_duplicated_lines_density improving 0.8% → 0.6%. Four of the five were measuring the registration idiom rather than the code (no individual tool in notes/deck/mail exceeds McCabe 8); semantic.py needed real decomposition of an 850-line function. Tracked on Deck card 1203.

3. mcp[cli] (>=2.1,<3) — flagged for an explicit owner decision, deliberately not resolved by me. You are right that it deserves a conscious "yes" rather than a default, and that is the repo owner's call, not mine. The argument for the range as-is: uv.lock pins 2.1.1, so reproducibility is unaffected today and nothing moves without a deliberate uv lock --upgrade; the range matches how Renovate manages this repo's other deps. The argument against, which I think is the stronger one and which I raised before you did: this SDK has now broken this project's assumptions twice (1.23's DNS-rebinding auto-enable, and this entire migration), so 2.2 arriving unremarked is a real risk. I have left it open in the PR body rather than narrowing it unilaterally.

4. Agreed — no BaseResponse, capability-gating or _make_request violations; that part of the diff is pure identifier renaming.

One correction to the review, since it affects how much weight note 1 carries: the summary says several lanes "hadn't run at PR-open time" and cites a prior partial run. That was accurate when written. As of a1b6b225 the full matrix has completed green three times, and the two intermediate red rounds were the exception-message regression above plus a stale pytest.raises(RuntimeError) assertion in tests/unit/server/test_sharing_link_expiry.py that I missed when changing _build_link_response.

ruff, ruff format and ty clean (ty: 201 → 0 diagnostics). Unit, smoke and both contract tiers green in CI.

cbcoutinho and others added 9 commits September 5, 2026 11:24
Bumps `mcp[cli]` from 1.29 to >=2.1,<3. The rename sweep is the bulk of the
diff; the behaviour changes are the risk, and are called out individually
below.

Mechanical:
- `FastMCP` -> `MCPServer`, `mcp.server.fastmcp*` -> `mcp.server.mcpserver*`
- `McpError(ErrorData(...))` -> `MCPError(code, message)` across 141 sites
- camelCase -> snake_case attribute access (`.isError`, `.structuredContent`,
  `.uriTemplate`, `.inputSchema`) and `ToolAnnotations` kwargs. The kwargs still
  construct either way; renaming them clears ~200 `ty` warnings.
- `streamablehttp_client` -> `streamable_http_client` + an `httpx2.AsyncClient`
- `mcp.shared.context.RequestContext` -> `ServerRequestContext` /
  `ClientRequestContext`
- `NextcloudFastMCP` -> `NextcloudMCPServer`: it was named after a class that
  no longer exists

Behavioural:
- `request_ctx` and `MCPServer.get_context()` are both gone, and v2 injects
  `Context` only into tools and *template* resources. That left capability
  gating in `list_tools()` and every static `@mcp.resource()` with no context —
  failing open silently rather than crashing. New `request_context.py`
  republishes it from a middleware.
- `Server.request_handlers` is gone; the tool-outcome metrics and client-fleet
  instrumentation were patching that dict. Both are now a middleware.
  `instrument_call_tool_outcomes(mcp)` keeps its signature.
- `MCPError` raised in a tool is a top-level JSON-RPC error in v2, not
  `is_error=True`. All ~140 of our raise sites are failures the model should
  read and react to, which is what `ToolError` means in v2 — mapped back at the
  `call_tool` boundary rather than changing the wire contract.
- `UnexpectedToolError` withholds the original message entirely. 21
  `raise ValueError` sites in tool bodies ("calendar_name is required...",
  comment length limits) were reaching the model as text and would now reach it
  as nothing; converted to `ToolError`.
- Streamable HTTP now caps POST bodies at 4 MiB, answering 413 before parsing.
  `WEBDAV_WRITE_MAX_MB` defaults to 50 MB, so `max_request_body_size` is derived
  from it.
- `transport_security` moved off the constructor onto `streamable_http_app()`.
- `ctx.elicit()` raises `NoBackChannelError` at 2026-07-28; the fallback now has
  a dedicated branch and `mcp_elicitation_total{reason="no_back_channel"}`.
- Deck resource deprecation notices moved from `ctx.warning()` (dropped on
  2026-era connections per SEP-2577) into descriptions clients actually see.

Tests: `Client(server)` negotiates 2026-07-28, so the new bridge and
tool-error tests pin the modern era; the existing `ClientSession` fixtures stay
on the legacy path, which is what real clients still speak.

Refs card 946 (Deck board 46).

BREAKING CHANGE: requires mcp>=2.1,<3 (protocol 2026-07-28). Server-initiated
elicitation no longer reaches 2026-era clients — progressive consent degrades to
message_only, with the login URL carried in the returned message. Deployments
pinning mcp<2 must stay on the previous release.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
…nnection

Deck card 946 asks for this explicitly: "verify the degradation empirically,
don't assume it." Until now the NoBackChannelError branch was reasoned about,
not exercised.

Runs the real progressive-consent path behind a Client(server) connection, which
negotiates 2026-07-28 — so it proves the SDK actually refuses the elicitation on
this era, rather than proving our except clause works. Asserts the login URL
survives into the tool result, and that the no_back_channel metric label fires,
since that counter is the only production signal that interactive consent has
stopped happening.

Also pins that registering an elicitation_callback does not rescue it: no
request ever reaches the client, so the obvious wrong fix is dead code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
The two places that get no Context — static @mcp.resource() functions and
list_tools() — both fail open rather than raising, so the wrong instinct here is
silent. Names current_context() as the narrow escape hatch, and says plainly
that get_context() and request_ctx are gone so nobody reaches for them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
Review catch on #1433. My audit of "exceptions whose text stops reaching the
model" grepped for `raise ValueError` and so missed this one: `_build_link_response`
raises `RuntimeError` when an OCS public-link payload carries no url, and
`nc_share_create_public_link` calls it with no enclosing handler.
`friendly_tool_error` only rewrites httpx errors, so under 2.x the model would be
told the call failed and nothing else.

Same class as the 21 ValueError sites, same fix. Swept the tool modules for every
other non-httpx raise: only `semantic.py`'s TypeError, which is caught by the
`except Exception` that re-raises as MCPError, so its message survives.

Also corrects two docstrings this PR had already invalidated (`Raises: ValueError`
on the expiry helper and its caller), and a comment in semantic.py that still
described v1 delivery — an MCPError from a tool is now mapped back to ToolError
at our boundary rather than shipping as a protocol error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
CI caught this: four login-flow integration tests failed with "Error executing
tool nc_notes_create_note" and nothing else. Two of them were scope denials —
the model was told the call was refused but not that it needed notes.write.

Root cause is broader than the ValueError sweep in the parent commit.
ScopeAuthorizationError and its subclasses derive from Exception, not ToolError,
because they are also raised on HTTP routes where ToolError would be the wrong
type. mcp 2.x turns any such exception into UnexpectedToolError and replaces its
message with a bare "Error executing tool <name>", so the reason never reaches
the model. friendly_tool_error only rewrites httpx errors and returned None for
everything else, which re-raised the message-less form.

Fixed once at the boundary that already exists for this rather than by
converting exception types one at a time — that list is open-ended, and the next
`raise` in a tool body would reintroduce it. UnexpectedToolError (the crash
case) now always carries a message; a plain ToolError is left alone because its
text is already the tool author's.

Pinned at the unit tier so this fails in seconds rather than in a Playwright
lane, including the empty-message case where str(exc) is "" and only the
exception type identifies the failure.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
…Error

Follows the exception-type change in the previous commit. I missed this test
when making it — I grepped tests/unit/test_sharing* and tests/client/, but it
lives in tests/unit/server/. Records *why* ToolError specifically, so the next
person does not swap it back for a bare exception.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
Review catch on #1433. My sed matched `.isError` as attribute access, so the
same names spelled as *strings* inside getattr/hasattr were left behind — and
both degrade quietly rather than erroring:

- test_mail_greenmail.py: `getattr(result, "isError", False)` now always returns
  the default, so _tool_payload stops raising on a failed tool and instead
  json.loads() the error text. All 32 call sites in that file lose the loud
  AssertionError its docstring promises; a CSRF rejection — the thing the suite
  exists to surface (#965) — would have shown up as an opaque JSONDecodeError.
- test_error_propagation.py (x3): `hasattr(response, "structuredContent")` is
  now always False, so the structured-content branch is dead and the three tests
  silently fall through to the is_error assertion, dropping the
  "structured content carries success=False" coverage they were written for.

Neither would have failed CI: the mail lane needs the mail profile, and the
error_propagation tests still pass down the else path.

The `"mimeType"` keys in server/notes.py are deliberately left camelCase — they
are dict keys in hand-built resource payloads, i.e. wire format, which v2 did not
rename.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
Review suggestion on #1433. The order depends on the SDK's exception hierarchy,
which a reader would otherwise have to go check: UnexpectedToolError subclasses
ToolError, so the specific clause must come first or every crash takes the
"message is already the author's" path and ships the SDK's contentless text.
MCPError is a separate tree (Exception directly), so its position is free.

Verified against the installed SDK rather than assumed:
UnexpectedToolError -> ToolError -> MCPServerError -> Exception, and
issubclass(MCPError, ToolError) is False.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
Review suggestion on #1433: _max_request_body_size() is pure arithmetic gating
user-visible behaviour, and nothing covered it. Get the base64 factor wrong and
a nc_webdav_write_file call under the advertised 50 MB starts failing at the
transport with an opaque 413 instead of that tool's explanatory ToolError —
silently, since no test would notice.

Covers the three cases that matter: the 50 MB default leaving room for the
base64 wire form, a small-or-unset setting never tightening below the SDK's
4 MiB floor (0/None mean "no app-level cap", not "no bytes"), and the limit
actually scaling with the setting.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
cbcoutinho added a commit that referenced this pull request Sep 5, 2026
SonarCloud flagged configure_{cookbook,semantic,notes,deck,mail}_tools at
cognitive complexity 153/80/76/68/45 against the 15 allowed. Pre-existing debt,
surfaced by #1433 only because its rename sweep touched lines inside them.

Two different causes, two different fixes.

**Four are the registration idiom, not the code.** Every tool is a nested
closure, and S3776 charges a nesting penalty per level, so the outer function
accumulates the complexity of every tool it contains even though no individual
tool in notes/deck/mail exceeds McCabe 8. Lifting the 71 tools to module level
and registering them with `mcp.tool(...)(fn)` leaves behaviour identical --
@mcp.tool() is simply the outermost decorator, applied last either way. Resource
functions stay nested: they call current_context(mcp) and genuinely close over
it.

**semantic.py is real complexity**, not an artifact: nc_semantic_search was an
850-line function, so extraction alone would have relocated the finding rather
than fixed it. Pulled out five cohesive pieces it was carrying -- candidate
retrieval, result mapping, context expansion, the date-bound guards, and the
rerank pass -- each named for what it does and documented with why it moved.

Cookbook's two outliers (create/update_recipe, 16 and 20 on their own) needed
their own pass: the ten sequential `if field:` assignments became one
field-mapping table, and the HTTP-status if/elif chains a status->message dict.
The create/update guard difference is preserved and now stated explicitly --
create drops falsy values, update drops only None so an empty string still
clears a field.

Result: max complexity across the five files is now 13, from 153.

The one behavioural risk is that module-level functions are shared across
MCPServer instances while stamp_required_capability mutates them. Safe because
the stamp is idempotent and deterministic per app, and now pinned by a test
rather than left to that argument.

Refs card 1203.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
@sonarqubecloud

sonarqubecloud Bot commented Sep 5, 2026

Copy link
Copy Markdown

@cbcoutinho
cbcoutinho merged commit 493935b into master Sep 5, 2026
29 checks passed
pi0n00r pushed a commit to pi0n00r/nextcloud-mcp-server that referenced this pull request Sep 5, 2026
The claude-review action's allowlist had no `Task`, so on a large diff the
reviewer fails silently. On cbcoutinho#1437 (~3800 lines across 6 files) it announced
"dispatched parallel sub-reviews of the three logical chunks", had all 12 of
those calls denied, then exited with is_error: false and 17 turns — leaving a
sticky comment that still read "Review in progress" with an unticked checklist
and no findings.

The check went SUCCESS regardless, because it reports that the action ran, not
that a review happened. So the PR presented as reviewed-and-clean when nothing
had been reviewed. Two runs failed identically, ~2 minutes apart; it is not a
flake. Smaller diffs are unaffected — cbcoutinho#1433 reviewed fine because it never
chose to fan out.

Two changes:

- Allow `Task`, so the fan-out the reviewer already attempts can actually run.
- Tell it, in the prompt, to say so in the tracking comment when it cannot
  finish. Its system prompt already asks it to report missing permissions and it
  did not, and a silent stall is the expensive failure here: a green check over a
  half-written comment is worse than a red one.

Cost note recorded inline: fan-out means several sub-agents per review, and a
single non-fanned-out run on that PR cost ~$1. Drop `Task` again if that proves
too expensive, accepting that large PRs then need splitting to be reviewable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant