feat(mcp): loopKey support for MCP-wrapped tools - #74
Merged
Conversation
Opt-in via doomLoop on callModel. Deterministic detection of runs that stop making progress: consecutive identical tool calls (per-tool fingerprint streaks over tool-declared loopKey identity, incl. repeated empty and invalid-JSON calls) and repeated text tokens (within-response block repetition + cross-step identical-text streaks). Graduated response ladder observe -> steer -> block -> stop, per-event override via the new DoomLoopDetected hook, streaks persisted in ConversationState.doomLoop across serialize/resume, and SessionEnd.reason 'doom_loop' + ModelResult.getDoomLoopVerdict() on stop.
Fingerprints: RFC 8785 (JCS) canonicalization + SHA-256/UTF-8 via WebCrypto replaces cyrb53/UTF-16 (cross-port contract now real; vectors in tests/vectors/doom-loop-fingerprints.json; bigint/NaN/circular/deep key material rejected with engine fallback to full-args identity). Streaks are round-scoped: N identical parallel calls in one round count once and share the round's decision (evaluations serialized in model-emission order under the async hash). loopKey is now function | field-list | false on the tool definition (declarative forms are data — serializable, MCP-transportable); undefined returns fall back with a warning instead of colliding. markMcp accepts a loopKey override. Stop verdicts seal state (synthesized halt outputs for unresolved calls — no dangling function_call 400s on resume), gate the allow-final-response and approval-resume request paths, persist across decision-only resumes, and clear on fresh conversational turns. Text-only no-tools stops report SessionEnd reason doom_loop. Steer guidance queued before a pause persists and delivers on resume. Server tools fingerprint at the step checkpoint (observe/steer/stop). Ladder configs warn on dead rungs and block-without-stop. Documented, test-locked misses: nonce-varying args without loopKey, paraphrased text.
Client-side: a loopKeys map on createMCPTools/rehydrateMCPTools (keyed by unprefixed MCP name; function | field list | false), threaded through buildTools into wrapMcpTool and attached via markMcp's injection point on both regular and generator wraps. Server-side: _meta['openrouter/loopKey'] on a tool definition (field list or false; data-only — functions can't cross the wire) is captured by listToolDefs, carried on McpToolDef, and round-trips through cache snapshots (SerializedMCPToolDef.loopKey) so rehydrated tool sets keep their identities. Client config wins over the server declaration.
The committed baseline (coupling 0.43, 133 import edges) predates the mcp package landing — origin/main itself measures 0.47 with 306 edges against it, passing only within tolerance. The doom-loop feature's real structural delta over current main is +0.012 coupling (0.47 -> 0.49, the new lib/doom-loop module's fan-in from model-result, tool-types, tool, async-params, index, and the mcp wrapper), which is within the gate's tolerance. Baseline regenerated from origin/main (61a2a9a) via sentrux v0.5.7 gate --save, matching the CI binary version. Cycle count (1, in the mcp package) and complex-function count (9) are pre-existing and unchanged by this branch.
Fix 3 (SDK): new 'signal' option on callModel — aborting stops the
tool loop at the next turn boundary AND aborts the in-flight request/
stream, rejecting with the abort reason; pre-aborted signals fail
before any dispatch. RequestOptions.timeoutMs now reliably bounds EACH
request even when a signal is present: the SDK skips its timeoutMs
wiring whenever a request carries a signal, so the engine composes
{run signal, caller signal, fresh per-dispatch AbortSignal.timeout}
via AbortSignal.any at every send site (initial, tool rounds, final,
retry, resume). 7 new unit tests (fake-timer driven) pin the composed
semantics.
Fixes 1+2 (DEV-658 e2e deflake): measured healthy path was 70-80s
across three GLM requests — the final turn alone burned 50-65s on a
750-1150-token reasoning burst — leaving no margin under the old 120s
budget (one CI window timed out attempt AND retry back-to-back).
Now: 300s budget, bounded generation (reasoning.maxTokens 512 +
maxOutputTokens 1024), a shallow unanswerable-without-search prompt
that preserves the leak pressure without inviting multi-hop parametric
reasoning, and a 90s per-request timeoutMs so a stalled provider fails
fast and the vitest retry gets a fresh draw. Verified 3/3 live passes
at 10-20s each.
The per-option conditional-spread chain pushed callModel to cc=16 (sentrux max 15) once the signal option landed. Build the options object once and strip undefined keys — identical absent-key semantics, one loop instead of thirteen branches.
The unit suite pins the signal/timeout composition against mocks; these five e2e tests prove what mocks cannot — the composed abort signal reaches the real fetch/stream. Covered: pre-aborted signal fails with zero dispatches; aborting mid-generation kills a live request fast (bounded wall-clock assertion, not exact timing); per-request timeoutMs bounds a live request even with a run signal present (the SDK-disabling configuration DEV-658 relies on); abort during tool execution stops before the follow-up dispatch; and a composed-signal run completes normally when neither bound fires. Verified 3/3 locally (~8s a run).
Unit: drop fake timers around AbortSignal.timeout — vitest fake timers cannot fake its Node-internal timer, so advancing the mocked clock proved nothing (a WRONG shared per-run timer would never fire in near-zero real time either, passing the per-dispatch test vacuously). Real tiny budgets (50-120ms, documented exception) make the two timeout tests actually discriminate; suite cost ~215ms. E2E: assert rejection IDENTITY, not just 'anything threw' — the mid-flight abort test walks the cause chain to OUR abort reason (the SDK wraps it in UnexpectedClientError), and the timeoutMs test matches the TimeoutError DOMException, so an unrelated transport/auth failure can no longer pass either test. Verified live: rejection shapes probed against the real API; 5/5 passing.
…tore
Review finding (cortex): DoomLoopMonitor's streak store was a plain
object; restoring a persisted blob {"tools":{"__proto__":{...}}} from
client-writable state storage reassigned the store's prototype, so
every unseen tool inherited the seeded streak (false stop verdicts /
detection bypass). The store is now a Map — hostile keys are inert
data, and a tool legitimately named __proto__ works. getState() keeps
emitting a plain-JSON record via Object.fromEntries (own-property
defines, no setter hits). Regression tests for both cases.
Also corrects the WebCrypto doc claim (Devin): globalThis.crypto is
unflagged in Node >=19, not >=18 (18 needed
--experimental-global-webcrypto and is EOL); supported floor is the
active LTS (CI runs Node 22).
…a turn New 'escalate' ladder rung between steer and block: instead of refusing or halting a stuck run, throw more intelligence at the NEXT turn and revert. Two mechanisms via doomLoop.escalation, combinable: - model: one-turn model swap (the base resolvedRequest is never mutated, so the following dispatch reverts automatically; a single- model override clears any fallback models list) - advisor: append the openrouter:advisor server tool with forwardTranscript and loop-diagnosing instructions, and pin toolChoice to it (allowed_tools/required) so the stuck model must consult before acting; object form passes through as advisor parameters A steer notice naming the detected loop accompanies the escalated turn. Budgeted: maxEscalations (default 2) per conversation, consumed at APPLICATION time (verdicts the engine never applies do not spend), escalationsUsed persisted in ConversationState.doomLoop so resumes cannot reset it, first-verdict latch so concurrent detectors in one window escalate once. Exhausted/unconfigured escalations fall through to weaker rungs; resolve-time warnings flag rung/mechanism mismatches. DoomLoopDetected action/overrideAction enums gain 'escalate' (override without config/budget downgrades to observe). 15 new tests.
![perry-the-pr-reviewer[bot]](https://avatars.githubusercontent.com/in/3222961?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
LukasParke
added
the
cortex-keep-updated
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
![cortex-github-agent[bot]](https://avatars.githubusercontent.com/u/139423088?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary
Update resolves the coverage gaps flagged in the prior pass: server-advertised loopKey parsing (advertisedLoopKey/listToolDefs) is now directly unit-tested against invalid/missing _meta shapes, and cache-types.ts gained loopKey shape validation in isSerializedToolDef so a corrupted/malformed cached loopKey now fails isSerializedMCPServer instead of being silently trusted. Core wiring (client-config precedence over server-advertised, markMcp injection, cache round-trip) is unchanged from the previous revision and remains correct.
Findings (1)
⚪ nit · packages/mcp/src/cache/cache-types.ts:64
isJsonSchemaObject(value) is evaluated twice (once for the loopKey extraction, once in the final return's boolean chain) — harmless but slightly redundant.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Summary
This update replaces the options.loopKeys?.[def.name] ?? def.loopKey precedence lookup in tool-wrapper.ts with an Object.hasOwn()-guarded lookup, fixing a real (if narrow) correctness bug: bracket access on a plain object follows the prototype chain, so an MCP tool literally named toString/constructor/valueOf/etc. would previously pick up the inherited Object.prototype method as its loopKey even when the caller's loopKeys map was empty or didn't mention that tool — silently discarding any server-advertised declaration. The fix is now covered by a dedicated regression test. No other logic changed; prior findings stand as noted below.
Findings (1)
⚪ nit · packages/mcp/src/cache/cache-types.ts:64
isJsonSchemaObject(value) is still evaluated twice in isSerializedToolDef (once for the loopKey extraction, once in the final conjunction) — unchanged from the prior revision, still just cosmetic.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
`createMCPTools` forwards an explicit allowlist of options into `rehydrateMCPTools` (FORWARDED_REHYDRATE_KEYS). `loopKeys` was missing from it, so with caching enabled a client-configured loop identity was silently discarded on every cache hit — doom-loop detection went dead on warm handles only, with no error and nothing failing. `loopKeys` is declared on both CreateMCPToolsOptions and RehydrateMCPToolsOptions, and rehydrate.ts already consumes it, so the omission was the whole bug. Adds a regression test asserting the loopKey lands on the wrapped tool after a cache hit (verified to fail without the one-line fix), and a comment on the allowlist noting that anything omitted is silently dropped — the failure mode has no signal, so the next option deserves the warning. Reported by devin on #74.
Contributor
There was a problem hiding this comment.
Summary
This update closes a real latent gap: loopKeys was missing from FORWARDED_REHYDRATE_KEYS, so with caching enabled the warm (cache-hit) path silently dropped client-configured doom-loop identities while cold connects honored them. The fix adds the key to the type-checked allowlist (create-mcp-tools.ts:47), documents the drop-on-omission hazard, and adds an end-to-end regression test that drives createMCPTools through the cache hit and asserts loopKey lands on the rehydrated tool. No new issues; only a cosmetic nit remains.
Findings (1)
⚪ nit · packages/mcp/src/cache/cache-types.ts:63-72
Unchanged from prior revisions: isJsonSchemaObject(value) is evaluated twice in isSerializedToolDef (once to extract loopKey, once in the return conjunction). Cosmetic only.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
extractServerToolIdentity built `identity` as a plain `{}`. Server-tool
output items are JSON-parsed API responses, so a `__proto__` key arrives
as a real own property — assigning it hit the prototype setter instead of
creating an own property, and canonicalizeKeyMaterial's Object.keys walk
then skipped it.
Perry flagged this as a consistency nit with minimal impact. It is
actually a false-positive source: two server-tool calls differing ONLY in
a `__proto__`-named field fingerprint identically, so they collide into a
streak and the detector reports a doom loop that is not one.
Adds a regression test with three same-query web_search items carrying
distinct `__proto__` values, asserting no detections. With the plain `{}`
it fails with two spurious detections.
Consistent with the field-list subset in resolveLoopKeyMaterial and the
Map-backed streak store.
Drops the duplicate isJsonSchemaObject call and, more usefully, removes a trap: the old form extracted loopKey via a ternary, so for a non-object value loopKey silently became `undefined` and hasValidLoopKey read as true. It was harmless because the conjunction re-checked the object-ness, but the local said "valid" about a value that was never inspected. Also records why each accepted loopKey shape is accepted. Suggested by Perry on #74.
Contributor
There was a problem hiding this comment.
Summary
Only change since the last head is the isSerializedToolDef refactor in packages/mcp/src/cache/cache-types.ts:62-76: it early-returns on a non-object value and reads loopKey once, which resolves the standing cosmetic nit about the duplicated isJsonSchemaObject(value) call with identical accept/reject semantics. No functional changes elsewhere and no new issues; all prior findings are now closed.
✅ No findings.
`advisor` is `boolean | Record<string, unknown>`, so `advisor: false` is
the documented way to switch it off. Config resolution counted any
non-undefined advisor as a mechanism, while the recovery step skips a
disabled one (`config.advisor !== undefined && config.advisor !== false`).
So `{ escalation: { advisor: false } }` resolved a working escalate rung
that, on trigger, called consumeEscalation() and applied no override at
all — burning one of the run's limited escalations and announcing recovery
while doing nothing. Note consumeEscalation() runs before the advisor
check, so the budget was spent regardless.
Now `{ advisor: false }` alone reads the same as `{}` (no rung), and a
disabled advisor is dropped from the resolved config rather than carried
through inert.
Adds three tests: advisor:false alone resolves no escalation; alongside a
real `model` it does not appear in the resolved config; advisor:true still
resolves. The first two fail without the fix.
Reported by devin on #73.
Contributor
Re-review at
|
`.agents/skills/public-api-examples/SKILL.md` requires a fenced example in
the changeset for any public-API change. `loopKeys` on createMCPTools /
rehydrateMCPTools and `SerializedMCPToolDef.loopKey` are public API, and
the changeset was prose-only.
Shows the client-side `loopKeys` map (field-list and `false` forms, keyed
by unprefixed MCP name) feeding a `doomLoop` run, plus the server-advertised
`_meta['openrouter/loopKey']` alternative and the precedence rule.
Shapes verified against source: loopKeys at types.ts:97, cache
{store, key?} at :73, handle.tools at :112.
Contributor
|
Base switched to Previous passes (heads The MCP portion is byte-identical to the head I approved and I have no new findings there. What changed is what an approval means: it would now bless the Two possibilities:
Advisory notes on the newly-in-scope code, from what is visible:
|
Contributor
There was a problem hiding this comment.
Summary
The MCP loopKey code is unchanged from the head I approved (6e5148c) and remains clean — all prior findings stay resolved. What changed is the base: retargeting from feat/doom-loop-detector to main pulled the whole stacked doom-loop/escalation/cancellation surface (~5,000 new lines across packages/agent, plus three @openrouter/agent changesets) into this PR's own diff, which is also truncated at 200 KB, so I can neither review it here nor approve it as if I had.
Findings (4)
🟠 major · packages/agent/src/lib/doom-loop.ts:1-1088
Base switch to main expanded this PR from 9 MCP files to 34, adding ~5,000 lines of agent-engine code (doom-loop detector, escalate ladder, run cancellation) plus three @openrouter/agent changesets, none of which the PR title/body describes and none of which was in scope for the four prior review passes. Either re-target at feat/doom-loop-detector (diff collapses to the already-approved MCP subset) or update the description and point to where this engine code was reviewed.
🟡 minor · packages/agent/src/lib/model-result.ts:2985
AbortSignal.any (Node >= 20.3) and globalThis.crypto.subtle in doom-loop.ts:520 (Node >= 19) raise the effective runtime floor, but packages/agent/package.json declares no engines field — Node 18 consumers hit a runtime TypeError only once they opt into signal/doomLoop.
🟡 minor · packages/agent/src/lib/model-result.ts:2981
this.options.client._options.timeoutMs reads an underscore-prefixed SDK internal; an upstream rename would silently drop the client-level timeout on the composed-signal path with no type error or failing test.
⚪ nit · packages/agent/src/inner-loop/call-model.ts:140-169
The option assembly was rewritten to a Record<string, unknown> plus a delete-undefined loop and a double cast (as unknown as GetResponseOptions), trading the previous compile-time checking of each forwarded key for runtime pruning — a future renamed option would no longer fail typecheck.
Contributor
Author
|
CI note on The failing criterion is complex functions 9 → 10 — not coupling (this PR is a different case from #86, which fails on coupling against a much older What the evidence says:
So the +1 appears only when #73's code is combined with this branch's, via the I am not touching the gate config or thresholds, and I am not restructuring working code to chase a metric that neither PR moved on its own. Two reasonable paths, and this is a human call:
All review threads here are resolved and |
Contributor
Re-review —
|
There was a problem hiding this comment.
Perry's Review
Verdict: 💬 Comments / questions
Risk: 🟡 Medium
Details
Re-reviewing after c1b0c02 (changeset example fix). All prior review threads are resolved, cortex approved, and the loopKey implementation is clean: server-advertised _meta parsing, client-side loopKeys map with Object.hasOwn precedence, cache snapshot validation in isSerializedToolDef, and the FORWARDED_REHYDRATE_KEYS fix for cache hits. The changeset now includes the required API example.
CI: structural-gate FAILURE (complex functions 9 to 10). The effective diff against main is only 11 files / 559 insertions (the gh pr diff includes merge history from the merged doom-loop PR). The one additional complex function is in the new loopKey code. Whether this is acceptable is a human judgment call — it's a single function crossing the sentrux threshold, not a structural regression across the codebase.
One cross-PR concern worth flagging: this PR still imports from the v1 MCP SDK and uses the three-argument callTool signature with the undefined middle parameter. PR #86 migrates to the v2 SDK and removes that middle argument. When whichever merges first lands, the other will need a follow-up to reconcile the import and callTool signature. This isn't a defect in either PR individually, but the merge order matters.
An empty `loopKey` field list — or one whose every field is absent from the arguments — produced an empty key subset, so every call to the tool fingerprinted identically. A detector armed to block a repeat would then refuse the SECOND unrelated call: `ls` and `rm -rf /` share an identity. `advertisedLoopKey` accepts `[]` (`[].every` is vacuously true), so once a server can advertise `_meta['openrouter/loopKey']` this is reachable from the wire, not just from a local mistake. Both shapes now warn and fall back to full arguments, mirroring how the function form already handles a degenerate result. `false` remains the way to exempt a tool. refactor(mcp): keep toCreateOptions under the complexity ceiling Ports the key-list + copy-loop from 6a87464 on the dual-protocol branch. `toCreateOptions` was at the max_cc=15 ceiling and this PR's `loopKeys` spread pushed it over, failing the structural gate. Whichever of the two PRs lands second inherits the other's key list.
Contributor
Author
|
Head is now structural-gate. Ported the key-list + copy-loop from
Three tests added, including Verification: 646 agent + 59 mcp tests pass, typecheck and lint clean. Non-blocking note you raised: the branch is still stacked on the pre-merge base, so GitHub shows inflated file/line counts. |
Comment on lines
+35
to
+46
| function advertisedLoopKey( | ||
| meta: Record<string, unknown> | undefined, | ||
| ): readonly string[] | false | undefined { | ||
| const advertised = meta?.['openrouter/loopKey']; | ||
| if (advertised === false) { | ||
| return false; | ||
| } | ||
| if (Array.isArray(advertised) && advertised.every((f) => typeof f === 'string')) { | ||
| return advertised as string[]; | ||
| } | ||
| return undefined; | ||
| } |
Contributor
There was a problem hiding this comment.
🔍 Server-advertised loopKey is trusted unconditionally, including false
advertisedLoopKey accepts false (full exemption) and any string array straight off _meta['openrouter/loopKey'], and wrapMcpTool applies it whenever the client hasn't configured an override (packages/mcp/src/tool-wrapper.ts:101-104). The agent-side guard added in this PR only protects against the degenerate empty list (packages/agent/src/lib/doom-loop.ts:469-476); a server can still fully disable doom-loop detection for its own tools by advertising false, or collapse all its calls onto one identity by advertising a field that is constant across calls (e.g. ['api_version']), which bypasses both new guards since the subset is non-empty. Worth deciding whether server declarations should be opt-in (e.g. a trustServerLoopKeys flag) rather than honored by default.
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
Author
There was a problem hiding this comment.
Both of these are the same finding at two severities, and I'm escalating rather than deciding it — it's a question about this feature's trust model, not a defect in its implementation.
Verified the mechanism at head. advertisedLoopKey (handle.ts:35-46) returns false or a string array verbatim, and wrapMcpTool (tool-wrapper.ts:101-104) applies it whenever the client hasn't set loopKeys[name]. So server declarations are honored by default, and:
false→resolveLoopKeyMaterialreturns{kind:'exempt'}, disabling detection for that tool entirely.['api_version'](or any field constant across calls) → a non-empty subset, so it passes both guards I added in6f9f3c1while still collapsing every call onto one identity.
You're right that my guards don't cover these. They were scoped to the degenerate shapes — empty list, all-fields-absent — where no legitimate intent exists. A constant-valued field is indistinguishable from a correct narrow key without knowing the tool's semantics, and false is a legitimate declaration that a tool repeats by design. Neither can be fixed by validating the value.
The real question is the default, and there are three answers:
trustServerLoopKeysdefaulting to false (your suggestion). Servers can advertise; clients opt in. Safest, but it makes the feature inert out of the box, which may defeat the point of wire advertisement.- Honor field lists, ignore
false. A server can narrow its identity but never exempt itself. Keeps most of the value; removes the total-bypass case. - Keep it as-is, document the trust boundary. Defensible if MCP servers are already trusted — they execute arbitrary tool calls, so a hostile server has far worse options than weakening a loop detector.
My read is (2), because it's the only one that removes the sharp edge without making the feature opt-in — but it's a judgment about how much an MCP server is trusted, which belongs to whoever owns that boundary. Leaving both threads open.
Worth noting for whoever decides: loopKey is a safety control, not a security boundary. It bounds wasted spend on a looping agent. A hostile server neutralizing it costs money, not integrity — which argues this is lower severity than the 🟨 marking suggests, though still worth a deliberate default.
Comment on lines
+35
to
+46
| function advertisedLoopKey( | ||
| meta: Record<string, unknown> | undefined, | ||
| ): readonly string[] | false | undefined { | ||
| const advertised = meta?.['openrouter/loopKey']; | ||
| if (advertised === false) { | ||
| return false; | ||
| } | ||
| if (Array.isArray(advertised) && advertised.every((f) => typeof f === 'string')) { | ||
| return advertised as string[]; | ||
| } | ||
| return undefined; | ||
| } |
Contributor
There was a problem hiding this comment.
🟨 Remote MCP server can disable doom-loop detection for its own tools
A tool definition fetched from a remote MCP server can now declare _meta['openrouter/loopKey'] and that value is trusted verbatim (advertisedLoopKey at packages/mcp/src/handle.ts:35-46, applied at packages/mcp/src/tool-wrapper.ts:101-104). A hostile or compromised server can send false, which makes the wrapped tool statically exempt from doom-loop detection (resolveLoopKeyMaterial returns {kind:'exempt'} for false, packages/agent/src/lib/doom-loop.ts:454-458), or a field list naming a constant/absent-in-practice field to collapse many distinct calls onto one identity. The agent-side guard added in this PR only catches the empty-array and all-fields-absent degenerate cases, not false or a constant single-field list, so the server-controlled path can still neutralize a client-side safety control the operator explicitly enabled via doomLoop.
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
Author
There was a problem hiding this comment.
Both of these are the same finding at two severities, and I'm escalating rather than deciding it — it's a question about this feature's trust model, not a defect in its implementation.
Verified the mechanism at head. advertisedLoopKey (handle.ts:35-46) returns false or a string array verbatim, and wrapMcpTool (tool-wrapper.ts:101-104) applies it whenever the client hasn't set loopKeys[name]. So server declarations are honored by default, and:
false→resolveLoopKeyMaterialreturns{kind:'exempt'}, disabling detection for that tool entirely.['api_version'](or any field constant across calls) → a non-empty subset, so it passes both guards I added in6f9f3c1while still collapsing every call onto one identity.
You're right that my guards don't cover these. They were scoped to the degenerate shapes — empty list, all-fields-absent — where no legitimate intent exists. A constant-valued field is indistinguishable from a correct narrow key without knowing the tool's semantics, and false is a legitimate declaration that a tool repeats by design. Neither can be fixed by validating the value.
The real question is the default, and there are three answers:
trustServerLoopKeysdefaulting to false (your suggestion). Servers can advertise; clients opt in. Safest, but it makes the feature inert out of the box, which may defeat the point of wire advertisement.- Honor field lists, ignore
false. A server can narrow its identity but never exempt itself. Keeps most of the value; removes the total-bypass case. - Keep it as-is, document the trust boundary. Defensible if MCP servers are already trusted — they execute arbitrary tool calls, so a hostile server has far worse options than weakening a loop detector.
My read is (2), because it's the only one that removes the sharp edge without making the feature opt-in — but it's a judgment about how much an MCP server is trusted, which belongs to whoever owns that boundary. Leaving both threads open.
Worth noting for whoever decides: loopKey is a safety control, not a security boundary. It bounds wasted spend on a looping agent. A hostile server neutralizing it costs money, not integrity — which argues this is lower severity than the 🟨 marking suggests, though still worth a deliberate default.
LukasParke
added a commit
that referenced
this pull request
Aug 3, 2026
#74 landed on main as f412281, which conflicted in four places. All four are additive — both sides kept: - build-tools.ts: this branch's SDK import path (@modelcontextprotocol/client, the point of the dual-revision work) plus main's ToolLoopKey symbol. - rehydrate.ts PASS_THROUGH_CREATE_KEYS: this branch's protocolNegotiation and probeTimeoutMs plus main's loopKeys. This is the cross-PR inheritance Devin predicted — whichever landed second takes the other's key list. - rehydrate.test.ts: this branch's snapshotWithDuplicateToolNames and two staleness describes plus main's loopKeyOf helper and its cache-hit forwarding describe. Verified: 14 mcp + 51 agent test files pass (18/18 in rehydrate.test.ts, covering both sides' additions), typecheck and lint clean.
LukasParke
added a commit
that referenced
this pull request
Aug 3, 2026
#74 (loopKey) and #91 (toolCall in hook context) landed on main. Three conflicts, all resolved as unions: - tool-executor.ts: both sides changed buildExecuteCtx's call sites — this branch added `extras`, main added `toolCall`. The signature had already auto-merged to accept both; the three call sites now pass both. - doom-loop.ts / doom-loop.test.ts: main's loopKey field-array guard (the empty-list and all-fields-absent fallbacks) arrived wholesale; this branch had nothing there. Two fixes the merge made necessary: - tool-types.ts: this branch carried a pre-#74 `ToolLoopKey` without the `readonly string[]` member, so `tool-wrapper.ts` failed to typecheck against the field-array form main now produces. Restored main's union. - model-result.ts crossed the fan-out ceiling again (16, limit 15) once the merge landed. `normalizeInputToArray` now comes from `conversation-state.js`, which already imported it and is the natural owner of conversation-input normalization. God files back to 0. Also carries a review fix (Devin, tool-context.ts): NEVER_ABORT_SIGNAL was one process-wide signal shared by every context built without cancellation sources. A tool body subscribing to it leaked a listener for the process lifetime, since the signal never fires and nothing removes them. Replaced with a per-context factory; two tests pin it, mutation-verified. Verified: 62 agent + 10 mcp test files pass, typecheck and lint clean, gate reports God files 0 -> 0 and complex functions at baseline.
LukasParke
added a commit
that referenced
this pull request
Aug 3, 2026
loop-key.test.ts arrived from main (#74) importing @modelcontextprotocol/sdk/client/index.js, which this branch removed from packages/mcp dependencies in favour of @modelcontextprotocol/client. It resolved locally only because the old package is still present transitively; a clean CI install would fail typecheck and lint, both of which compile tests/** per turbo.json. The last site in the migration — nothing else under src/ or tests/ still names the old package.
LukasParke
added a commit
that referenced
this pull request
Aug 4, 2026
…07-28) (#86) * fix(mcp): correct client version, cover transport fallback, flag 2026-07-28 deprecations MCP protocol revision 2026-07-28 shipped today. This is the non-breaking groundwork; the migration itself is deliberately deferred. Why defer: no released SDK negotiates 2026-07-28 by default. Verified empirically against a local HTTP capture — @modelcontextprotocol/client@2.0.0 still sends the `initialize` handshake with protocolVersion "2025-11-25" and omits the Mcp-Method / Mcp-Name headers the new revision requires, and @modelcontextprotocol/core@2.0.0 does not export LATEST_PROTOCOL_VERSION at all (internal value is "2025-11-25"). Migrating today would restructure the dependency tree without changing a byte on the wire, while breaking published API. Changes: - Fix the self-reported client version: DEFAULT_CLIENT_INFO said '0.1.0' while the package is 0.0.1. The published 0.0.1 tarball ships this, so every server it connects to is told the wrong version. - Add tests/unit/mcp-connection.test.ts (9 cases) covering transport selection and the Streamable HTTP -> SSE fallback. Every existing unit test vi.mocks mcp-connection.js, so this path had no coverage; these fake the SDK transports instead so the real connect() runs. Verified the suite fails when the pinned-transport guard is broken. - Mark @deprecated, type-level only: SerializedMCPServer.sessionId (sessions removed, SEP-2567), CreateMCPToolsOptions.onElicitation (server-initiated elicitation removed for MRTR, SEP-2322), and MCPTransportKind 'sse' (HTTP+SSE deprecated, SEP-2596). - Document the negotiated revision and the full migration gap in the README. No runtime behavior changes and no breaking API changes. Co-Authored-By: Claude <noreply@anthropic.com> * fix(mcp): generate clientInfo version from package.json Replaces the hardcoded '0.0.1' in DEFAULT_CLIENT_INFO with a constant generated from package.json, so the version we self-report to every MCP server cannot drift from the package we actually publish. package.json is the source of truth. `build` runs gen-version.mjs before tsc, so a changesets version bump is picked up automatically before publish (the release workflow runs `pnpm run build` ahead of `changeset publish`). src/version.ts is committed rather than gitignored: CI's lint, typecheck, and unit-test jobs compile src without a build step, and turbo's `dependsOn: ["^build"]` only builds upstream packages, so nothing would regenerate it in those jobs. tests/unit/version.test.ts closes the gap by failing when the committed constant drifts from package.json. Verified: - bump package.json to 0.1.0 without regenerating -> drift test fails - run build -> file regenerates, test passes - dist layout unchanged (esm/index.js, not esm/src/), all 6 export-map paths resolve - esm/version.js ships in the tarball; scripts/ does not - 571 tests pass, lint and typecheck clean Note: importing package.json directly was tried and rejected — it pulls the file into the compilation, shifting the implicit rootDir so output becomes esm/src/**, which invalidates every path in the export map. module: "Node16" also rejects JSON import attributes. Co-Authored-By: Claude <noreply@anthropic.com> * chore(mcp)!: swap SDK v1 for @modelcontextprotocol/client v2 Replaces @modelcontextprotocol/sdk@^1.29.0 with @modelcontextprotocol/client@^2.0.0. Every item here is compile-forced — the package does not build without all of them — so they land together. - All 8 source import sites plus 4 test specifiers collapse to the single '@modelcontextprotocol/client' package. Notably we do NOT add @modelcontextprotocol/core as a direct dependency: its '.' export is a zod-schema barrel (173 exports, all /Schema/), every type and value we use lives in `client`, and core arrives as a pinned transitive dep. - callTool loses its middle argument: v2 is `callTool(params, options)`. This is the one change with runtime rather than compile-time consequences — leaving the v1 three-arg form would have put `signal` and `onprogress` in a dropped slot, silently killing cancellation and progress streaming. Verified against v2's types: `TS2554: Expected 1-2 arguments, but got 3`. - setRequestHandler / setNotificationHandler are method-name-first in v2. Spec methods supply their own schema; passing a bare zod schema as the second argument crashes at runtime reading '~standard'. The elicitation handler is unchanged otherwise and now serves both protocol eras — on 2026-07-28 the multi-round-trip driver dispatches input_required through this same handler. - Deletes the isTransport runtime guard. It existed solely because SDK v1 typed `sessionId` as `string | undefined` rather than optional, which exactOptionalPropertyTypes rejected at the connect() call site. v2 declares it optional, so client.connect() typechecks directly and connectWith() inlines into its three call sites. - Collapses the three v1 module mocks in mcp-connection.test.ts into one factory on the unified package, and records versionNegotiation.mode per constructed Client so a later commit can assert the negotiation default with no network. customConditions: [] stays — eventsource and eventsource-parser still ship exports.source pointing at raw .ts, which is the original reason. Verified: typecheck clean, 50/50 unit tests pass, real (unmocked) build succeeds. No behavior change intended in this commit. Co-Authored-By: Claude <noreply@anthropic.com> * test(mcp): cover both protocol eras and the callTool argument shape Two new suites, no source changes — so they characterize the SDK's current behavior (versionNegotiation defaults to 'legacy') before the next commit opts us into 'auto'. tests/unit/protocol-era.test.ts (9 tests) runs a hand-rolled MCP server over InMemoryTransport, so there is no network, no fixture process, and no MCP_TEST_URL gate. It pins the facts the rest of the package depends on: - legacy server: server/discover probe, then initialize fallback, getProtocolEra() === 'legacy' - modern server: NO initialize at all, getProtocolEra() === 'modern' - modern server still populates getServerVersion() and getServerCapabilities() — handle.ts reads both synchronously, so if the modern era left them empty, resource tools would silently vanish and snapshots would lose serverInfo - sessionId is undefined in the modern era (SEP-2567) - input_required is fulfilled through the SAME registered elicitation/create handler, then the call is retried (SEP-2322) — this is what justifies keeping onElicitation rather than deprecating it - { pin } fails loudly when the revision is not offered Two field names worth recording, both of which I got wrong first: the server/discover result field is `supportedVersions` (not `protocolVersions`), and `inputRequests` is an object keyed by request id (not an array). tests/unit/call-tool-shape.test.ts (3 tests) guards the v2 callTool signature. That regression is silent rather than loud: with the v1 three-arg form, `signal` and `onprogress` land in a slot the SDK does not read, so cancellation and progress stop working while every other test still passes. Verified by mutation — restoring the three-arg call fails all 3. Both suites mutation-tested: dropping modern advertisement fails 4 era tests; the MRTR handler-invocation assertion is load-bearing. 62/62 unit tests pass, typecheck and lint clean. Co-Authored-By: Claude <noreply@anthropic.com> * feat(mcp)!: negotiate protocol revision 2026-07-28 by default Any MCP server now works out of the box, whether it speaks 2025-11-25 or 2026-07-28. Previously we only spoke the 2025 handshake. Adds `protocolNegotiation?: 'legacy' | 'auto' | { pin: string }` to CreateMCPToolsOptions and RehydrateMCPToolsOptions, mapped onto the SDK's versionNegotiation. Defaults to 'auto' — the SDK itself defaults to 'legacy', but a library whose job is "point it at a server" should reach both eras with no configuration. Under 'auto' the client probes with server/discover, then either goes modern (per-request _meta envelope, no handshake) or falls back to the 2025 initialize handshake. `'legacy'` skips the probe, which matters on flaky servers: on HTTP a probe timeout is treated as an outage and rejects, where 'legacy' may still connect. `{ pin }` fails loudly rather than falling back. The type is declared in transport-types.ts rather than re-exported from the SDK, so this does not put an SDK type in our public API — the problem MCPAuth already has with OAuthClientProvider. Threaded through every path that reaches connect(), including FORWARDED_REHYDRATE_KEYS. Omitting it there would have made a cache HIT silently fall back to the default while a cache MISS honoured the caller — the same bug class as the pre-existing staleness gap. `inputRequired` is deliberately left unset: the SDK's defaults (auto-fulfil on, 10 rounds) are what we want, and pinning them would freeze values the SDK may tune. 4 new tests assert the 'auto' default, explicit 'legacy', pin pass-through, and that the policy also applies to the SSE fallback client. Mutation-verified: reverting the default to 'legacy' fails the default test. 66/66 unit tests pass, typecheck and lint clean. Co-Authored-By: Claude <noreply@anthropic.com> * fix(mcp): honour staleness.maxAgeMs on the direct rehydrate path Pre-existing gap, unrelated to the protocol migration — separated out so it doesn't read as migration fallout. `staleness.maxAgeMs` was only checked in `createMCPTools`'s cache-hit path (create-mcp-tools.ts). A caller holding their own snapshot and calling `rehydrateMCPTools()` directly got no staleness check at all — `rehydrate.ts` never read `cachedAt` — so tools of unbounded age were replayed silently. Adds `snapshotIsStale()` beside the existing `tokensExpired()` and folds it into the same guard, so a stale snapshot routes through `freshConnect` exactly like expired tokens or missing credentials already do. Also adds `staleness` to `RehydrateMCPToolsOptions` and to FORWARDED_REHYDRATE_KEYS. `toCreateOptions` deliberately does NOT forward it: that builds options for a fresh connect, which has no snapshot age to compare against. 3 tests cover within-maxAge replay, over-maxAge re-list, and no-maxAge replay-regardless-of-age. Mutation-verified: dropping the check fails the over-maxAge test. The fake client in rehydrate.test.ts gained a `listTools` stub, which is now reachable because a stale snapshot falls through to freshConnect. 69/69 unit tests pass, typecheck and lint clean. Co-Authored-By: Claude <noreply@anthropic.com> * docs(mcp): retire the deprecations that dual-era support made false PR #86 originally annotated three surfaces as doomed under 2026-07-28, on the premise that we could not speak that revision. Now that we negotiate both, one of those annotations was simply wrong and the other two needed re-tensing. - REMOVE the @deprecated on `onElicitation`. It works on both revisions: 2025-era servers send `elicitation/create`, and on 2026-07-28 the SDK's multi-round-trip driver routes `input_required` through the same handler. Replaced with an explanation of the dual mechanism. - KEEP `transport: 'sse'` deprecated — SEP-2596 stands regardless. - RE-TENSE the `sessionId` notes from "will be removed" to "is undefined on modern connections", and add matching notes on ConnectOptions and MCPConnection, which were newly no-ops against 2026-07-28 servers and carried no annotation at all. - ADD `MCPOAuthClientProvider`, a re-export of the SDK's OAuthClientProvider under our own name. That type is reachable through the public `MCPAuth` oauth variant, so consumers were importing it from the SDK directly — an import path that just changed under them. Now they can name it without depending on our dependency. - REWRITE the README protocol section. Its central claim — "no released SDK negotiates it by default ... migrating today would change our dependency tree without changing a single byte on the wire" — was the stated reason for not migrating, and is now false. Replaced with what actually happens on the wire per revision, how to override, and the three real behavioral differences. 69/69 unit tests pass, typecheck and lint clean. Co-Authored-By: Claude <noreply@anthropic.com> * chore(mcp): changeset for 0.1.0 Bumps minor rather than patch: this release breaks the public API (OAuth provider type) and pre-1.0 minors are the conventional vehicle. A patch would land 0.0.2, indistinguishable from the queued bugfix. Deletes .changeset/mcp-2026-07-28-prep.md and folds its still-true content into the new changeset. It asserted "no released SDK speaks 2026-07-28 by default yet" and claimed the onElicitation deprecation — both false in this same release, and shipping three release notes that contradict each other would be worse than one coherent one. Co-Authored-By: Claude <noreply@anthropic.com> * refactor(mcp): keep toCreateOptions under the complexity ceiling The structural gate failed with "Complex functions increased: 9 -> 10". The culprit was `toCreateOptions` at cc=16, one over the max_cc=15 in .sentrux/rules.toml: it had grown to 14 conditional spreads, two of them added by this PR (protocolNegotiation, and staleness reaching the options surface). Replaces the per-key spreads with a typed key list and one copy loop, mirroring `forwardedRehydrateOptions` in create-mcp-tools.ts which already forwards the same set in the opposite direction. `as const satisfies readonly (keyof RehydrateMCPToolsOptions & keyof CreateMCPToolsOptions)[]` means a key that isn't valid on both types fails to compile rather than silently dropping — the same guard the sibling helper uses. `staleness` is deliberately excluded: it compares a snapshot's age, and this builds options for the fresh-connect fallback which has no snapshot. Verified locally with sentrux 0.5.7 (same version CI pins): complex functions back to 9, and the 9 remaining are all pre-existing in packages/agent, untouched here. Behavior unchanged — 69/69 unit tests pass, typecheck and lint clean. Co-Authored-By: Claude <noreply@anthropic.com> * ci: regenerate committed generated files during changeset version The Version PR that changesets opens would have failed its own CI. Chain, all three links verified: - packages/mcp/src/version.ts is generated but committed (deliberately — see the docblock in scripts/gen-version.mjs: lint/typecheck/test run without a build step). - tests/unit/version.test.ts asserts PACKAGE_VERSION === package.json's version. - `changeset version` bumps package.json and commits, regenerating nothing. turbo's `test` task is `dependsOn: ["^build"]` — `^` is upstream deps only, so mcp's own build (which runs gen:version) never runs before its tests. Reproduced by bumping packages/mcp/package.json to 0.0.2 and running `turbo run test --filter=@openrouter/mcp`: AssertionError: expected '0.0.1' to be '0.0.2' Fixes it at the source rather than loosening the test, which is the part that actually catches drift. Adds a root `version` script that runs `changeset version`, then `turbo run gen:version`, then refreshes the lockfile — and points changesets/action at it. Same bump, same commit, generated file included. Also adds the `gen:version` turbo task (uncached; package.json in, src/version.ts out) so any future package with committed generated output is picked up automatically. Verified: same bump with the fix applied regenerates version.ts to 0.0.2 and the mcp suite passes 69/69. Reported independently by devin (x2) and cortex on #86. * docs(changeset): document the 'auto' default as breaking + add API example Two findings from devin's re-review of e4c0273. 1. `protocolNegotiation` defaulting to 'auto' where the SDK defaults to 'legacy' was documented only in a code comment at mcp-connection.ts. The changeset's Breaking section listed just the OAuth type change, so a consumer reading the CHANGELOG and not the source learned nothing about the riskier change. Now a second Breaking bullet names the failure mode (probe against a gateway that rejects unknown methods goes from working to failing) and the escape hatch. 2. `.agents/skills/public-api-examples/SKILL.md` requires a fenced example in the changeset for any public-API change. This adds `protocolNegotiation` and exports `MCPOAuthClientProvider`, and was prose-only. Shapes verified against source: protocolNegotiation at types.ts:51, `{ pin: string }` at transport-types.ts:31, MCPOAuthClientProvider exported from index.ts:4. Does not resolve whether 'auto' should stay the default — devin proposes a one-shot 'legacy' retry on connect failure, which is a design call for a human. This only stops the change being undocumented. * chore: drop --lockfile-only from the release script, un-reformat turbo.json Both from review of e4c0273. - `pnpm install --lockfile-only` removed from the `version` script. cortex and devin both flagged it: `changeset version` only rewrites `version` fields and CHANGELOGs, and all internal deps are `workspace:*`, so there is nothing for it to pick up. It is a no-op in the expected case, and its one real effect — an unreviewed lockfile diff inside "chore: version packages" — has no upside. - turbo.json restored to the file's own compact style. My edit had gone through json.dumps(indent=2), expanding every array to one element per line and turning a 5-line addition into a 61-line diff. Nothing formats root turbo.json (`lint` is `turbo run lint`, which only covers packages/*/src and packages/*/tests), so the reformat was gratuitous. The semantic diff against main is now exactly two entries: the new `gen:version` task, and `scripts/**` added to `build.inputs` so a change to gen-version.mjs invalidates the build. Verified by comparing parsed JSON rather than the textual diff — the first pass at un-reformatting silently dropped `scripts/**`. Release fix re-verified after the edits: bump to 0.0.2 → gen:version regenerates → mcp suite 69/69. * fix(mcp): don't reset the staleness clock on a snapshot replay `makeHandle` writes the snapshot back to the cache on construction, and `serializeArgs` stamped `cachedAt: Date.now()` unconditionally. On the replay path (rehydrate.ts -> makeHandle with snapshotToToolDefs) that restamped a tool set that was never re-listed, so every rehydrate pushed the age back to zero. Net effect: `staleness.maxAgeMs` could never fire on a repeatedly -rehydrated snapshot. A process that rehydrates on each use silently served unbounded-age tools — the exact guarantee this PR added the staleness check in rehydrate to provide. `makeHandle` now takes `replayedCachedAt`, threaded from `snapshot.cachedAt` on the replay path only. `refresh()` clears it, because that path does re-list and its age genuinely restarts. A cold connect omits it and stamps now, unchanged. Regression test asserts the written-back snapshot keeps its original cachedAt (fails without the fix: expected 1785353064705 to be 1785353004705, i.e. restamped to now). Reported by devin on #86. * fix(mcp): close a client whose connect() failed The SDK stores the transport before calling `start()` and returns without teardown when `start()` itself throws, so a client whose `connect()` rejected still holds an open socket. Release it explicitly on every failure path in `connect()`: pinned SSE, pinned Streamable HTTP, the HTTP-to-SSE fallback, and the both-failed case. `close()` is idempotent — `this._transport?.close()`, and each transport guards on a `_closed` flag — so covering the one path the SDK misses cannot double-close the paths it already handles (the probe failure and the `initialize` handshake, both of which self-clean). Pre-existing rather than a regression, but the `'auto'` default makes it reachable far more often: a `server/discover` probe timeout against a strict gateway is now an expected failure mode, and each one leaked a keep-alive connection during the probe-to-fallback window. * test(mcp): cover refresh() clearing a replayed cachedAt The replay-preserves-age case was tested; its mirror was not. If `refresh()` stopped clearing `replayedCachedAt`, a handle built from a snapshot would keep writing the original timestamp after a genuine re-list, so every later rehydrate would read a permanently-stale age and take the `freshConnect` path — the cache would never warm again. Verified the test fails when that line is removed. * docs(changeset): note the failed-connect transport leak fix * fix(mcp): honour staleness when reconnectOnExpiry is false `rehydrateMCPTools` ANDed the stale check with `reconnectOnExpiry`, so `{ staleness: { maxAgeMs }, reconnectOnExpiry: false }` replayed a snapshot of any age with neither a re-list nor an error — the exact hole `staleness.maxAgeMs` exists to close, still open on that one path. `reconnectOnExpiry: false` opts out of rebuilding the transport, not out of bounded-age tools. A stale snapshot now re-lists over the connection already opened for the replay, which honours `maxAgeMs` without the reconnect the caller declined. `refresh()` also clears the carried `cachedAt`, so the write-back records the new age rather than the snapshot's. Also corrects a stale comment: `- 60_000` is a minute, not an hour. * fix(mcp): close the replay connection when rehydrate fails `rehydrateMCPTools` opened a connection for the snapshot replay and, on a post-connect failure, neither closed it nor let anything else reach it — so the transport leaked for the process lifetime. A service that retries exhausts its sockets. `freshConnect` already guarantees this on the cold path (handle.ts tears the connection down in its catch); this gives the replay path the same guarantee. Reachable via `buildTools` rejecting on a duplicate tool name, a caller's `cache.store.set` throwing during the initial write-back, or the stale re-list failing. `connection` moves outside the `try` so the `catch` can reach it. It stays undefined when `connect()` itself fails, which is correct — `connect()` releases its own client on every failure path. * test(mcp): cover list_changed dispatch and clientInfo; tighten pin type Three gaps cortex flagged, all silent-failure shaped — green tests while the behavior is broken. `tools/list_changed` dispatch: the subscription is registered with a bare string rather than an SDK schema value. The name itself is compile-checked (the parameter is a `NotificationMethod` literal union — a typo produces TS2345, verified), but nothing proved an inbound notification reaches the `setToolListChangedHandler` callback. Since `autoRefreshOnListChanged` defaults to on, a broken dispatch path means refreshes silently never happen. Three tests now drive the real `makeClient` over InMemoryTransport in both eras; each fails if the method name is swapped for a different valid one. `makeClientForTest` is the seam for that, marked `@internal` and deliberately not re-exported from the entrypoint. `clientInfo`: the fake Client discarded its first constructor argument, so nothing checked that the generated version actually reaches the SDK — the thing version.ts exists to protect. Now asserted for the default, the caller override, and the SSE fallback client. `{ pin: string }`: accepted any string, so a mistyped revision only failed at connect time. Now a literal union with a `(string & {})` arm, so the two known revisions typo-check while a future one still compiles without a cast. Exported as `MCPProtocolRevision` since `MCPProtocolNegotiation` is public. * fix(mcp): make failed-connect teardown and stale re-list failures honest Two follow-ups from Devin's review of a50c568. The failed-client releases used `close().catch(() => {})`, which only intercepts a rejected promise. A synchronous throw from `close()` would escape and replace the original connection error with a teardown error — worst on the path where the real diagnosis matters most. All four sites now go through `releaseFailedClient`, which wraps the call in a `try`. Two tests cover it: the connect error still surfaces, and the SSE fallback still runs, when `close()` throws synchronously. The stale re-list added in 2515977 turned a previously-survivable replay into a hard failure, reported as "Failed to rehydrate MCP connection from snapshot" — misleading, since the rehydrate succeeded and it was the refresh that failed. Failing is still correct: serving tools older than the caller's own `maxAgeMs` is the bug that check exists to prevent. But it now throws `MCPStaleSnapshotError` naming staleness, so a caller who would rather have stale tools than an exception can catch that specifically. It subclasses `MCPCacheError`, leaving existing catch sites working, and the outer handler rethrows it as-is rather than re-wrapping and double-closing. Devin also raised a session-terminating DELETE on close. Not applicable: that is `terminateSession()`, a separate method; `close()` only aborts its controller and fires `onclose`. * refactor(mcp): keep rehydrateMCPTools under the complexity ceiling The nested try/catch for the stale re-list pushed `rehydrateMCPTools` to cc=17, over the gate's ceiling of 15 — structural-gate caught it on a50c568. Two extractions, no behavior change: - `refreshStaleReplay` takes the stale re-list and its error handling. - `toReplayConnectOptions` takes the six optional connect spreads, which were most of the remaining branch count. Named to distinguish it from `toCreateOptions`, which builds the fallback options rather than the replay's. Verified against main's baseline (CI gates the merge ref, so that is the comparison that counts): `✓ No degradation detected`, complex functions back to 9. The branch's own baseline.json is untouched. * fix(mcp): route every teardown-on-failure through one guarded close I fixed the synchronous-throw hazard in `connect()` last commit and left the same bare `.catch()` at three other sites — Devin caught the inconsistency. `rehydrate.ts:329` was the consequential one: the throw happens inside `catch (err)`, so it replaced the real failure *and* skipped the `freshConnect` fallback two lines below. A rehydrate that used to self-heal would start rejecting because its teardown misbehaved. `closeQuietly` now backs all four sites (both `connect()` failure paths, `freshConnect`, `refreshStaleReplay`, and the rehydrate catch), so the guarantee holds uniformly instead of depending on each caller remembering that `x.close().catch()` misses a synchronous throw. Not exported from the entrypoint — callers close through the handle, which reports failures. Two tests: the `freshConnect` fallback still runs, and the original error still surfaces, when `close()` throws synchronously. Reverting `closeQuietly` to the bare `.catch()` fails all four sync-throw tests. Also answers Devin's SDK-response-cache question with a test rather than a claim. Eviction lives in the base `_onnotification` dispatcher keyed off the method name (`notifications/tools/list_changed` → evict `tools/list`), so it fires regardless of `ClientOptions.listChanged` — which we opt out of. The new case gives the fake server a non-zero `ttlMs` (with the default 0 a cache bug is invisible), proves the second `listTools()` is served from cache, then proves the notification puts the third back on the wire. Without it, `autoRefreshOnListChanged` could silently no-op after a version bump. * fix(mcp): force fresh tools/list reads and stop replaying sessionId Two v1 → v2 migration hazards Devin found, both silent. `refresh()` documents a forced re-read but went through the SDK's default `cacheMode: 'use'`, and v2 added a per-client response cache honouring the server's `ttlMs` on `tools/list` (24h ceiling). Inside that window a refresh returned the cached list, so an app calling it to pick up newly added server tools could keep the old set. v1 had no response cache, so this was a regression introduced by the migration rather than a pre-existing gap. Every internal `tools/list` now sends `cacheMode: 'refresh'` — not `'bypass'`, which would skip the write and leave a stale entry for later `'use'` readers. The `tools/list_changed` path was already safe (the SDK evicts in its notification dispatcher, covered by a test last commit); this closes the consumer-initiated path, which has no notification to trigger eviction. Devin also asked whether the SDK ignores or rejects a `sessionId` in the modern era. It ignores it — and worse than either option: when a transport reports a sessionId, `_connectNegotiated` returns early with no probe and no `initialize`, leaving `getProtocolEra()`, `getServerCapabilities()` and `getServerVersion()` all undefined, without erroring. `serverHasResources()` reads those capabilities, so replaying a snapshot's sessionId silently dropped resource tools. Verified against the real SDK over InMemoryTransport. The replay no longer forwards it; sessions are removed in 2026-07-28 anyway (SEP-2567), so a fresh handshake is both correct and one round trip on a path that already opens a connection. Both tests mutation-verified: restoring `cacheMode: 'use'` and re-adding the sessionId forwarding each fail exactly their own case. * fix(mcp): re-read resource listings, keep TTL for resource contents Devin asked for a deliberate decision on the synthetic resource tools after `listToolDefs` got `cacheMode: 'refresh'`. The answer differs per verb, so this makes the asymmetry explicit in code and pins both halves with tests. `list_resources` now sends `'refresh'` on both `resources/list` and `resources/templates/list`. A listing's whole purpose is to report what exists *now*: a model that creates a resource via a tool call and immediately lists must see it, and being handed a cached listing looks to the model like the write silently failed. The SDK does evict `resources/list` on `notifications/resources/list_changed`, but that only helps servers which both advertise and send it — one with a non-trivial `ttlMs` and no notification would serve a stale listing for the full TTL. `read_resource` deliberately keeps the default. Contents can be large, and a server-declared `ttlMs` on `resources/read` is a considered statement that the bytes are reusable for that long; overriding it would turn every model read into a fetch and discard caching the spec intends. Freshness there is already per-URI — the SDK evicts on `notifications/resources/updated`. The local `RequestOptions` type now carries `cacheMode` so `collectPages` preserves the disposition on every page rather than only the first. Mutation-verified: dropping the list `cacheMode` fails two of the three new tests, and the read-side assertion is a negative so it catches the reverse drift too. * fix(mcp)!: stop persisting sessionId to cache snapshots Reversing my own call from earlier in this review. I argued the persisted `sessionId` was "inert data rather than a trap" and should stay for snapshot-format stability; Devin's security framing is the correct one and mine was wrong. An `Mcp-Session-Id` is bearer-equivalent to an authenticated server session. Once the replay path stopped forwarding it (74e722d), the value became write-only — so it was being persisted to whatever store the caller configured (Redis, a database, a file) with no functionality in exchange. That it sat behind `cacheCredentials` at all is the tell: it was always treated as credential-grade. Removed from `SerializeInput` entirely rather than just skipping the write, so it cannot be reintroduced by a future edit that looks like plumbing. `SerializedMCPServer.sessionId` stays on the type — a validator that rejected it would turn every existing cache entry into a hard failure — with its doc rewritten to say it is never written or read and any value found is untrusted legacy data. Two existing cache tests asserted the old behavior; replaced with one that pins the new guarantee, including `Object.hasOwn` so the key is absent rather than merely undefined. Also covers Devin's probe-timeout question. `makeClient` relies on the SDK default, which is bounded: `negotiateEra` resolves `probe.timeoutMs ?? defaultTimeoutMs`, filled from `options?.timeout ?? DEFAULT_REQUEST_TIMEOUT_MSEC` (60s). A gateway that black-holes `server/discover` rejects rather than hanging. Test drives a production-shaped client against a server that accepts the probe and never answers, proving the timeout parameter reaches the probe — the era tests above use their own client with an explicit bound, so the default was untested on the path every connect now takes. * docs(mcp): add the required examples for the new exports; guard expires_in Two follow-ups from Devin's review of bdbd426. The repo rule in .agents/skills/public-api-examples/SKILL.md requires every new export to appear in a compilable example in the changeset, the PR body, and the README section that documents it. I added `MCPStaleSnapshotError` and `MCPProtocolRevision` with none of the three, while the prose told consumers to catch the error — migration guidance with nothing to copy. All three now carry an example, and the PR body gained the `### API example` section it never had. Verified the snippet compiles against the real API rather than eyeballing it. The README's caching section also now states that `rehydrateMCPTools()` enforces `staleness.maxAgeMs` and can reject, that `refresh()` bypasses the SDK response cache, and that session ids are never persisted. Its verification numbers were stale too (590 tests → 619). Devin also asked whether SDK v2's `StoredOAuthTokens` still carries a relative `expires_in`, since `tokensFromProvider` narrows it with a runtime `typeof` check that degrades silently: an absolute-expiry field would simply never match, snapshots would carry no `expiresAt`, and `tokensExpired()` would treat expired credentials as usable forever. It does still carry it (verified by compiling against the type). Added a type-level assertion so that stops being a fact someone has to re-verify by hand — a future SDK bump that changes the shape fails `tsc` instead of silently disabling expiry checks. * feat(mcp)!: degrade to the 2025 handshake when the probe is refused; ship 1.0.0 Both open decisions resolved by @LukasParke: major release, and option 2 for the `'auto'` default. `'auto'` is now strictly additive. When the caller leaves `protocolNegotiation` unset, a failed connect is retried once with `'legacy'`, so a proxy, WAF, or gateway that hangs or 5xx's on an unknown method connects exactly as it did before this package started probing. Modern servers still get 2026-07-28. The cost is one extra attempt on a path that was already failing, which is where latency matters least. That also closes the pinned-SSE finding without separate work: all three `makeClient` sites — pinned SSE, Streamable HTTP, and the SSE fallback — sit under the retry, so the two-transport fallback stops being a single point of failure against probe-hostile infrastructure. The retry fires on any failure rather than only on probe-shaped errors. Inspecting the cause would save a wasted attempt against a genuinely dead server, but it would couple this to SDK error codes, and a reshaped error would silently disable the degradation — the exact failure mode this review kept surfacing. An explicit `protocolNegotiation` is honoured verbatim, including `'auto'`: naming a mode means accepting its failures, and silently overriding a `{ pin }` would defeat pinning. So `'legacy'` becomes a performance choice rather than a compatibility one, and the docs now say so. `connect()` splits into a public wrapper owning the retry policy and `connectWithNegotiation` performing exactly one mode, which also keeps it under the complexity ceiling. Seven tests, mutation-verified both directions: removing the retry fails 4, and removing the explicit-mode guard fails 6 transport tests that would otherwise silently double their attempt counts. The `probeHostile` fake rejects any probing mode and accepts `'legacy'`, so a test cannot pass unless the retry really switched modes. Changeset is `major` — `1.0.0` (verified: semver.inc('0.0.1','major')). The `'auto'` Breaking note is gone, since it is no longer a break; the OAuth provider change remains the one breaking item. * fix(mcp): bound the legacy retry and skip it on auth failures Devin caught two real problems with the degradation I added in 7fc4ce5, and my own test was the evidence for the first. **Attempt amplification.** `connectWithNegotiation` already walks Streamable HTTP then SSE when no transport is pinned, so retrying the whole thing meant up to four `connect()` calls against an unreachable server where it used to be two — each with its own request timeout, enough to push a caller past its own deadline. `expect(state.clientsCreated).toBe(4)` was sitting right there asserting the amplification I had just documented as "one extra attempt". The retry now pins the caller's transport preference, so it is a single attempt: a probe-hostile server takes two attempts, a dead one three. **Replayed side effects.** Retrying on any failure meant an `UnauthorizedError` was retried too — re-driving an OAuth provider's authorization flow, so a second `redirectToAuthorization` and a second PKCE verifier overwriting the first. Credentials being rejected is not something a different protocol revision fixes, so the retry is now skipped there. This is the one place the error's identity is inspected; the retry still fires on any other failure rather than trying to recognise probe errors, since that would couple us to SDK error codes. `isAuthFailure` walks the `cause` chain, because `connectWithNegotiation` wraps transport errors — a top-level `instanceof` would miss the nested case and retry anyway. Depth-capped so a cyclic cause cannot hang. Corrected the counts in the changeset and README, which both understated the cost. Four tests, mutation-verified: removing the transport pin fails two, removing the auth skip fails one, and one asserts the nesting that makes the chain walk necessary. * fix(mcp): let the retry re-walk the ladder; see auth failures from any attempt Two more from Devin on the bounded retry — the second is a regression I introduced one commit ago while fixing the first review round. **The transport pin broke SSE-only legacy servers.** I pinned Streamable HTTP to cap the attempt count. But a legacy server reachable only over SSE, behind probe-hostile infrastructure, fails both transports under `'auto'` — and with the retry pinned to HTTP it never gets offered SSE again. A server that connected before this PR stopped connecting, which is precisely the regression the degradation exists to prevent. The retry re-walks the ladder again. That puts a genuinely dead server back at four dials, two per mode. I had traded correctness for an attempt count without noticing, so the count is now asserted as a deliberate decision instead: what matters is that it is a fixed multiple, not a retry loop. **The auth guard only saw the last failure.** On the two-transport path `connectWithNegotiation` discarded `httpErr` entirely — the fallback wrapped only `sseErr`. So an `UnauthorizedError` from Streamable HTTP followed by an unrelated SSE failure (the same URL answering 404 to an SSE GET, never reaching the auth path) left no auth error in the chain, and the retry re-drove the OAuth flow anyway. Exactly the side effect the guard was written to prevent, one commit after writing it. `MCPConnectionError` now carries every underlying failure on `errors`, named after `AggregateError` rather than inventing a field, and `isAuthFailure` searches that tree instead of the `cause` spine. Corrected the attempt counts in the changeset and README — the "three attempts" claim I wrote last commit was wrong the moment the pin came off — and added the compilable example the repo's public-API rule requires for the new `errors` field. Mutation-verified: re-pinning the transport fails 3 tests, and narrowing `isAuthFailure` back to the cause spine fails the cross-transport one. * fix(mcp): aggregate both negotiation passes; widen the auth guard to 401/403 Two more from Devin, both on the retry I added two commits ago. **`errors` was a partial record.** When the legacy retry also failed, its rejection propagated untouched and the `'auto'` pass's failures vanished — half the attempts gone, including any auth-shaped rejection `isAuthFailure` had not matched. That directly contradicted the contract I had just written on `MCPConnectionError.errors` ("every failure behind this one, in attempt order"), so the docs promised a complete record while the code delivered half of one. `connect()` now wraps a failed retry and concatenates both passes, flattening nested `errors` so callers iterate a list of real attempts rather than a tree of wrappers. **The auth guard only recognised `UnauthorizedError`.** Devin asked me to check the SDK's taxonomy, and the answer is that it is not one type: the version-negotiation probe does not route 401/403 through the OAuth flow — `classifyHttpError` turns them into `SdkHttpError` with `ClientHttpAuthentication` / `ClientHttpForbidden`. So a probe rejected for auth reasons was retried, re-driving the flow the guard exists to prevent, on the most common auth path of all. Now also matches a duck-typed `status` of 401 or 403. Reading the numeric status rather than `instanceof SdkHttpError` plus an `SdkErrorCode` comparison: the status is the stable half of that contract, and it additionally catches a gateway surfacing 401/403 in some other error shape. Three tests, each mutation-verified: dropping the aggregation fails the attempt-count one; removing the status check fails the 403 one; widening the status check to any number fails the 404 one, which exists so the guard cannot over-match and silently disable the degradation. * fix(mcp): bound the probe at 5s; unwrap wrappers; find auth in AggregateError Three from Devin's review of fb89f92. **The probe could hang for minutes.** `makeClient` left `probe.timeoutMs` unset, so the SDK gave the probe the full 60s request timeout. Under `'auto'` the probe is the first request of every connection, and with the legacy retry re-walking the ladder that is four attempts — ~4 minutes before `createMCPTools()` rejects, on the path a caller gets with no configuration. Now bounded at 5s, with `probeTimeoutMs` to raise it for a slow server. A probe that misses the window isn't lost: it surfaces as a failure the legacy retry handles, the same path a refusal takes. This is the option I declined two rounds ago as premature pending the `'auto'` decision. That decision is made, and Devin's arithmetic showed the exposure is worse than I'd assumed — so it lands now. **`errors` could contain wrappers.** `flattenAttempts` only unwrapped when `errors` was non-empty, but a single-transport pass wraps its one failure with only `cause` set. On the pinned-HTTP path the aggregate became two opaque `MCPConnectionError`s, so a caller scanning for a rejected token had to know to dig through `cause` on some entries and not others — and the field's own doc promised real attempts. **`isAuthFailure` ignored `AggregateError` members.** Node's happy-eyeballs path reports a 401 as an `AggregateError` member rather than a `cause`, so a spine-only walk missed it and re-drove the OAuth flow. Now reads any array `errors`, which covers ours and `AggregateError` alike. All three mutation-verified. The unwrap test initially passed against its own mutation because I wrote it on the pinned-SSE path, which rethrows raw and never had the bug; retargeted to pinned HTTP, where it fails without the fix. * fix(mcp): raise the probe ceiling to 30s; wrap pinned-SSE failures Two from Devin on 57f29e9. The first is a correctness bug I introduced one commit ago, and my own justifying comment was the wrong reasoning. **5s could make modern-only servers unreachable.** I argued the tight ceiling was safe because "a probe that misses the window is not lost: the legacy retry handles it". That holds for 2025-era servers only. On HTTP the SDK classifies a probe timeout as an outage and rejects (`classifyProbeOutcome` returns a legacy verdict for stdio alone), and the legacy retry then sends `initialize` — removed in revision 2026-07-28 (SEP-2575). So a modern-only server slower than the ceiling fails *both* passes and does not connect, where the SDK's own default would have waited and succeeded. Serverless cold starts routinely exceed a few seconds, which makes that a correctness bug rather than the latency tradeoff I described. 30s instead: comfortably past a cold start, and still caps the black-holed-gateway case at roughly half the SDK default rather than four times it. The JSDoc now states that a probe timeout is unrecoverable instead of claiming the opposite. **Pinned SSE rethrew raw while every other path wrapped.** Pre-existing, but the new `connect()` wrapper made it asymmetric: with `protocolNegotiation` unset the outer retry aggregated it into an `MCPConnectionError`; with it set the transport error escaped. Same server, same failure, different `catch`. Now wrapped like everything else. Also fixed a test that had started taking 10s in real time: it asserted the probe bound via `connect`'s `timeout`, which our explicit `probe.timeoutMs` now overrides, so it was waiting out the real default. Uses `probeTimeoutMs`. Both mutation-verified: restoring 5s fails the ceiling test, restoring the raw rethrow fails the wrapping test. * fix(mcp): correct the probe-default docs and pin them; narrow isAuthStatus Two from Devin on 961bf65. **Three JSDoc sites still said 5000** after I raised the constant to 30_000 — types.ts, rehydrate.ts, and mcp-connection.ts — while the README, changeset, and tests all said 30s. Anyone reading the IDE hover would have budgeted six times less than the client actually waits, which is the worst kind of doc bug: confidently wrong rather than absent. Fixed, and added a test that pins the JSDoc against the value the code passes, because a number duplicated across four files has now drifted once and will again. It greps the source for `defaults to <n>` near the probe option and compares against what the fake Client observed. Verified it catches the exact drift Devin found. **`isAuthStatus` now requires an `Error`.** Devin asked whether any SDK shape attaches a non-authoritative `status`. Audited: only `SdkHttpError` exposes one, and its value is the real response status — but the SDK does build log records with `status: 0`, and a plain object riding in a `cause` is likelier to be a response or payload than a rejection. Over-matching there would silently suppress the retry and make a probe-hostile-but-authenticated server unreachable, which is the regression the retry exists to prevent, so the guard is worth narrowing even though nothing in v2 trips it today. Both mutation-verified: reintroducing the 5000 doc fails the drift test, loosening the `Error` check fails the payload test. * fix(mcp): make cache writes best-effort; stop auth falling through the ladder Two from Devin on ea7474b. **A store outage failed the whole call.** `refresh()` re-lists and then writes back, so both steps surfaced identically — and the stale path converted either into `MCPStaleSnapshotError`, discarding a connection whose tools had just been read successfully and reporting it as a re-list failure. Worse, the recovery this PR documents rehydrates through the same store, so it failed too: the escape hatch was as broken as the thing it escaped. `writeCache` now tags failures as `MCPCacheWriteError` (subclassing `MCPCacheError`), the stale path treats that as survivable, and the construction-time write is best-effort. A handle is fully usable without its cache entry; the next rehydrate re-reads it. Callers who want a store outage to be fatal can catch the new type. **Auth failures fell through the transport ladder.** A 401 on Streamable HTTP tried SSE with the same `authProvider`, re-entering the SDK's auth path for a second `redirectToAuthorization` and overwriting the saved PKCE verifier. This is the same duplicated side effect `connect()`'s retry guard was written to prevent — I guarded the outer layer and left it intact one level down, then wrote a test asserting both transports are tried on an auth failure without noticing that was the hazard. Three tests repointed: they used a failing cache write as their post-connect failure trigger, which is no longer fatal. They now use a snapshot with duplicate tool names, which `buildTools` genuinely rejects. All three changes mutation-verified. * fix(mcp): guard the rehydrate fallback against auth failures; document new API Findings from a full self-review of the final state, requested after 14 incremental review rounds. **The freshConnect fallback was a third unguarded reconnect layer.** The same duplicated-OAuth hazard was fixed tonight at two layers — connect()'s legacy retry skips auth failures, and the transport ladder short-circuits on them — but rehydrateMCPTools' catch still routed *any* replay failure into freshConnect with the same auth. A credential rejection on the replay therefore re-drove the OAuth flow one layer up from both guards. The fallback now consults the same isAuthFailure, exported (not via the package entrypoint) so all three layers share one definition and a future widening applies everywhere. **MCPCacheWriteError and probeTimeoutMs shipped without the examples the repo requires.** Same public-api-examples violation as MCPStaleSnapshotError earlier tonight — new export and new public option, present in prose, absent from every ts block in the changeset and from the PR body. Both now have compile-verified examples in both places. Reviewed and found sound, for the record: the connect flow's three layers compose without double-teardown (closeQuietly is idempotent and each layer closes only clients it created); flattenAttempts covers all four wrap shapes; probeTimeoutMs threads through create/rehydrate/replay/freshConnect identically to protocolNegotiation; the untouched modules (tool-wrapper, elicitation, auth-resolver, result-mapper) check out — the progress-pump generator in tool-wrapper cannot drop a trailing event because finalize flips `done` only after the queue drain re-runs. * fix(mcp): only treat 401/403 as auth failure when OAuth is configured Devin found the guard I widened two rounds ago cancelling the recovery in exactly the scenario the recovery exists for. Proxies and WAFs commonly answer an unknown method like `server/discover` with 403 — not just 5xx or a hang — and `isAuthStatus` classified any 401/403 as a credential rejection. That suppressed both the SSE fallback and the legacy retry, so a server behind such a gateway failed outright where it connected before this PR. The duplicated-side-effect rationale only holds for OAuth: a second `redirectToAuthorization`, an overwritten PKCE verifier. With bearer, headers, or no auth, a retry merely re-sends a request. So the duck-typed status check is now consulted only when the caller configured `auth: { kind: 'oauth' }`, threading the auth kind through `isAuthFailure` at all three guard sites. `UnauthorizedError` stays unconditional — the SDK only throws it from the provider-wrapped fetch and the authorization flow, so it inherently means a provider is in play. Two new tests pin the un-suppressed cases (no auth, bearer) and the existing suppression tests now configure OAuth. Mutation-verified: un-scoping the status check fails both new tests. * test(mcp): make the doc-drift guard actually cover mcp-connection.ts Devin caught the guard passing vacuously. The regex required the digits almost immediately after "efaults to", but mcp-connection.ts writes the default as "Defaults to\n * `DEFAULT_PROBE_TIMEOUT_MS` (30000)" — symbol name, newline, and parens in between — so the file produced zero matches and the for-loop passed on emptiness. The test's stated purpose was to catch the exact drift that had already happened, and it could not have caught it in one of the three files it named. Two changes: the pattern now tolerates up to 80 non-digit characters (including newlines) between the phrase and the number, and each file must produce at least one match — zero means the regex drifted from the prose, not that the file went quiet. Mutation-verified in the file that was previously uncovered: setting its doc back to 5000 now fails the test. * fix(mcp): errors on MCPConnectionError is never empty Devin: the auth short-circuit rethrows the first pass's error untouched, and a single-transport pass builds that error with only `cause` set — so `errors` was `[]` while the docs promised "every underlying failure, flat and in attempt order". A caller iterating `errors` without also inspecting `cause` saw nothing in precisely the auth case, where the rejection is the one thing worth finding. Fixed in the constructor rather than at the rethrow site: `errors` now defaults to `[cause]` when no explicit list is given, so every construction site gets the uniform contract and `flattenAttempts`' unwrap arm becomes a formality rather than load-bearing. The doc's "Empty when only one transport was attempted" claim is gone — it described the bug as if it were the design. Changeset example comment corrected to match. Test pins the exact shape Devin described: an auth failure on the implicit default yields errors of length 1 containing the UnauthorizedError. Mutation-verified — reverting the constructor default fails it. * fix(mcp): store outages never gate notification or fail reads Two from Devin on ad8ea0a, both consequences of MCPCacheWriteError now being a distinct, throwing type. **A failed write silenced tool-change subscribers.** `refresh()` adopts the new tools before it writes the snapshot back, so when the write rejected, the auto-refresh handler's `.then()` never ran and listeners were never told — while `handle.tools` already returned the new set. Subscribers stayed permanently out of sync, and the handler's own comment ("listeners keep the last good tool set") had become false: the set was already swapped. The handler now converts MCPCacheWriteError into a notification with the current tools; a failed re-list stays silent, which is correct because nothing was swapped. **A failed read was fatal while writes were best-effort.** `store.get` was awaited unguarded in tryCacheHit, so a store blip on the lookup rejected `createMCPTools()` when a plain miss would have connected fine — meaning the release note's "a store outage leaves you with a working handle" only held if the outage arrived after the read. A failing read is now a miss. Both mutation-verified: re-gating notification on the write fails the subscriber test; making the read fatal again fails the miss test. * docs(mcp): reconnectOnExpiry JSDoc matches its changed semantics Devin: two behaviors of `reconnectOnExpiry` changed in this PR and neither JSDoc site was updated — the hover text still promised "transparently fall back on any connection failure". 1. An auth failure now bypasses the fallback (the fallback reuses the same auth, so retrying cannot succeed and would re-drive an OAuth flow); those reject with MCPCacheError instead. 2. An over-age snapshot (staleness.maxAgeMs) is now a fallback trigger, which the option's one-line doc never mentioned. Both the field JSDoc and the rehydrateMCPTools block now state the full trigger list and the auth exception, per the repo's changed-option-semantics rule in public-api-examples/SKILL.md. * fix(mcp): point the loopKey test at the new SDK package loop-key.test.ts arrived from main (#74) importing @modelcontextprotocol/sdk/client/index.js, which this branch removed from packages/mcp dependencies in favour of @modelcontextprotocol/client. It resolved locally only because the old package is still present transitively; a clean CI install would fail typecheck and lint, both of which compile tests/** per turbo.json. The last site in the migration — nothing else under src/ or tests/ still names the old package. * fix(mcp): thread signal through connect; skip the no-op replay write Cortex's re-review (three findings on the merged-up head). **Callers could not bound the connect ladder.** ConnectOptions had no signal and freshConnect did not forward options.signal into connect(), so a caller with a request deadline had no way to abort — and the JSDoc's "roughly half the SDK default" undersold the worst case: only the 'auto' probes are capped at 30s; the legacy retry's initialize attempts run under the SDK's 60s, so a black-holing gateway costs ~30+30+60+60 ≈ 3 minutes on the default path (vs ~2 minutes pre-PR). signal now threads from createMCPTools/rehydrate through every client.connect (the SDK's ConnectOptions extends RequestOptions, which accepts one), an aborted caller is never retried under 'legacy', and the JSDoc states the real arithmetic. **Every replay performed a write that is a no-op.** With replayedCachedAt carried forward, the snapshot written on construction is the one just read — same tool defs, same cachedAt — so the warm path paid one external store round-trip per rehydrate for nothing. The construction write is now skipped on replays; cold connects still write, and refresh() clears replayedCachedAt and writes. This also narrows DEV-766's surface: one fewer path that can rewrite a credentialed entry under different options. Cortex's third finding (list_resources bypassing the response cache per page) is acknowledged on the thread rather than changed — the freshness rationale was argued and accepted earlier; an opt-out remains available if field usage shows the round trips matter. All mutation-verified: dropping the signal forward, retrying despite abort, and restoring the replay write each fail exactly their own test. * fix(mcp): declare Node 20; document signal/no-seed semantics; pin legacy+probe Perry and Devin's requested re-review of 4a2e295, plus cortex. **engines: node >=20** (cortex major, Perry suggestion, independently). The v2 client declares it; without our own engines field a Node 18 consumer installs with at most a transitive warning and fails at load time. Declared, and listed as a Breaking bullet in the changeset since it is a runtime prerequisite the 0.0.1 package did not have. **signal docs were stale** (Devin). The option's JSDoc and README row still said "every tool call" after yesterday's change threaded it into connecting, the probe, the legacy retry, and reconnects — an undocumented new way for a connection to be cut short. Both surfaces now state the full scope. **Replay-no-write is now documented at the option** (Devin). The skip is correct for the cache-hit route but means a direct rehydrate never seeds a caller-supplied store until refresh(); the cache option's JSDoc and the README now say exactly that and how to seed deliberately. **legacy+probe pinned against the real SDK** (Devin). Both the implicit retry and explicit 'legacy' construct a probe block alongside a non-probing mode; a real-constructor test now proves the SDK tolerates that shape, so an SDK that starts validating the combination fails a test instead of production connects. **Test doubles return null, not undefined** (Devin) — matches MCPCacheStore.get's declared type rather than relying on widening. * fix(mcp): keep OAuth tokens rotating and scrub legacy sessionIds on replay Devin found two interactions of yesterday's replay-write skip, plus asked for an SDK audit; all three from the requested re-review round. **OAuth token staleness.** The per-hit write-back was also the only place a warm cache hit re-serialized the OAuth provider's current tokens. Without it, the stored entry keeps cold-connect-time tokens, and once those pass expiresAt every rehydrate detours through freshConnect even though the provider holds a valid refreshed token — self-healing, but a permanent latency tax after first expiry. A replay with an OAuth provider and cacheCredentials: true now re-persists (best-effort). **Legacy sessionId scrub.** Stopping the field being written composed with never rewriting on the warm path into: entries from earlier versions keep their bearer-equivalent Mcp-Session-Id in the external store indefinitely. A replay that reads such an entry now writes it back once, copied minus the field. Copied, not re-serialized — a re-serialize under this call's options would strip a credentialed entry when cacheCredentials was not repeated, which is exactly DEV-766. **UnauthorizedError audit** (Devin's third question): every construction site in @modelcontextprotocol/client@2.0.0 is inside a `this._authProvider`-gated branch, the withOAuth middleware, or the authorization flow itself — the plain-401-no-provider path throws nothing of the sort, so treating it as unconditional in isAuthFailure cannot suppress the degradation for bearer/headers/no-auth callers. Documented on the thread with the site list. Scrub mutation-verified; token-rotation write covered by the OAuth branch of maintainReplayedEntry. * fix(mcp): scrub only store-held bytes, never after a stale refresh Devin found two real defects in yesterday's maintainReplayedEntry, one round after I added it, plus a TTL semantics question. **The scrub could clobber a fresh refresh.** On the over-age + reconnectOnExpiry: false path, refresh() re-lists and writes the fresh entry — then the scrub ran behind it and wrote the caller's older input snapshot back, so the store went straight back to stale and every subsequent load paid the re-list again. The scrub is now skipped entirely when a stale refresh ran: the refresh's own serialize already omits sessionId and carries current tokens, so every maintenance concern is satisfied by construction. **The scrub could introduce credentials.** It wrote a copy of the caller's input snapshot, whose auth block reaches the store even when this call never set cacheCredentials — and a direct rehydrate may have loaded that snapshot from a file or a different store entirely. The scrub now reads the store first and rewrites only what it already holds, minus the field: an empty or sessionId-free entry produces no write at all. Copy-what-you-hold also keeps the DEV-766 posture — it cannot strip credentials either. **TTL-refresh semantics** (Devin's third): documented rather than changed. MCPCacheStore's contract says nothing about TTLs, and re-adding a warm-path write to keep SETEX entries alive would resurrect the no-op round trip cortex flagged. The changeset now tells write-extends-TTL store implementations to size their TTL to the staleness window they want. Both fixes mutation-verified: unskipping the scrub after refresh fails the clobber test; writing the input snapshot without the read-back fails the no-introduction test. * fix(mcp): scrub read-back only when the input snapshot carries a sessionId Devin: the read-back I added last round to keep the scrub from introducing credentials meant every warm cache hit read the store twice — tryCacheHit reads the entry, then maintainReplayedEntry read the same key again just to learn it had no sessionId, which is true of every entry this version writes. Double read load on Redis/DB for the common case, and the changeset's "warm hits no longer touch the store" claim was false the round after I wrote it. Cheap gate first: the scrub can only be needed when the INPUT snapshot carries a legacy sessionId. On the warm path the input IS the store's entry, so a sessionId-free input proves a sessionId-free store and we return before any store call. Only legacy entries pay the read-back, which still guards the introduce-credentials case for direct rehydrates whose snapshot came from elsewhere. Test pins the invariant directly: a modern warm replay performs zero store operations — gets and sets both counted. Mutation-verified. * fix(mcp): 403 degrades even under OAuth; abort stops the in-pass ladder Devin, two threads on f99c70b. **The 403 suppression was wrong for OAuth users too.** Devin re-raised the gateway-403 scenario, now scoped to OAuth — and on a second SDK audit the right cut is by status, not by auth kind. The PKCE side effects the guard protects (saveCodeVerifier, redirectToAuthorization) live exclusively behind the SDK transport's `status === 401 && authProvider` branch; a 403 never enters the OAuth flow at all, so a retry after a 403 re-drives nothing. But WAFs commonly answer unknown methods like server/discover with 403 — so suppressing on it made OAuth deployments behind such gateways permanently unreachable, in exchange for preventing a side effect that cannot occur. isAuthStatus now matches 401 only. The step-up-exhaustion 403 the SDK can throw is a genuine authz failure that will simply fail again under 'legacy' — one wasted retry there buys connectivity everywhere else. **Abort now stops t…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #73 (
feat/doom-loop-detector). Closes the review finding that MCP tools had no way to declare a doom-looploopKey— the entire MCP surface was stuck on whole-arguments identity.What's added
Two declaration channels, both landing on
tool.function.loopKeyviamarkMcp's injection point (shipped in #73):loopKeysoncreateMCPTools/rehydrateMCPTools, keyed by the tool's unprefixed MCP name. Accepts anyToolLoopKeyform:_meta['openrouter/loopKey']on the MCP tool definition: a field-name array orfalse. Data-only by design (functions can't cross the wire); unknown shapes ignored. Captured inlistToolDefs, carried onMcpToolDef.loopKey, and round-tripped through cache snapshots (SerializedMCPToolDef.loopKey), so rehydrated tool sets keep their identities without alistTools()round-trip.Client config takes precedence over the server declaration. Both wrap branches (regular +
emitProgressgenerator) get the same wiring.Tests
10 new tests (
tests/unit/loop-key.test.ts): all three client-config forms, unprefixed-name keying, server-advertised pickup, client-over-server precedence, generator-tool coverage, absent-by-default,buildToolsplumbing, and cache serialize → JSON round-trip for both field-list andfalsedeclarations.Verification
pnpm test(mcp): 10 files, 49 passed (10 new)pnpm typecheck,pnpm lint: cleanSurface changes
CreateMCPToolsOptions.loopKeys/RehydrateMCPToolsOptions.loopKeysMcpToolDef.loopKey,WrapToolOptions.loopKeys,BuildToolsOptions.loopKeysSerializedMCPToolDef.loopKey(additive within snapshot version 1)@openrouter/mcp)