test: verify the SSE crawl transport end to end - #104
Merged
Conversation
`ServerSpec` has accepted `transport: sse` since the beginning and `crawler/transports.py` has always had a branch for it, but nothing in the repository ever executed that branch. No test, no script, no CI job constructed a spec with `transport="sse"`, and the fixture server could not speak SSE at all, so the branch could not have been covered even by accident. The MCP 2.x migration rewrote that module with SSE untested. This is the same failure shape the `minimum-mcp` job was hardened against: a public surface that looks supported because the type admits it. The fixture server grows an `sse` mode, `test_crawl_sse` crawls it for real through `crawl_server`, and `verify_mcp_floor.py` generalizes its streamable-http leg into `http_transport(mode, transport, path)` so both HTTP transports are driven at the declared SDK floor. Putting it there is deliberate: the `minimum-mcp` job is a required PR check, so this cannot go stale the way a schedule-only script can. Verified green at both mcp 2.1.1 (the declared floor) and 2.2.0. No runtime behaviour changes; `transports.py` is docstring-only. Also from review: - reap the fixture child after `kill()`, or teardown returns while it is still exiting and the next test races it for a port - build the floor script's legs from factories, so a failing leg does not leave later coroutines unawaited and bury the real error in RuntimeWarnings Headers forwarding stays unexercised on both HTTP transports; filed as #103. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ue4wAZWpL7PFe7ARenR7fz
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
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.
What and why
ServerSpechas always acceptedtransport: sse, andcrawler/transports.pyhas always had a branch for it. Nothing in the repository ever executed that
branch. No test, no script, no CI job constructed a spec with
transport="sse", and the fixture server could not speak SSE at all, so thebranch could not have been covered even by accident. The MCP 2.x migration
(#99) rewrote that module with SSE untested.
That is the failure shape this repo has been bitten by before: a broken script
sat green through 14 PR checks because only a scheduled workflow ran it. A
public surface looks supported because the type admits it.
The good news: SSE works. It was verified green at both mcp 2.1.1, the
declared floor, and 2.2.0, the current lockfile version. Nothing was broken.
This PR makes that a fact CI enforces rather than an assumption.
Changes
tests/fixtures/sample_server.pysseargv mode; without it SSE was untestabletests/test_crawler.py_serve(mode)helper shared by both HTTP fixtures; newtest_crawl_ssescripts/verify_mcp_floor.pystreamable_http()generalized tohttp_transport(mode, transport, path);sseleg addedtests/test_release_hardening.pytoolgraph/crawler/transports.pyREADME.md,CHANGELOG.mdThe floor script is the deliberate home for the SSE leg.
minimum-mcpis arequired PR check that installs the declared floor, so this cannot go stale the
way a schedule-only script can. It adds one server subprocess per invocation.
Review findings applied
Codex reviewed this and returned SHIP with no blockers and no majors. Two
minors were real and are fixed here:
proc.kill()without a followingwait(), soteardown could return with the child still exiting and the next test racing
it for a port. Pre-existing, but now shared by two fixtures.
failing leg left the later ones unawaited and buried the real error under
RuntimeWarnings. They are now built from factories.Verification
Not covered
ServerSpec.headersis forwarded to upstream MCP servers by both HTTPtransports and is still never tested with a non-empty value. The two transports
use different SDK mechanisms there, so one can break without the other, and it
is the auth path. Filed as #103; this PR does not close it.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Ue4wAZWpL7PFe7ARenR7fz