cli: reconcile --print-handoff deep-link with #1576 canonical format (#1666)

[cliffhall]

Closes #1666

Stacked on #1670 (#1576) — based on v2/1576-deeplink-connect so the diff shows only the #1666 changes. Superseded by the Wave 4 rollup; this PR is for focused review.

Summary

#1575 shipped --print-handoff with an interim deep-link shape (hardcoded transport: "http", marked TODO(#1576)). Now that #1576 owns the canonical ?serverUrl&transport&autoConnect format, --print-handoff emits exactly it:

• New exported deepLinkTransport(serverUrl, transport) derives http|sse from the resolved server: explicit --transport wins, else auto-detect from the URL path (/sse → sse), else default http. Unlike resolveServerConfig it defaults rather than throwing on an ambiguous path — the handoff is best-effort and the web parseDeepLink also defaults an unknown transport to http.
• buildHandoff now takes the transport, emits the normalized serverUrl + derived transport in the query, and keeps autoConnect=<token> — which web: deep-link auto-connect (?serverUrl&transport&autoConnect) + machine-readable connection status #1576 adopted as the canonical CSRF gate, so the interim guess was already right.
• Removed the TODO(#1576) marker; updated the doc comment + CLI README.

Tests

• Handoff emits transport=http for a /mcp server, transport=sse for /sse, and honors explicit --transport sse.
• deepLinkTransport unit tests cover explicit / auto-detect / default / stdio / unparseable-URL branches.

CLI coverage gate green (npm run test:coverage, EXIT 0; cli.ts 100/92.8/100/100).

Part of the #1579 decomposition (Wave 4). Per AGENTS.md the Closes line won't auto-fire on v2/main; the issue is closed and its board card moved on the Wave 4 rollup merge.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 43s —— View job

---

Review — --print-handoff deep-link reconciliation (#1666)

• Read PR diff vs origin/v2/1576-deeplink-connect
• Examine cli.ts context + web deepLink.ts for consistency
• Verify transport derivation & normalization logic
• Assess test coverage
• Post review

Verdict: LGTM. This is a clean, well-scoped reconciliation. The emitted deep-link now matches exactly what the web parseDeepLink (clients/web/src/utils/deepLink.ts) consumes, and I verified the two ends agree on every axis that matters.

What I verified against parseDeepLink

• Transport default parity. deepLinkTransport defaults to http on an ambiguous/unparseable path; the web parser does the same (params.get("transport") ?? "http", then ALLOWED_TRANSPORTS.has(...) ? … : "http" at deepLink.ts:157-160). The doc comment's claim that both default rather than throw is accurate.
• serverUrl shape. The query serializes normalizeServerUrl(serverUrl) (= new URL().href), which is the same canonical form the web validateServerUrl returns (url.href) and the same shape the OAuth store keys under — so --use-stored-auth lookups stay consistent. Good.
• autoConnect gate. autoConnect=<MCP_INSPECTOR_API_TOKEN> is exactly the CSRF token the web parser requires (autoConnect !== authToken → reject). Correctly omitted when the token is unset, with the note field flagging it — matches the retained "unset" test.
• sse auto-detect. /sse path → sse, explicit --transport wins over the path. Matches resolveServerConfigs' detection intent and the new tests confirm both branches.

Minor nits (non-blocking)

1. Broken JSDoc {@link}. Both cli.ts:544 and cli.ts:571 reference {@link resolveServerConfig} (singular), but no such symbol exists — the real resolver is resolveServerConfigs (plural, in core/mcp/node/config.ts) and it isn't imported into cli.ts, so the link resolves to nothing. Worth fixing the name so the cross-reference actually links. Fix this →

2. normalizeServerUrl(serverUrl) computed twice in buildHandoff (lines 592 and 597). Harmless (idempotent, cheap), but you could hoist it to a const normalizedUrl and reuse it for both the query param and the serverUrl field for a touch less repetition.

3. Optional test gap. deepLinkTransport is exercised as a pure function and end-to-end via --print-handoff, but a .sse-ending path that is not exactly /sse (e.g. /foo.sse) would match endsWith("/sse")… actually it wouldn't (no leading /), so that's fine. No action needed — just confirming the endsWith("/sse") boundary is what you intend (a path segment, not a suffix). It matches resolveServerConfigs, so consistent.

Nice touches

• The catch comment explaining why an unparseable URL defaults to http (web rejects non-http(s) serverUrl anyway) is exactly the right level of justification.
• Removing the interim TODO(#1576) and rewriting the doc comment + README to describe the now-canonical shape keeps the docs honest.
• Test coverage is thorough: explicit-wins, auto-detect, default, stdio, and unparseable branches for the pure function, plus http/sse/explicit end-to-end assertions.

None of the nits block merge — they're doc/style polish.
· v2/1666-print-handoff

[cliffhall]

Thanks — addressed both nits in ac490007: corrected the JSDoc to name the real resolveServerConfigs (core/mcp/node/config.ts) in plain text (it isn't imported here, so {@link} wouldn't resolve), and hoisted the doubly-computed normalizeServerUrl(serverUrl) to a single normalizedUrl. #3 (the /sse boundary) was confirmed intentional — matches resolveServerConfigs.

@claude review

[claude]

Claude finished @cliffhall's task in 34s —— View job

---

Follow-up review — nits addressed in ac490007

• Review JSDoc resolveServerConfigs correction
• Review normalizedUrl hoist
• Verify tests/behavior unchanged
• Post review

Verdict: LGTM. Both nits from the prior review are cleanly resolved, and neither changes runtime behavior.

1. JSDoc reference — fixed. {@link resolveServerConfig} (a non-existent symbol) is now the plain-text `resolveServerConfigs` (core/mcp/node/config.ts). Plain text over {@link} is the right call here — since resolveServerConfigs isn't imported into cli.ts, a {@link} would render as dead text anyway. The surviving {@link parseDeepLink} on the next line is a genuine cross-file reference to the web parser and reads fine.

2. normalizeServerUrl hoist — fixed. const normalizedUrl = normalizeServerUrl(serverUrl) is now computed once and reused for both the query serverUrl param and the returned serverUrl field. Behavior is identical (normalizeServerUrl is a pure new URL().href), just less repetition.

One thing I double-checked: deepLinkTransport(serverUrl, transport) on the next line is still passed the raw serverUrl, not normalizedUrl — which is correct, since it parses the URL itself for /sse auto-detection and normalization would be a no-op for path inspection. No accidental coupling introduced.

3. /sse boundary — confirmed intentional (matches resolveServerConfigs), no change needed, as you noted.

No new tests were required since neither change alters observable behavior, and the existing http/sse/explicit + pure-function branch coverage still exercises the touched lines. Nothing blocking — ship it.
· v2/1666-print-handoff

[cliffhall]

Superseded by the Wave 4 rollup #1676, which contains this change (reviewed to LGTM here) merged base→tip. Closing in favor of the rollup.