fix(mcp): set session manager on non-OAuth HTTP MCP clients

[nick-stebbings]

Summary

McpClient::new_with_config() creates a client with session_manager: None.
Non-OAuth HTTP MCP servers never get Streamable HTTP session tracking — the
client omits Mcp-Session-Id headers, so servers that maintain per-session
state silently lose it between calls.

The authenticated path (new_authenticated) already wires the session manager.
This fix adds set_session_manager() to all three non-OAuth fallback sites:

• app.rs: startup MCP client initialization (both with/without secrets store)
• cli/mcp.rs: ironclaw mcp test command
• extensions/manager.rs: hot-reload MCP activation

Also adds the set_session_manager() public method to McpClient and a unit test.

Test plan

• cargo check — clean
• cargo clippy --all --tests — zero warnings
• cargo fmt -- --check — clean
• New test: test_set_session_manager

🤖 Generated with Claude Code

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves an issue where non-OAuth HTTP MCP clients were not correctly configured with a session manager, leading to a loss of per-session state on servers. By ensuring that a session manager is consistently applied during client initialization, this change guarantees that Mcp-Session-Id headers are always included in requests, thereby maintaining session state and improving the reliability of interactions with stateful MCP servers.

Highlights

• Session Manager Configuration: Introduced a new public method set_session_manager to the McpClient struct, allowing explicit assignment of a session manager for Streamable HTTP session tracking.
• Client Initialization Fixes: Applied the set_session_manager method to all non-OAuth HTTP MCP client instantiations across app.rs, cli/mcp.rs, and extensions/manager.rs to ensure proper session ID header tracking.
• Unit Testing: Added a dedicated unit test (test_set_session_manager) to verify the correct functionality of the new session manager setter method.

Changelog

• src/app.rs
  • Modified MCP client initialization logic to explicitly set the session manager for non-OAuth clients.
• src/cli/mcp.rs
  • Updated the ironclaw mcp test command's client creation to attach the session manager for unauthenticated MCP clients.
• src/extensions/manager.rs
  • Adjusted the hot-reload MCP activation client creation to ensure the session manager is set for non-OAuth clients.
• src/tools/mcp/client.rs
  • Added a new public method set_session_manager to McpClient for assigning a session manager.
  • Included a new unit test test_set_session_manager to validate the functionality of the new method.

Activity

• Verified cargo check passed cleanly.
• Confirmed cargo clippy --all --tests reported zero warnings.
• Ensured cargo fmt -- --check passed cleanly.
• Added and passed a new unit test: test_set_session_manager.
• The pull request was generated with Claude Code.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

The pull request introduces a set_session_manager method to the McpClient struct, enabling the attachment of a session manager for Streamable HTTP session tracking, and integrates this method into McpClient instantiation across src/app.rs, src/cli/mcp.rs, and src/extensions/manager.rs, along with a new unit test. Review comments suggest refactoring the set_session_manager into a builder-style with_session_manager method for more idiomatic Rust, which would simplify client instantiation at call sites and require updating the corresponding unit test.

_{Note: Security Review did not run due to the size of the PR.}

[nick-stebbings]

All four review comments addressed in 3f7753c: changed set_session_manager(&mut self, ...) to with_session_manager(mut self, ...) -> Self builder pattern. All call sites updated to use chained builder syntax, test renamed to test_with_session_manager.

[zmanian]

Review: fix(mcp): set session manager on non-OAuth HTTP MCP clients

Clean bug fix. Non-OAuth MCP clients were missing Mcp-Session-Id tracking, causing stateful servers to silently lose session context between calls.

Strengths:

• Fix applied consistently to all three non-OAuth paths (app.rs, cli/mcp.rs, extensions/manager.rs)
• Good commit progression: initial fix with set_session_manager() -> refactored to idiomatic with_session_manager() builder pattern
• Unit test added for the new method

No concerns. LGTM -- approve.

[zmanian]

Re-review: fix(mcp): wire session manager on non-OAuth HTTP MCP clients

The latest commit (4b054c4) consolidates the fix into create_client_from_config() in factory.rs, which is the right approach since app.rs and extensions/manager.rs both go through the factory now. The with_session_manager() builder pattern is clean and idiomatic.

However, one call site was missed:

Missed call site: cli/mcp.rs

src/cli/mcp.rs line ~509 still creates a bare McpClient::new_with_config(server.clone()) without attaching a session manager in the non-OAuth else branch of ironclaw mcp test. This path does not go through create_client_from_config(). The session_manager local variable is created on line 489 but only used in the has_tokens branch. This is the same bug the PR is fixing, just in a different call site.

// cli/mcp.rs ~line 509 -- missing session manager
} else {
    McpClient::new_with_config(server.clone())  // should chain .with_session_manager(session_manager)
};

The PR description explicitly lists cli/mcp.rs as a fixed path, and the earlier commits (3f7753c) may have fixed it, but the latest force-pushed commit dropped that fix.

Session header handling

The Mcp-Session-Id header handling in build_request_headers() is correct per the Streamable HTTP spec -- it only adds the header when a session ID exists, and session creation happens during initialize().

Test coverage

The unit test for with_session_manager() is fine for the builder method itself, but there's no test for the factory's non-OAuth branches. A test that exercises create_client_from_config() with an HTTP server config (no secrets, no OAuth) and asserts session_manager.is_some() on the result would directly cover the bug being fixed.

Summary

The factory fix is correct but incomplete -- cli/mcp.rs still has the same bug. Fix that call site and this is good to merge.

[nick-stebbings]

Fixed in becfbc4:

1. cli/mcp.rs missed call site — added .with_session_manager(session_manager) to the non-OAuth else branch (line 509). This path bypasses the factory so it needed the same treatment.

2. Factory regression test — added test_factory_non_oauth_http_has_session_manager: creates an HTTP config with no secrets (non-OAuth path), calls create_client_from_config(), asserts has_session_manager() is true.

3. has_session_manager() accessor — added to McpClient for test assertions without exposing the field.

[zmanian]

Both items from previous review addressed:

1. Missed cli/mcp.rs call site -- Fixed. Non-OAuth else branch now chains .with_session_manager(session_manager).
2. Factory regression test -- Added. test_factory_non_oauth_http_has_session_manager covers the exact bug scenario through the factory path.

Clean has_session_manager() accessor for testability. Approved.

Note: Fork PR -- CI only ran classify/scope. Logic verified by diff review.

[zmanian]

Replacement PR opened as #986 with the approved fix rebased onto current staging. The current codebase already routes CLI creation through the MCP factory, so the replacement preserves the original intent in the remaining live HTTP path and adds focused regression coverage.

[zmanian]

Superseded by #986, which has merged into staging.