Do not resend a non-idempotent request after a session-expiry 404 - #209
Merged
Conversation
2.1.0 made tools/call non-retryable (#196) because a "transient" failure can arrive after the server executed the request. Session recovery was a second, independent path around that guarantee: any session-bearing HTTP 404 calls restart_session_and_resend, which re-sent the original request without consulting NON_IDEMPOTENT_METHODS. A tools/call whose session expired therefore produced two identical POSTs. A 404 usually does mean the request was rejected outright, but it does not prove it — a session can expire after the tool ran, which is exactly the ambiguity the no-replay rule exists for. The session is still restarted, so recovery keeps working for idempotent requests. A non-idempotent request is not re-sent; it raises ConnectionError, which with_retry never rescues, so no other path can turn it into a second attempt. Both call sites route through the new helper, including the one that skips re-initialization when another caller already restarted the session. Found by Codex adversarial review of the 2.1.0 release PR (#208) and reproduced before fixing: two tools/call POSTs before, one after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
simonx1
added a commit
that referenced
this pull request
Aug 4, 2026
Version bump, changelog, and the documentation the release needs. - lib/mcp_client/version.rb and the Gemfile.lock self-reference move to 2.1.0; the gemspec reads the constant, so it needs no edit. - CHANGELOG documents the PRs merged since 2.0.0 (#188-#210) as one coherent story, with the behavior changes hosts must act on called out under Breaking Changes and Migration notes: tools/call is no longer auto-retried (including through session recovery), task operations refuse to guess a server, peer-facing error messages are constant, and logs no longer carry payloads. - README gains a "Treating the Server as Untrusted" section summarizing what the transports now refuse from a peer, documents the retry semantics for tools/call, and documents max_decompressed_body_bytes. Two claims are deliberately narrow: the OAuth check is textual (DNS is not resolved) and the response-size limit is Streamable HTTP only, since that is the only transport requesting gzip. Two defects found while writing the docs, both fixed here: - max_decompressed_body_bytes could not actually be set through the documented path. streamable_http_config did not accept it and ServerFactory did not forward it, so the option only worked when constructing ServerStreamableHTTP directly. Wired through both, with specs asserting the config path, the default, and the validation. - The README claimed "No runtime dependencies" while the gemspec declares faraday, faraday-follow_redirects, faraday-retry and base64. Metrics/ParameterLists now sets CountKeywordArgs: false. The *_config builders are keyword-only option factories, and the codebase already carried six hand-written disables for exactly this; those are removed. The two code bugs Codex found while reviewing this release ship separately in #209 and #210. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
simonx1
added a commit
that referenced
this pull request
Aug 4, 2026
## Summary Releases **2.1.0**, covering the PRs merged since 2.0.0 (#188–#210): a security pass over every transport, plus Ruby 4.0.6 support. > **Rebased.** Codex's review of this PR found two real code bugs. Rather than smuggle them into a release commit, they shipped separately as **#209** (`tools/call` replayed by session recovery) and **#210** (peer payloads reaching the SSE error logs), both merged. This branch was rebuilt on top of them, so its diff is now version + docs + the two doc-driven fixes below — no security code changes. The review comment below has the reproductions. The theme is one sentence: **a remote MCP server is untrusted input.** 2.0.0 was correct against a cooperative server but assumed good faith in places where a hostile — or merely compromised — peer controls the data. The wire protocol is unchanged and the ordinary client API is unchanged; what changed is what the client accepts, retries, logs and reflects back. ## What's in the release | | | |---|---| | Version | `lib/mcp_client/version.rb`, `Gemfile.lock` self-reference → **2.1.0** (gemspec reads the constant) | | CHANGELOG | New 2.1.0 entry in the established format, with **Breaking Changes** and **Migration notes** | | README | New "Treating the Server as Untrusted" section; `tools/call` retry semantics; `max_decompressed_body_bytes` | ### Behaviour changes hosts must act on These are spelled out in the CHANGELOG's Migration notes: - **`tools/call` is never retried automatically** — including through session-expiry recovery (#209). A "transient" failure can arrive after the server executed the request, and JSON-RPC has no idempotency key to make a replay safe. Retry explicitly if your application knows it is safe, and treat the error as *outcome unknown*. - **Task operations refuse to guess a server.** Pass the `Task` handle from `call_tool_as_task`, or name the server. A bare ID still works with one configured server; with several it raises `ArgumentError` rather than acting on the wrong one. - **Peer-facing error messages are constant** and **DEBUG logs no longer contain payloads** — if you parsed either for detail, the detail now lives only in local logs. Error codes are unchanged. - **Cross-origin SSE endpoints/redirects and non-HTTPS or loopback OAuth discovery URLs from a peer are refused.** Local development still works when the *configured* server is itself local. ## Two defects found while writing the docs Writing the documentation turned out to be a review in its own right: 1. **`max_decompressed_body_bytes` could not be set through the documented path.** `streamable_http_config` did not accept it and `ServerFactory` did not forward it, so the option introduced in #188 only worked when constructing `ServerStreamableHTTP` directly — i.e. the knob was effectively unreachable for anyone using the normal config API. Wired through both, with specs asserting the config path, the default, and the positive-integer validation. 2. **The README claimed "No runtime dependencies"** while the gemspec declares `faraday`, `faraday-follow_redirects`, `faraday-retry` and `base64`. Pre-existing, corrected here. ## RuboCop config `Metrics/ParameterLists` now sets `CountKeywordArgs: false`. The `*_config` builders are keyword-only option factories where each keyword is a documented, defaulted setting; counting them against a positional-argument limit would push new options into an opaque hash. The codebase already carried **six hand-written `rubocop:disable Metrics/ParameterLists`** comments for exactly this reason — those are now removed, which is what the 58 autocorrections in the diff are. ## Verification | Check | Result | |---|---| | RSpec, Ruby 4.0.6 (default) | **1698 examples, 0 failures** | | RSpec, Ruby 3.3.5 | **1698 examples, 0 failures** | | RuboCop | 136 files, no offenses | | `gem build` | builds `ruby-mcp-client-2.1.0.gem`; ships lib + LICENSE + README only (no specs/examples/secrets) | | `Gem::Specification#validate` | passes | | Version consistency | `version.rb`, `Gemfile.lock`, gemspec all report 2.1.0 | | Examples suite (pre-release, on merged main) | 18 PASS · 0 FAIL · 2 SKIP | The two example skips are environmental: no `ANTHROPIC_API_KEY` in this environment, and the interactive browser-OAuth example. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
Found by Codex during the adversarial review of the 2.1.0 release PR (#208), reproduced before fixing.
#196 stopped
tools/callbeing replayed bywith_retry, on the grounds that a "transient" failure can arrive after the server executed the request. Session recovery was a second, independent path around that same guarantee. Any session-bearing HTTP 404 callsrestart_session_and_resend, which re-sent the original request without consultingNON_IDEMPOTENT_METHODS:A 404 usually does mean the request was rejected outright — but it does not prove it. A session can expire after the tool ran, which is precisely the ambiguity the no-replay rule exists for. This is also the sharper version of the problem, because unlike a retry it happens even with
retries: 0.Fix
resend_after_session_restartgates the resend on the sameNON_IDEMPOTENT_METHODSconstant:tools/list,resources/read, …).ConnectionError— chosen deliberately becausewith_retrynever rescues that class, so no other path can quietly turn this into a second attempt — with a message saying the request was not resent because it may already have executed.After:
Testing
Two regression tests in
streamable_session_lifecycle_spec.rb:tools/callhitting an expired session is attempted exactly once, raises, and the session is restarted (asserting we didn't fix this by disabling recovery);tools/listis still re-sent against the fresh session.Full suite 1695 examples / 0 failures, RuboCop clean.
Note
The behaviour change is user-visible: a tool call interrupted by an expired session now raises instead of transparently recovering. It is documented in the 2.1.0 CHANGELOG and README in #208, which will pick this up once merged.
🤖 Generated with Claude Code