Conversation
Co-authored-by: Bill Easton <strawgate@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae82929702
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
docs/deployment/http.mdx
Outdated
| - **MCP Inspector** - Browser-based debugging tool for testing MCP servers | ||
| - **Postman** - API testing platform with MCP request support (see [Testing guide](/patterns/testing#testing-with-postman)) |
There was a problem hiding this comment.
This section says browser-based MCP clients that need CORS include Postman, but the new Testing guide explicitly states MCP support is available in the Postman desktop app. Desktop clients are not subject to browser CORS, so this guidance is internally inconsistent and can lead readers to enable CORS unnecessarily (weakening security) when using Postman. Consider either removing Postman from this browser-only list or clarifying that the CORS requirement applies only to Postman’s browser-based client, if MCP support exists there.
Useful? React with 👍 / 👎.
|
/marvin how does this match the postman docs? Are we right? |
|
Claude finished @strawgate's task in 2m 21s —— View job Verification Results
SummaryThe codex review comment is correct - our documentation has an inconsistency. Here's what the research shows: Key Findings1. Postman Desktop vs Web:
2. CORS Requirements:
3. The Inconsistency:
This is misleading because:
RecommendationsOption 1 (Recommended): Remove Postman from the CORS list since we're specifically documenting the desktop app: Browser-based MCP clients that need CORS include:
- **MCP Inspector** - Browser-based debugging tool for testing MCP servers
- **Custom browser-based MCP clients** - If you're building a web app that directly connects to MCP serversOption 2: Clarify that only Postman's web version needs CORS (but this adds complexity without value since we recommend the desktop app): - **Postman web app** - Browser-based API testing (note: Postman desktop app does not require CORS)The codex review is right: mentioning Postman in a "browser-based" CORS section contradicts our guidance about using the desktop app. Option 1 is cleaner and matches what we're actually recommending users do. Sources
|
|
/marvin let's also add mcpjam and let's incorporate the feedback |
|
Claude finished @strawgate's task in 4m 16s —— View job Task Complete
SummarySuccessfully incorporated the feedback and added MCPJam Inspector documentation: Fixed CORS/Postman Inconsistency:
Added MCPJam Inspector Documentation:
Key Insights:
All tests pass (4036 passed) and linting is clean. Changes committed and pushed to the PR branch. |
- Add comprehensive MCPJam Inspector documentation to testing guide - Replace Postman with MCPJam Inspector in CORS browser-based clients list - MCPJam Inspector is browser-based and needs CORS; Postman desktop app does not - Provide comparison of when to use MCPJam vs Postman vs Pytest Co-authored-by: Bill Easton <strawgate@users.noreply.github.com>
Test Failure AnalysisSummary: Tests are timing out during teardown on Windows when trying to clean up a temporary directory in . Root Cause: The test fixture uses without the parameter. On Windows, file handles can remain open slightly longer than expected, causing to fail during cleanup when it tries to scan directories that haven't been fully released yet. This is a well-documented Windows-specific issue with Python's temporary directory cleanup. Suggested Solution: Add to the call in the fixture (line 34 of ). This pattern is already used elsewhere in the codebase (see ). Change line 34 from: with tempfile.TemporaryDirectory() as temp_dir:To: with tempfile.TemporaryDirectory(ignore_cleanup_errors=True) as temp_dir:Detailed AnalysisThe test timeout occurs during fixture teardown: The issue is that This is a known Python issue on Windows that affects multiple projects. The References: Related Files
|
Test Failure AnalysisSummary: Tests are timing out during teardown on Windows when trying to clean up a temporary directory in test_oauth_proxy_storage.py. Root Cause: The test fixture temp_storage uses tempfile.TemporaryDirectory() without the ignore_cleanup_errors=True parameter. On Windows, file handles can remain open slightly longer than expected, causing shutil.rmtree() to fail during cleanup when it tries to scan directories that have not been fully released yet. This is a well-documented Windows-specific issue with Python temporary directory cleanup. Suggested Solution: Add ignore_cleanup_errors=True to the TemporaryDirectory() call in the temp_storage fixture at line 34 of tests/server/auth/test_oauth_proxy_storage.py. This pattern is already used elsewhere in the codebase at tests/server/middleware/test_caching.py:293. Change line 34 from: with tempfile.TemporaryDirectory() as temp_dir: To: with tempfile.TemporaryDirectory(ignore_cleanup_errors=True) as temp_dir: Detailed AnalysisThe test timeout occurs during fixture teardown at:
The issue is that MultiDiskStore from the key-value library may still have file handles open when pytest tries to clean up the temporary directory. On Windows this causes rmtree to hang when scanning directories. This is a known Python issue on Windows that affects multiple projects. The ignore_cleanup_errors=True parameter available in Python 3.10+ tells TemporaryDirectory to ignore errors during cleanup and remove files on a best-effort basis which is appropriate for test fixtures. References:
Related Files
|
|
/marvin let's put mcpjam first and postman second and let's do a thorough review of our instructions to make sure they are minimal and reference the external documentation from mcpjam and postman. We dont want to have to constantly check if these instructions are right. Let's double check everything we say about each one against their docs and make sure they are accurate. |
|
Claude finished @strawgate's task in 5m 46s —— View job Task Complete
SummarySuccessfully reordered the testing documentation and thoroughly verified all claims against official sources. The documentation now follows your requested structure with MCPJam first and Postman second, while maintaining minimal instructions that reference external documentation. Key ChangesDocumentation Structure:
Content Minimization:
Accuracy Verification:
Quality Assurance:
What We AvoidedFollowing your guidance to "not constantly check if these instructions are right," the new documentation:
|
…ructions - Reorder sections to put MCPJam Inspector before Postman - Minimize instructions and reference official documentation - Verify all claims against official sources - Update CORS section to correctly list MCPJam as browser-based - Reduce MCPJam section from ~40 to ~15 lines - Reduce Postman section from ~65 to ~25 lines Co-authored-by: Bill Easton <strawgate@users.noreply.github.com>
1 participant
Summary
Adds documentation for testing FastMCP servers using Postman's MCP request feature. Postman now provides native support for the Model Context Protocol, allowing developers to interactively test and debug their servers through Postman's visual interface.
This enhancement provides developers with an additional testing option alongside the existing pytest-based testing approach. Postman is particularly useful for manual testing, exploring server capabilities, and debugging during development.
Changes
docs/patterns/testing.mdxwith setup instructions, usage guidance, and comparison to pytestdocs/deployment/http.mdxalongside MCP InspectorFixes #3091
Generated with Claude Code