test(mcp_server): add smoke test for tools/call deadlock regression#86
Merged
ykykj merged 1 commit intoMay 10, 2026
Merged
Conversation
Spawns vibe-trading-mcp via subprocess and walks through the JSON-RPC happy path: initialize + tools/list + tools/call analyze_options. The critical assertion is that tools/call does not hang — without the registry pre-warm in mcp_server.main() (PR #85), the lazy registry build runs inside FastMCP's worker thread and deadlocks on the first call. The test also verifies (a) the tool catalogue has at least 20 entries including analyze_options/get_market_data/list_skills, and (b) the analyze_options result has a positive price and a delta in [0, 1] — covering tool-registration regressions and Black-Scholes formula breaks in addition to the deadlock. analyze_options is chosen over network-dependent tools (get_market_data, web_search) so the test stays deterministic and runs offline in CI. Verified locally with differential validation: - with pre-warm: PASS in 3.91s - pre-warm commented out: FAIL with 15s timeout on tools/call - pre-warm restored: PASS in 3.81s
6 tasks
Collaborator
|
Thanks for adding this MCP smoke test. I verified the test locally as well, and it is useful for guarding against the tools/call deadlock regression. One small note for future PRs: since this is an integration-style subprocess test, we may later tune the default pytest selection if the suite gets slower. For now this looks good. |
Contributor
Author
|
Thanks for the local verify and the heads-up on integration-style test categorization — noted for future PRs (likely will add a |
12zuev
added a commit
to 12zuev/Vibe-Trading
that referenced
this pull request
May 25, 2026
…HKUDS#86) Append-only experiment log per AUTORESEARCH.md protocol. Seeded with the changes already shipped 2026-05-25: - Binance MIN_CONFIDENCE 0.61→0.55 - TRADE_CRYPTO_ANALYSIS_LIMIT 3→10 - INLINE_ANALYSIS_LIMIT_MAX 1→3 - Binance MAX_CONCURRENT_POSITIONS 5→7 - vol_penalty piecewise + dynamic ceiling (PR HKUDS#144, status=running, evaluate at 2026-05-26T17:15Z) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
12zuev
added a commit
to 12zuev/Vibe-Trading
that referenced
this pull request
May 25, 2026
Karpathy pattern fully wired: AUTORESEARCH.md + experiments.tsv + new cron job be4888eb that follows the 10-step experiment protocol. Founder: безлимит на время/токены, цель — постоянный рост в заработке. Cron now actually learns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
longmfe
pushed a commit
to longmfe/Vibe-Trading-Mars
that referenced
this pull request
Jul 14, 2026
…KUDS#86) Spawns vibe-trading-mcp via subprocess and walks through the JSON-RPC happy path: initialize + tools/list + tools/call analyze_options. The critical assertion is that tools/call does not hang — without the registry pre-warm in mcp_server.main() (PR HKUDS#85), the lazy registry build runs inside FastMCP's worker thread and deadlocks on the first call. The test also verifies (a) the tool catalogue has at least 20 entries including analyze_options/get_market_data/list_skills, and (b) the analyze_options result has a positive price and a delta in [0, 1] — covering tool-registration regressions and Black-Scholes formula breaks in addition to the deadlock. analyze_options is chosen over network-dependent tools (get_market_data, web_search) so the test stays deterministic and runs offline in CI. Verified locally with differential validation: - with pre-warm: PASS in 3.91s - pre-warm commented out: FAIL with 15s timeout on tools/call - pre-warm restored: PASS in 3.81s
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
3 participants
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.
Summary
tools/calldeadlock fix).vibe-trading-mcpover stdio and verifies the JSON-RPC happy path:initialize+tools/list+tools/call analyze_options.Why
#85 fixes a deadlock where
tools/callhangs indefinitely if the registry pre-warm inmcp_server.main()is removed. Without a regression test, a future contributor could revert that line during a refactor and reintroduce the bug —initializeandtools/listwould still pass, so the breakage would be silent until users hittools/call.This test fails deterministically (15s timeout on
tools/call) when the pre-warm is removed, with an assertion message that points back at #85.What it covers
initialize— JSON-RPC handshake completes within 30s (covers cold imports).tools/list— at least 20 tools registered, includinganalyze_options,get_market_data, andlist_skills. Catches tool-registration regressions and missing package data.tools/call analyze_options— pure CPU Black-Scholes calculation; must return within 15s. This is the call that deadlocks without the fix.price > 0,delta ∈ [0, 1]. Catches Black-Scholes formula regressions.Differential validation
Verified locally on Windows + Python 3.12 + fastmcp from main:
mcp_server.main()statetools/listsucceeds in all three runs — onlytools/callexposes the deadlock, which is the exact symptom users hit (e.g. #42, #32).Why no network in the test
analyze_optionsis a Black-Scholes calculation with no external service. Picking it overget_market_datakeeps the test:Test plan
pytest agent/tests/test_mcp_server_smoke.py -vpasses locally (~4s)mcp_server.main()is removedfinally; no shared state with other testsNotes for reviewer
Tested locally on Windows + Python 3.12 with the differential validation table above. CI will be the first run on Linux + Python 3.11. The test uses only portable APIs (
subprocess.Popen,pathlib.Path,os.pathsep,sys.executable,threading), so cross-platform issues are unlikely — but I'm happy to follow up with fixes if anything surfaces in CI.This is also the first subprocess-spawning test in this repo (existing tests use direct imports / FastAPI TestClient). I picked subprocess because the deadlock only manifests across the FastMCP server thread boundary, which is hard to mock faithfully. Happy to refactor toward a fixture-based / mocked approach if that fits the repo's testing philosophy better.
Checklist
agent/src/agent/,agent/src/session/,agent/src/providers/) — only adds a test file underagent/tests/.sys.executableand resolves repo root from__file__.CONTRIBUTING.md(Conventional Commit title,@pytest.mark.integration, Google-style docstrings).