-
Notifications
You must be signed in to change notification settings - Fork 8
Add stateless_http configuration option #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add stateless_http configuration option #12
Conversation
michaelmoore-s1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together! The AWS Bedrock deployment guide is excellent — the IAM policies are thorough, the setup instructions are clear, and the security considerations are well thought out.
I ran through the code and tests and found a few issues to address:
- There’s a type error in
tests/unit/test_cli_helpers.py:83where the test passesNone, but the function signature expects abool. You’ll need to either update the signature tostateless_http: bool | Noneor passFalsein that test. - The docstring for
_apply_environment_overridesis missing the new parameter, which Ruff flags as a D417 error. server.py:138defines a module-levelhttp_appwith hardcoded parameters, while the CLI creates its own instance dynamically. If someone runsuvicorn purple_mcp.server:http_appdirectly, that instance won’t reflect the configured settings.- Both JSON policy files are missing trailing newlines (per POSIX standards), and the main README should document the new environment variable alongside the existing ones.
- All current test assertions check for
stateless_http=False— consider adding at least one test that verifies behavior when it’s set toTrue.
A larger architectural concern is configuration management. This PR introduces PURPLEMCP_STATELESS_HTTP as an environment variable, but there’s no corresponding field in the Settings class in config.py. Per the project’s conventions, configuration values should flow through that Pydantic model to ensure validation and consistency. As it stands, this setting bypasses that mechanism entirely. I recommend adding a stateless_http field to Settings with appropriate defaults and documentation.
Thanks again for the PR — looking forward to reviewing the next iteration once these items are addressed.
…itional information to the README
…ing to reflect new stateless_http arg.
michaelmoore-s1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. You addressed several points from the previous review. The new stateless_http field in Settings is exactly what we needed, and the docstring updates look good.
Configuration management is now correctly centralized in Settings with validation and sensible defaults. The _apply_environment_overrides function documents the new parameter clearly, and the test_streamable_http_mode_options_and_stateless test confirms the flag is parsed and propagated.
There are a few remaining items to address before merge:
-
The module-level
http_appinserver.py:138-142hardcodestransport="sse". Since the CLI supports bothsseandstreamable-http, the module-level instance should read the transport fromSettingsso consumers of the module get the configured transport by default. -
The JSON policy files are missing trailing newlines. Please add a newline to the end of
bedrock-agentcore-iam-policy.jsonandbedrock-agentcore-trust-policy.jsonto comply with POSIX conventions. -
The current tests verify parsing and CLI propagation, but there is no test for behavioral differences when
stateless_httpisTruevsFalse. Add a test that asserts FastMCP is invoked with the expectedstateless_httpvalue in each case.
Once these are addressed, this should be ready to merge.
…unction for test_server.py
… tests for each http_app permutation.
## Summary Fixes issues introduced in PR #12 (stateless_http configuration) and bumps version to 0.6.0. ### Bug Fixes - **CI failure fix**: Added `stdio` to `transport_mode` Literal type - the CLI was setting `PURPLEMCP_TRANSPORT_MODE=stdio` but Settings only accepted `http`, `streamable-http`, or `sse` - **Exception handling**: Changed `contextlib.suppress(BaseException)` to `suppress(Exception)` in `server.py` to avoid catching `KeyboardInterrupt`, `SystemExit`, etc. - **Type cleanup**: Removed unnecessary `| None` from `stateless_http` field type - **Description fix**: Corrected `transport_mode` field description (was copy-paste error from `stateless_http`) ### Documentation - Added `PURPLEMCP_TRANSPORT_MODE` to README environment variables section - Updated CHANGELOG for 0.6.0 release ### Version Bump - 0.5.1 → 0.6.0 (new feature: stateless HTTP mode for AWS Bedrock AgentCore) ## Test plan - [x] `uv run ruff format` passes - [x] `uv run ruff check --fix` passes - [x] `uv run mypy` passes - [x] Unit tests pass locally - [x] CI server-startup-tests should pass (previously failed due to `stdio` not in Literal type) - [x] CI docker-startup-tests should pass
This change is required when running this MCP Server container in Amazon Bedrock AgentCore agent runtime. The change adds support for
stateless_http=True, including with the environment variablePURPLEMCP_STATELESS_HTTP.--
Tested by following the instructions at https://docs.aws.amazon.com/bedrock-agentcore/latest/devguide/runtime-mcp.html
Prior to this change, step 4 on the page above failed (Test with MCP client (remote)): the client hung, not getting back the response from the MCP Server.
After this change, the client correctly received the MCP information such as list of tools etc.
You can see in the AWS example (step 1) that they set this value for their FastMCP server.
--
Note an alternative one-line change that also worked for AgentCore testing was to change server.py as shown below. However, that may risk breaking other modes such as stdio (untested).
--
The changes here seem more in line with how other configuration options are being handled.