fix: preserve Response enumerability, refresh transport docs (#2319 round 12)
Copying url/redirected/type onto the rebuilt response as enumerable own properties changed what a consumer sees: `Object.keys`, object spread and `JSON.stringify` are all affected. Which answer is correct turns out to be the host's business — under the Fetch standard these are prototype getters with no own enumerable key, while happy-dom defines them as own enumerable data properties — so the descriptor is copied from the freshly built Response rather than either answer being hardcoded. The test compares against a baseline built in the same runtime for the same reason. Three comments still described the transport fetch as unbounded, which round 11 changed: - the wrapper's own docblock said it must never wrap the transport, obscuring that isExempt is what makes doing so safe; - the auth-chain comment in inspectorClient repeated it; - buildEffectiveAuthFetch's docblock said transport-internal discovery is not covered by a deadline. All three now state the real distinction — both chains are bounded, and they differ only in that the transport chain exempts the MCP endpoint — and the docblock says why reusing the transport chain here would be wrong rather than merely different. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
f491c7fc9a
commit
234d0a9ac4
2 changed files with 45 additions and 14 deletions
|
|
@ -220,9 +220,19 @@ function rebuildResponse(response: Response, body: ArrayBuffer): Response {
|
|||
},
|
||||
);
|
||||
for (const key of ["url", "redirected", "type"] as const) {
|
||||
// Enumerability is taken from whatever the freshly built `Response` already
|
||||
// has for this key, rather than asserted. Hard-coding either answer makes
|
||||
// the rebuilt response observably different from a native one — and which
|
||||
// answer is right is the host's business: under the Fetch standard (and
|
||||
// undici) these are prototype getters with no own enumerable key at all,
|
||||
// while happy-dom defines them as own enumerable data properties. Copying
|
||||
// the descriptor keeps `Object.keys`, object spread and `JSON.stringify`
|
||||
// identical to a response that never passed through here, in both
|
||||
// (Copilot). `false` for the getter case, where there is no own descriptor.
|
||||
const existing = Object.getOwnPropertyDescriptor(rebuilt, key);
|
||||
Object.defineProperty(rebuilt, key, {
|
||||
value: response[key],
|
||||
enumerable: true,
|
||||
enumerable: existing?.enumerable ?? false,
|
||||
configurable: true,
|
||||
});
|
||||
}
|
||||
|
|
@ -232,12 +242,15 @@ function rebuildResponse(response: Response, body: ArrayBuffer): Response {
|
|||
/**
|
||||
* Wrap a `fetch` so every call through it is bounded by `timeoutMs`.
|
||||
*
|
||||
* Apply this to an **OAuth-path** fetch only. It must never wrap the transport
|
||||
* fetch: a Streamable HTTP or SSE response is a long-lived stream that is
|
||||
* *supposed* to stay open, and a deadline there would sever every connection
|
||||
* after the budget. `InspectorClient` keeps the two apart by wrapping inside
|
||||
* `buildEffectiveAuthFetch` rather than wrapping `this.fetchFn`, which the
|
||||
* transport is handed directly.
|
||||
* Every request that reaches it unexempted is bounded — so on a chain that
|
||||
* carries anything other than OAuth work, `isExempt` is not optional. A
|
||||
* Streamable HTTP or SSE response is a long-lived stream that is *supposed* to
|
||||
* stay open, and a Streamable HTTP POST for a long-running tool call
|
||||
* legitimately withholds its response headers for minutes; a deadline over
|
||||
* either severs a working connection. `InspectorClient` uses the wrapper on
|
||||
* both of its chains and the two differ only in this: the OAuth chain passes no
|
||||
* predicate because every request on it is OAuth work, while the transport
|
||||
* chain passes `exemptMcpEndpoint`.
|
||||
*
|
||||
* `isExempt` lets one wrapped fetch serve a mixed chain. It is how the
|
||||
* *transport* fetch can be bounded at all: that chain carries both the SDK's
|
||||
|
|
|
|||
|
|
@ -849,8 +849,9 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
// URL of the RFC 8414 request that preceded it — defeating the whole point
|
||||
// of naming the endpoint — while every probe in the loop shared that one
|
||||
// request's budget (Copilot). Innermost here matches the CLI's
|
||||
// `storedAuthFetch`. The transport keeps `this.fetchFn`, deliberately
|
||||
// unbounded: its bodies are streams that are supposed to stay open.
|
||||
// `storedAuthFetch`. It also passes no `isExempt`, unlike the transport
|
||||
// chain above: every request on this chain is OAuth work, so there is
|
||||
// nothing here to exempt.
|
||||
this.effectiveAuthFetch = this.buildEffectiveAuthFetch(
|
||||
withRfc8414OidcCompat(withOAuthRequestTimeout(withOverrides)),
|
||||
);
|
||||
|
|
@ -1022,11 +1023,18 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
|
||||
/**
|
||||
* @param base - the composed OAuth-path fetch, deadline innermost (#2319).
|
||||
* Passed in rather than read off `this.fetchFn`, which is the *transport*
|
||||
* chain and must stay unbounded. The cost of keeping the two apart is that
|
||||
* the discovery the SDK runs from inside the transport (the 401/refresh path)
|
||||
* is not covered by the deadline; that leg sits inside an SDK request and is
|
||||
* bounded by its per-request timeout.
|
||||
*
|
||||
* Passed in rather than read off `this.fetchFn` because the two chains are
|
||||
* bounded on different terms, not because one of them is unbounded. Both
|
||||
* carry a deadline; the transport chain additionally exempts the MCP endpoint
|
||||
* (`exemptMcpEndpoint`), since its bodies are streams meant to stay open and
|
||||
* a long-running tool call may withhold its headers for minutes. Reusing it
|
||||
* here would extend that exemption to the OAuth manager's own requests — and
|
||||
* an authorization server published at the MCP endpoint's path would then go
|
||||
* unbounded on the one chain that exists to bound it.
|
||||
*
|
||||
* The SDK's transport-internal discovery (the 401/refresh path) is covered by
|
||||
* the transport chain's own deadline, not by this one.
|
||||
*/
|
||||
private buildEffectiveAuthFetch(base: typeof fetch): typeof fetch {
|
||||
// Capture auth response bodies (OAuth discovery, DCR, token exchange) so
|
||||
|
|
|
|||
Loading…
Reference in a new issue