Repository navigation
refactor(tests): share the advertised-MCP-tool call - #863
Merged
Merged
Conversation
One transition_timeout_s bounds both the room's startup and the cleanup the test expects to give up. At 2.0s the real stdio peer's cold start (~1.2-1.4s on CI) left little margin, so loaded runners abandoned the start; the finally then missed a not-yet-spawned pipe holder, which kept stdout open for 120s and surfaced as CleanupIncompleteError. The peer now runs from a module that imports only acp and the stdlib, roughly halving its startup; the bound is 5s; and the holder waits on a release file the test always touches, so it never outlives the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KoeFsnuQr2KMCP7XfoEkE9
…ocol error Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KoeFsnuQr2KMCP7XfoEkE9
AlexanderZ-Band
commented
Oct 19, 2026
AlexanderZ-Band
left a comment
AlexanderZ-Band
left a comment
Collaborator
Author
There was a problem hiding this comment.
Cycle 1 review (my-code-review): 1 suggestion, 1 nit, both Test Quality; nothing blocking. Edge-Case and Logical Bugs hypotheses (timing under load, recovery/disposal ordering, holder races, crash-recovery lazy imports) were all reproduced against the real peer with injected startup delays and dropped — none failed or was reachable.
FakeACPAgent and the stdio peer both found an advertised MCP server and called a tool on it; tests/mcpclient.py now owns that once, keeping the clear not-advertised error for the stdio peer too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KoeFsnuQr2KMCP7XfoEkE9
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Main's controlled-pipe peer replaces the stdio peer module and holder-release file this branch added, so those are dropped; the shared MCP tool-call helper remains. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DksuPncgQrhHzfhqMyGUUW
AlexanderZ-Band
changed the title
AlexanderZ-Band
enabled auto-merge
October 20, 2026 07:01
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.
Why
FakeACPAgentcarried its own lookup of an advertised MCP server and its own tool call, a copy of what any other ACP test peer needs to do to call a Band tool.What
tests/mcpclient.pynow ownsadvertised_serverandcall_advertised_tool, andFakeACPAgent.mcp_server/call_mcp_tooldelegate to them, keeping the "was not advertised" error.Scope
Test-only. The earlier startup-headroom fix for the pipe-holder test is dropped: main's controlled-pipe peer (#831) removes that race.
🤖 Generated with Claude Code
https://claude.ai/code/session_01DksuPncgQrhHzfhqMyGUUW