Skip to content

fix(cli): serialize mcp_http tests - #226

Merged
aram-devdocs merged 1 commit into
mainfrom
fix/mcp-http-serial-test
May 6, 2026
Merged

fix(cli): serialize mcp_http tests#226
aram-devdocs merged 1 commit into
mainfrom
fix/mcp-http-serial-test

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

Summary

  • The 4 async HTTP-transport tests in crates/plumb-cli/tests/mcp_http.rs each call reserve_port(), which binds a TCP listener on port 0, reads back the OS-assigned port, drops the listener, then spawns a child to re-bind it. The drop-then-bind window is a TOCTOU race; under parallel execution the kernel can hand the same port to two reservations and the loser surfaces "Connection reset by peer" (most often on http_transport_rejects_invalid_bearer_token).
  • Add serial_test = "3" to [workspace.dependencies] and the plumb-cli [dev-dependencies].
  • Annotate every #[tokio::test] HTTP fn with #[serial_test::serial(mcp_http)] so HTTP tests serialize against each other while non-HTTP tests stay parallel.

Test plan

  • cargo test -p plumb-cli --test mcp_http — 6 passed locally
  • cargo fmt --all -- --check
  • cargo clippy -p plumb-cli --tests -- -D warnings
  • CI green on the affected matrix

🤖 Generated with Claude Code

The 4 async HTTP transport tests in `crates/plumb-cli/tests/mcp_http.rs`
each call `reserve_port()` which binds a TCP listener on port 0, reads
back the OS-assigned port, and drops the listener before spawning a
child process to bind it. The drop-then-bind window is a TOCTOU race —
under parallel test execution the kernel can hand the same port to two
concurrent reservations, causing the second child to fail with
"Connection reset by peer" (most often surfacing on
`http_transport_rejects_invalid_bearer_token`).

Annotate the 4 async tests with `#[serial_test::serial(mcp_http)]` so
they serialize against each other while non-HTTP tests still run in
parallel. The named group keeps the blast radius minimal.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

I now have all the context needed to write the full review.


PR #226fix(cli): serialize mcp_http tests

Changed files

  • Cargo.toml — adds serial_test = "3" to [workspace.dependencies]
  • crates/plumb-cli/Cargo.toml — adds serial_test to [dev-dependencies]
  • Cargo.lock — adds scc, sdd, serial_test, serial_test_derive; also bumps windows-sys 0.60.2 → 0.61.2 (transitive)
  • crates/plumb-cli/tests/mcp_http.rs — annotates 4 async tests

Determinism

No impact. All changes are dev-only; serial_test never touches the production binary or observable output. ✓

Workspace layering

serial_test is placed under [dev-dependencies] of plumb-cli — the correct crate (top-level binary, only one allowed to have broad deps). No layer violation. ✓

Error handling / unsafe

No library code touched. ✓

Test correctness

crates/plumb-cli/tests/mcp_http.rs:114,127,139,151#[serial_test::serial(mcp_http)] is placed before #[tokio::test] on all four sites. This is the required ordering: serial must wrap the tokio runtime, not the other way around. ✓

Scope is tight: only the four port-binding async tests share the mcp_http key. All other tests remain parallel. The fix is minimal and targeted.

New dependencies — license check

serial_test v3.4.0 (MIT), serial_test_derive v3.4.0 (MIT), scc v2.4.0 (Apache-2.0), sdd v3.0.10 (Apache-2.0). All four are on deny.toml's allow-list. cargo-deny will pass. ✓

One observation (not a blocker)

Cargo.lock bumps windows-sys 0.60.2 → 0.61.2 as a transitive side-effect of the dep resolution. This is benign and expected when adding a new dep, but it wasn't mentioned in the PR description. Not a problem — just worth noting that cargo-deny will validate it on CI.

Punch list

Nothing blocking or warning-class. PR is correct and minimal.

Verdict: APPROVE


PR #225fix(ci): skip Claude review on dependabot/renovate PRs

Changed files

  • .github/workflows/claude-code-review.yml — updates the if: guard on the review job

Analysis

.github/workflows/claude-code-review.yml:29-43 — The original single-condition guard:

if: github.event.pull_request.draft == false

is replaced with a >- YAML folded scalar that appends two actor exclusions. The >- operator folds newlines into spaces and strips the trailing newline, producing a syntactically valid single-line GitHub Actions expression. ✓

The exclusions are dependabot[bot] and renovate[bot] — the canonical actor names for the hosted versions of both bots. The motivation (reduced GITHUB_TOKEN scopes causing the action's App-token request to fail) is documented inline with a reference to Anthropic's own security guidance. ✓

Minor warning (not a blocker): Some self-hosted Renovate installations use the actor name renovate-bot (no brackets). If this repo ever runs self-hosted Renovate, the CI failure would resurface. A belt-and-suspenders guard could add && github.actor != 'renovate-bot'. Current scope only covers the hosted bots, which is what the PR claims to fix.

No Rust code changes; all determinism, layering, and error-handling rules are out of scope.

Punch list

  • .github/workflows/claude-code-review.yml — consider adding github.actor != 'renovate-bot' if self-hosted Renovate is ever used. Not a blocker for the stated fix.

Verdict: APPROVE

@aram-devdocs
aram-devdocs merged commit ffddaee into main May 6, 2026
25 of 34 checks passed
@aram-devdocs
aram-devdocs deleted the fix/mcp-http-serial-test branch May 6, 2026 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant