fix(fx-core): make MCP OAuth metadata discovery robust and failures visible

docs/02-architecture/adr/ADR-0020-mcp-server-url-validity.md

@@ -0,0 +1,166 @@
+# ADR-0020 — MCP server URL validity: when to check, and whether to block
+
+- **Status:** Accepted
+- **Date:** 2026-07-27
+- **Source:** [`mcp-remote-servers.md` §3](../external-dependencies/mcp-remote-servers.md#3-open-questions)
+
+## Context
+
+Forced by an external fact. When a user supplies a remote MCP server URL, a
+common mistake is to paste the host or a parent path instead of the endpoint
+— `https://mcp.notion.com/` instead of `https://mcp.notion.com/mcp`. The
+toolkit currently accepts such a URL, scaffolds a project around it, and the
+mistake surfaces only at provision time or not at all.
+
+[`mcp-remote-servers.md` §1.3–§1.4](../external-dependencies/mcp-remote-servers.md#13-measured-behavior--documented-endpoint-urls)
+records what nine real endpoints and fourteen truncated forms return. Four
+measured facts drive this decision:
+
+- No valid endpoint returned `404`. A `404` from an `initialize` `POST` is
+  therefore a zero-false-positive negative signal on the sample measured.
+- It is not a complete signal: it caught six of the fourteen truncated URLs.
+  Widening to the other negative shapes measured — `403`, `405`, and `2xx`
+  without a JSON-RPC envelope — raises that to ten of fourteen, still without
+  misclassifying any of the nine valid endpoints.
+- Those wider shapes are weaker evidence. Both measured `403`s came from a WAF
+  intercepting ahead of the application, and a WAF fronting a *valid* endpoint
+  could reject the probe the same way. `405` came from a valid endpoint too,
+  when addressed with the wrong method.
+- `5xx` and transport failures mean the server is unreachable, not that the
+  URL is wrong, and must be distinguished from both of the above.
+
+One valid server that *can* answer `404` is absent from the sample rather than
+nonexistent. The transport spec's backwards-compatibility rule has a client
+read `404` or `405` from the `initialize` `POST` as "this may be a deprecated
+HTTP+SSE server" and disambiguate with a `GET` that opens an event stream.
+No such server was reachable to measure — every `/sse` path probed is either a
+streamable-HTTP endpoint or authorization-gated ahead of transport dispatch —
+but the requirement is normative, so the probe performs that `GET` before
+forming a `notEndpoint` verdict (fact page §1.2). The zero-false-positive
+property is preserved by construction rather than by the sample.
+
+A fifth fact constrains how success is recognized: `https://learn.microsoft.com/api/mcp`
+requires no authorization at all and answers `initialize` with `200`, while
+`https://substrate-sdf.office.com/` answers the same request with `200` and
+HTML. The status is not the signal; the JSON-RPC envelope in the body is.
+Recognizing it also means a confirmed endpoint stops being indistinguishable
+from a probe that learned nothing.
+
+The scaffold flows also differ in what backstop they already have. The paths
+that fetch tools detect a wrong URL independently, because a wrong URL yields
+zero tools. The dynamic-tool-discovery path performs no tool fetch by design
+and has no such backstop, which is why the `404` signal currently surfaces
+there as a post-scaffold warning.
+
+The question this ADR settles is *where* the check belongs and *how hard* it
+should push back — not whether `404` is meaningful, which §1 establishes.
+
+## Options considered
+
+- **A — Post-scaffold warning only (current behavior).** The probe already
+  runs during scaffolding; a `404` becomes a warning in the scaffolding
+  summary. Cheapest, and cannot break input flows. But it tells the user after
+  the project exists, and the remedy is to recreate it.
+
+- **B — Non-blocking hint at input time.** Move the signal to the MCP server
+  URL question so the user sees it while the field is still editable, but
+  allow them to proceed. Requires the URL question to probe on accept, which
+  it does not currently do on the VS Code path. Costs a network round-trip in
+  the question walk and must degrade silently when offline.
+
+- **C — Blocking validation at input time.** Same probe, but reject the URL.
+  The measured precision supports it, but the sample is nine endpoints, a
+  gateway mid-deploy can legitimately `404`, and a hard block on a network
+  probe makes the create flow fail closed when the network does.
+
+- **D — Positive validation instead of negative.** Require evidence the URL
+  *is* an MCP endpoint — a successful `initialize`, or a `401` challenge —
+  rather than looking for evidence it is not. Strictly stronger: it would also
+  catch the `200`-returning wrong URLs that `404` misses. But it inverts the
+  failure mode, rejecting valid endpoints whenever the probe cannot complete,
+  and the path-prefix `401` case (§1.4) would still pass.
+
+- **E — B plus telemetry, then reconsider C.** Ship the hint, measure how
+  often it fires and how often users correct the URL afterwards, and use that
+  to decide whether blocking is justified. Defers the precision question to
+  data instead of a nine-endpoint sample.
+
+- **F — C for the certain signal, A for the weaker ones.** Block at input time
+  on `404` alone; let the other negative shapes surface as a post-scaffold
+  warning. Two tiers to implement and reason about, but the strength of the
+  pushback then matches the strength of the evidence.
+
+## Decision
+
+**F — block at input time on `404`; warn after scaffolding on the weaker
+negative shapes.**
+
+The URL question rejects a value if, and only if, an unauthenticated
+`initialize` `POST` to it completes and the server answers `404`. Every other
+outcome accepts: a `401`, a successful `initialize`, `403`, `405`, any other
+status, a `5xx`, a timeout, a DNS or TLS failure. The user must have received a
+positive statement from the server that nothing is routed at that path before
+they are stopped.
+
+The probe classifies the URL into three states rather than reporting only
+whether authorization is required:
+
+| State | Produced by | Effect |
+|---|---|---|
+| `confirmed` | `401`, or `2xx` carrying a JSON-RPC envelope | none |
+| `notEndpoint` | `403`, `404`, `405`, or `2xx` without a JSON-RPC envelope, and no HTTP+SSE stream at the URL | `404` blocks at input; the rest warn after scaffolding |
+| `undetermined` | `5xx`, other statuses, timeouts, transport failures | none |
+
+Blocking on `404` is chosen over the non-blocking hint (B) because the measured
+precision is 100% across nine endpoints and the failure it prevents is
+expensive: the project is scaffolded around the wrong URL, authorization
+discovery then resolves that *host's* authorization server and writes
+plausible-but-wrong endpoints, and on the dynamic-tool-discovery path nothing
+else in the flow disagrees. A warning the user can click past does not prevent
+that, and the remedy after the fact is to recreate the project.
+
+The weaker shapes are not promoted to blocking because both measured `403`s
+came from a WAF rather than the application. Wrongly blocking a legitimate URL
+is worse than missing a wrong one — the user has no way past a blocking
+validator, whereas a warning still leaves a usable project.
+
+It is chosen over pure positive validation (D) for the same reason in stronger
+form: D rejects valid endpoints whenever the probe cannot complete, which
+breaks offline and restricted-network scaffolding. The rule adopted here fails
+open on every ambiguous outcome by construction — `undetermined` never
+surfaces at all.
+
+The negative statuses are enumerated (`403`, `404`, `405`) rather than
+generalized to "any 4xx", because statuses such as `429` and `408` are
+transient and would slander a URL that is in fact correct. `400` is excluded
+for a stronger reason: the transport spec *requires* a server to answer `400`
+when it rejects the `MCP-Protocol-Version`, so a valid endpoint can produce
+one.
+
+The check is not a substitute for the tool-fetch signal on the paths that have
+one, and does not claim completeness — §1.4 of the fact page documents a
+truncated URL that answers `401` and therefore passes.
+
+## Consequences
+
+- New constraints §2.8 – §2.13 on
+  [`mcp-remote-servers.md`](../external-dependencies/mcp-remote-servers.md#2-constraints-derived-from-these-facts).
+- Scaffolding gains a network round-trip in the question walk on every
+  platform. Previously the VS Code path performed no probe at this point.
+- A URL that is going to be rejected costs a second round-trip, the HTTP+SSE
+  `GET`. Only the `notEndpoint` path pays it; a confirmed or undetermined URL
+  never does.
+- A valid MCP server that is mid-deploy behind a gateway returning `404` will
+  block the create flow with no override. This is the accepted cost of the
+  decision; the residual risk is bounded because a deploying gateway more
+  commonly returns `5xx`, which accepts.
+- The rejection message must name the measured cause and the likely fix, since
+  the user has no way past it.
+- The post-scaffold warning on the dynamic-tool-discovery path now fires for
+  every `notEndpoint` shape, not only `404`. Its wording can no longer name a
+  status code.
+- The `404` half of that warning becomes unreachable through the interactive
+  create flow. It is retained because that path is also reached through entry
+  points that do not walk the URL question.
+- `confirmed` is recorded but not rendered anywhere. Whether to show positive
+  feedback for it is left open on the fact page §3.

docs/02-architecture/adr/README.md

@@ -38,6 +38,7 @@ decision. The format for a new ADR is defined inline below under
 | ADR-0017 | [Named pipeline + step / actionTemplate whitelist](ADR-0017-named-pipeline-step-whitelist.md) | Accepted | [`scaffolding.create.proposal.md` §14](../scaffolding.create.proposal.md#14-adrs-this-proposal-will-be-decomposed-into) |
 | ADR-0018 | [`ScaffoldRuntime` + T1/T2/T3 test pyramid + design-first `descriptor.spec` gate (application of ADR-0013, not a competing tiering)](ADR-0018-scaffold-runtime-test-pyramid.md) | Accepted | [`scaffolding.create.proposal.md` §14](../scaffolding.create.proposal.md#14-adrs-this-proposal-will-be-decomposed-into) |
 | ADR-0019 | [Dual-stream scaffold telemetry (v3 verbatim + parallel `scaffold-v4-*` family)](ADR-0019-dual-stream-scaffold-telemetry.md) | Accepted | [`scaffolding.create.proposal.md` §14](../scaffolding.create.proposal.md#14-adrs-this-proposal-will-be-decomposed-into) |
+| ADR-0020 | [MCP server URL validity: when to check, and whether to block](ADR-0020-mcp-server-url-validity.md) | Accepted | [`mcp-remote-servers.md` §3](../external-dependencies/mcp-remote-servers.md#3-open-questions) |
 
 > **Reading order vs. numeric order.** The Index is in **numeric order** —
 > the immutable, reservation-ordered ADR id is the anchor for every forward

docs/02-architecture/external-dependencies/README.md

@@ -72,6 +72,7 @@ Every fact in §1 of the fact page must have a row in the code map. If a
 | Office Add-in tooling (`office-addin-project` / `office-addin-manifest`, `convertProject` precondition, MetaOS schema pins) | [`office-addin-tooling.md`](office-addin-tooling.md) | [`office-addin-tooling.code-map.md`](office-addin-tooling.code-map.md) |
 | Microsoft Graph DriveItem resolution (`/sites`, `/shares` endpoints, share-URL encoding, stable identifiers) | [`graph-driveitem-resolution.md`](graph-driveitem-resolution.md) | [`graph-driveitem-resolution.code-map.md`](graph-driveitem-resolution.code-map.md) |
 | Experimentation (Azure ExP / TAS) (`vscode/ab` endpoint, `clientid` assignment unit, `vscode` config namespace, `getTreatmentVariableAsync` feature-flag contract) | [`experimentation-exp.md`](experimentation-exp.md) | [`experimentation-exp.code-map.md`](experimentation-exp.code-map.md) |
+| Remote MCP servers (`initialize` probe contract, 2025-03-26 vs 2025-06-18 authorization discovery, endpoint-validity signals) | [`mcp-remote-servers.md`](mcp-remote-servers.md) | [`mcp-remote-servers.code-map.md`](mcp-remote-servers.code-map.md) |
 
 ## What does NOT live here
 

docs/02-architecture/external-dependencies/mcp-remote-servers.code-map.md

@@ -0,0 +1,34 @@
+# Remote MCP servers — Code Map
+
+Navigation aid for refactor work on the remote MCP server binding. Maps each
+fact in [`mcp-remote-servers.md`](mcp-remote-servers.md) to its current
+location in source.
+
+> **This file is not part of the contract.** It is expected to churn as code
+> moves. Constraints live in
+> [`mcp-remote-servers.md`](mcp-remote-servers.md#2-constraints-derived-from-these-facts);
+> updates here do not require an ADR.
+
+| Fact (from `mcp-remote-servers.md` §1) | File(s) |
+|---|---|
+| §1.1 Both auth generations — discovery entry point | `packages/fx-core/src/common/mcpToolFetcher.ts` (`resolveMCPOAuthMetadata`) |
+| §1.1 `resource_metadata` chain (2025-06-18) | `packages/fx-core/src/common/mcpToolFetcher.ts` (`candidatesFromProtectedResourceMetadata`) |
+| §1.6 Protected-resource document derived from the server URL | `packages/fx-core/src/common/mcpToolFetcher.ts` (`buildProtectedResourceCandidates`, `candidatesFromDerivedProtectedResource`) |
+| §1.1 Origin-root fallback (2025-03-26) | `packages/fx-core/src/common/mcpToolFetcher.ts` (`buildMCPServerWellKnownCandidates`) |
+| §1.1 Issuer-derived candidates, no origin-root forms | `packages/fx-core/src/common/mcpToolFetcher.ts` (`buildWellKnownCandidates`) |
+| §1.2 `initialize` POST probe | `packages/fx-core/src/common/mcpToolFetcher.ts` (`probeMCPServerAuth`) |
+| §1.2 Streamable HTTP / SSE transport selection for tool listing | `packages/fx-core/src/common/mcpToolFetcher.ts` (`fetchMCPTools`) |
+| §1.2 Shared re-export consumed by v3 call sites | `packages/fx-core/src/component/utils/mcpToolFetcher.ts` |
+| §1.2 JSON-RPC envelope check, tolerant of SSE framing | `packages/fx-core/src/common/mcpToolFetcher.ts` (`carriesJSONRPCEnvelope`) |
+| §1.3 / §1.4 Three-state URL verdict | `packages/fx-core/src/common/mcpToolFetcher.ts` (`MCPEndpointStatus`, `MCPAuthProbeResult.endpointStatus`) |
+| §1.4 Negative statuses treated as "not an MCP endpoint" | `packages/fx-core/src/common/mcpToolFetcher.ts` (`NOT_AN_ENDPOINT_STATUSES`) |
+| §1.2 Deprecated HTTP+SSE ruled out before a 4xx becomes a verdict | `packages/fx-core/src/common/mcpToolFetcher.ts` (`servesLegacySSETransport`, `announcesLegacyMessageEndpoint`) |
+| §1.4 Warning raised on the dynamic-tool-discovery scaffold path | `packages/fx-core/src/component/generator/declarativeAgent/helper.ts` (`generateForMCPForDAWithAuth`, warning type `mcpServerUrlNotAnEndpoint`) |
+| §1.4 Warning text | `packages/fx-core/resource/package.nls.json` (`core.MCPForDA.mcpServerUrlNotAnEndpoint`) |
+| §1.4 Tool-fetch based signal on the non-DT paths | `packages/fx-core/src/component/generator/declarativeAgent/helper.ts` (`generateForMCPForDA`), `packages/fx-core/src/core/FxCore.declarativeAgent.ts` (warning type `mcpNoToolsFetched`) |
+| §1.1 / §1.4 v3 endpoint resolution and placeholder fallback | `packages/fx-core/src/component/utils/mcpAuthScaffolder.ts` (`resolveMCPAuthEndpoints`) |
+| §1.1 / §1.4 v4 endpoint resolution and placeholder fallback | `packages/fx-core/src/v4/mcp/mcpAuthScaffold.ts` (`resolveEndpoints`), `packages/fx-core/src/v4/mcp/mcpAuthAction.ts` |
+| §1.4 v4 tool fetch on the static path | `packages/fx-core/src/v4/runtime/steps/mcpStatic.ts` |
+| §2.10 MCP server URL question (input-time blocking validation site) | `packages/fx-core/src/question/scaffold/vsc/teamsProjectTypeNode.ts` (`MCPForDAServerUrlNode`, `additionalValidationOnAccept`) |
+| §2.10 Rejection message | `packages/fx-core/resource/package.nls.json` (`core.createProjectQuestion.mcpForDa.ServerUrl.notAnMcpEndpoint`) |
+| §1.3 / §1.4 Measured behavior regression tests | `packages/fx-core/tests/component/utils/mcpToolFetcher.test.ts`, `packages/fx-core/tests/component/generator/declarativeAgentGenerator.test.ts` |