docs: client log levels, elicitation completion notification; drive json-schema-2020-12-preservation on the alpha.11 referee - #2686
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings (all on the elicitation guide snippets), I also checked the other strand of this PR and found no issues: the new json-schema-2020-12-preservation scenario in test/conformance/src/everythingClient.ts mirrors the existing scenarios' lifecycle/negotiation pattern and passes the observed inputSchema through verbatim; the setLoggingLevel snippet in logging-progress-cancellation.examples.ts is self-asserting with a fail-fast throw; and both expected-failures*.yaml changes are comment-only reconciliation matching the alpha.11 pin bump.
Extended reasoning...
The three confirmed findings all concern the URL-elicitation documentation pattern in examples/guides/servers/elicitation.examples.ts (missing request-timeout handling in the documented parked elicitInput flow, no cancellation path in the client-side wait, and unbounded polling loops in the harness). Since findings are present and the hunt exited on max_rounds, approval is off the table; this note only records what else was examined and ruled out. I read the full diff for the conformance strand: the new client scenario follows the established registerScenario pattern with the same isModernConformanceRun() era branching as tools_call, and echoes focal.inputSchema untouched, which is exactly what a preservation check needs. The logging guide addition's assertion (delivered.join(',') !== 'warning' plus the exact filtered text) correctly verifies the documented filtering claim and throws on mismatch. The expected-failures YAML edits add only reconciliation comments alongside the pin bump from alpha.10 to alpha.11, consistent with the convention stated in those files.
01a76c4 to
99299f2
Compare
claude
Bot
added
the
v2
…ication Add "Let the client set the level" to the logging page (setLoggingLevel, the auto-installed logging/setLevel handler, filtered output) and "Signal that the URL flow finished" to the elicitation page (createElicitationCompletionNotifier + the client-side wait on notifications/elicitation/complete). Both companions run the new rounds and assert the quoted output.
… alpha.11 referee Register the client-side json-schema-2020-12-preservation scenario in the everything client (tools/list, then echo the observed inputSchema through json_schema_echo; modern lifecycle under a 2026-07-28 run), bump @modelcontextprotocol/conformance 0.2.0-alpha.10 -> 0.2.0-alpha.11 and record the reconciliation in both expected-failures baselines. All four legs pass with no new entries.
99299f2 to
49b9cad
Compare
There was a problem hiding this comment.
LGTM — the latest push addresses all of my earlier feedback and this run found no new issues.
What was reviewed:
- Confirmed the elicitation doc/example now raises the elicitInput timeout, forwards
ctx.mcpReq.signal, handles client-side cancellation (abort listener cleans up thefinishedmap), raises the host'stools/calltimeout, and replaces the unbounded polling with a boundedwaitForthat throws — all four prior comments resolved. - Checked doc claims against the SDK:
createElicitationCompletionNotifier(elicitationId, options?)matchespackages/server/src/server/server.ts:1249, and the 2025-11-25-only note matches the changelog's wire-registry removal for 2026-07-28. - Read the logging section and its self-verifying harness (delivered-level assertion), the new
json-schema-2020-12-preservationconformance driver, the alpha.11 pin/lockfile, and both expected-failures reconciliations — consistent, test-harness-only changes with no runtime SDK code touched.
Extended reasoning...
Overview
The PR touches docs and test infrastructure only: two guide pages (docs/servers/elicitation.md, docs/servers/logging-progress-cancellation.md) plus their type-checked .examples.ts companions under examples/guides/servers/, and a conformance-harness update (test/conformance/package.json pin bump to @ modelcontextprotocol/conformance@ 0.2.0-alpha.11, lockfile, a new json-schema-2020-12-preservation scenario in everythingClient.ts, and reconciled expected-failures*.yaml headers). No package under packages/ changes, so nothing ships to consumers beyond documentation.
Security risks
None. There is no runtime code, no auth/crypto/permission path, and no new dependency in the workspace packages — the lockfile change is confined to the conformance referee's own dependency tree (ajv 8.20.0 for the referee only, per the lockfile diff). The doc snippets model good practice (signal forwarding, cleanup on abort) rather than introducing risk.
Level of scrutiny
Moderate scrutiny was appropriate because doc snippets here are load-bearing: they sync into the guide pages and run in CI as self-verifying examples, and the previous two review rounds found real problems in exactly this material (missing timeouts, missing signal forwarding, leak-prone client handler, unbounded polling). I re-read the current diff against each prior finding and confirmed every one is fixed in the pushed commits: { timeout: 10 * 60_000, signal: ctx.mcpReq.signal } on elicitInput, an abort listener that deletes the finished entry and resolves cancelled, a { timeout: 10 * 60_000 } on the host's callTool, and a bounded waitFor (400 × 5 ms, throws a diagnostic) replacing the infinite loops. I also verified the documented API surface exists as described (createElicitationCompletionNotifier signature in server.ts, the 2025-11-25-only availability matching the changelog's wire-registry note, and the setLoggingLevel/logging/setLevel claim backed by the harness assertion that only warning is delivered).
Other factors
The bug hunt ran to dry_streak with zero findings this run. The examples are executed in CI (pnpm docs:examples) and assert the exact quoted output, so the pages' behavioral claims are machine-checked rather than prose-only. The conformance changes follow the convention stated in the YAML headers (bump the pin and reconcile baselines in the same change), the new scenario mirrors the existing scenario structure in everythingClient.ts (including the modern-lifecycle branch), and no baseline entries were added or removed. No outstanding third-party objections exist in the timeline — the only inline comments were my own, now addressed by the new commits — so approval is safe.
1 participant
Documents the two remaining guide gaps — a client raising the log level (
setLoggingLevel) and the URL-elicitation completion notification — and makes the conformance harness drive thejson-schema-2020-12-preservationclient scenario against the0.2.0-alpha.11referee.Motivation and Context
The SEP-1730 documentation check for v2 came back 46/48 after #2679:
client.setLoggingLevel()andnotifications/elicitation/completehad runnable code underexamples/but no how-to prose on the guide site. Separately, the client conformance leg reported one un-baselined failure:json-schema-2020-12-preservationlanded in the referee after our alpha.10 pin and the everything client had no driver for it, so the run hit "Unknown scenario".docs/servers/logging-progress-cancellation.md— "Let the client set the level":setLoggingLevel, the auto-installedlogging/setLevelhandler, filtered output; one-line 2026-07-28 note.docs/servers/elicitation.md— "Signal that the URL flow finished":createElicitationCompletionNotifierserver-side, the client-side wait on the notification; 2025-era-only note.test/conformance/src/everythingClient.ts— registers the preservation scenario (list tools, echo the observedinputSchemaverbatim throughjson_schema_echo; modern lifecycle under a 2026-07-28 run).test/conformance/package.json— referee pin0.2.0-alpha.10→0.2.0-alpha.11; bothexpected-failures*.yamlreconciled (no entries added or removed; the two scenarios alpha.11 added both pass).How Has This Been Tested?
pnpm sync:snippets --check, examples typecheck + lint, both guide companions run and assert the quoted output. Conformance legs as CI runs them:client:all449 passed / 9 baselined,client:2026385 passed / 0 failed,server:all199 passed / 30 baselined,server:2026151 passed / 0 failed. The preservation scenario passes 9/9 on both wires, and the frozen-set runs (conformance sdk --requirements 2025-11-25and2026-07-28) score 18/18 and 32/32 with it reported as added-after-release.Breaking Changes
None.
Types of changes
Checklist
Additional context
Lockfile: besides the pin, alpha.11 declares
ajv ^8.20.0, so pnpm addedajv@8.20.0for the referee and re-resolved the referee's transitive@modelcontextprotocol/sdk@1.29.0snapshot onto it; workspace packages stay on ajv 8.18.0.