Repository navigation
test(core): cover legacy MCP timeout migration into the startup budget - #54174
Bearmancer wants to merge 3 commits into
Conversation
github-actions
Bot
added
needs:compliance
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix looks correct. v1 used a server's timeout for the connection handshake as well as for listing and tool calls (withTimeout(client.connect(...), mcp.timeout ?? 30_000)). Mapping it to startup restores that, while experimental.mcp_timeout rightly stays on catalog and execution only. I confirmed the new test fails on the base and passes here. normalization.test.ts and config.test.ts pass, and the packages/core typecheck is clean. I couldn't do an end-to-end run with a slow MCP server because opencode mcp list from source failed while starting the background service, on both the base and this branch.
Public API: unchanged. The config schema stays the same; only the v1→v2 migration output gains timeout.startup.
Points worth fixing:
- The evidence in the description doesn't support the change.
DEFAULT_STARTUP_TIMEOUTis already 30000, so a legacytimeout: 30000gets the same 30s connection budget with or without this change, and the 61.9s failure has another cause. The "only lengthens the connect budget" claim is also wrong: any legacy value below 30000 now shortens connection from 30s to that value (e.g. 5000 gives 5s). That matches v1, so it's the right behaviour, but please correct the description. prettier --checkfails on both changed files (the longtimeout:lines inmigrate.tsand the new test). Please run the formatter.- For a two-line fix the new test isn't needed and can be dropped to keep the change minimal. If you keep it, a generic server name and no
semantic-scholarcommand would be enough.
Related: #48743
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
Thanks for the review. Three things you flagged, addressed:
I kept the regression test, since the bug existed precisely because nothing asserted this mapping, and made the server name generic per your note. The compliance bot has since confirmed the PR meets the guidelines. |
The V1->V2 MCP migration maps a per-server scalar timeout to catalog and execution but never startup. Since startup is the only field the connection handshake reads, a migrated scalar is equivalent to leaving it unset, and the handshake falls back to DEFAULT_STARTUP_TIMEOUT. Map the scalar to startup as well so the configured value reaches the budget that governs connection.
Also run prettier over both changed files, which the original commit missed.
fea2459 to
0c17e57
Compare
Bearmancer
changed the title
github-actions
Bot
added
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
Closing as V1-obsolete: this test pins the V1-to-V2 MCP timeout migration (packages/core/src/v1/config/migrate.ts), which no longer matters. |
Issue for this PR
Closes #54184
Type of change
What does this PR do?
This is the regression test for the V1 to V2 MCP timeout migration reported in #54184, which nothing asserted before.
The production change is already on
dev:migrateMcpTimeout(timeout)inpackages/core/src/v1/config/migrate.tsreturns{ startup: timeout, catalog: timeout, execution: timeout }. The gap was that no test pinned that mapping, so a legacy scalartimeoutcould stop reaching the connection/startup budget without any test noticing.This PR adds that assertion to
packages/core/test/config/normalization.test.tsand changes nothing else. The migration helper itself is not touched.How did you verify your code works?
I ran the focused suite from
packages/coreon this branch (bun run script/test.ts test/config/normalization.test.ts): 38 pass, 0 fail, 340 expect() calls. I then temporarily revertedmigrateMcpTimeoutto dropstartupand reran the same file: the new test failed as expected (36 pass, 2 fail), which shows it pins the mapping; I restored the file right after and the tree is clean again.bun typecheckfrompackages/coreexits 0. I started the fullbun run testsuite but stopped it under the time-box, so it is not cited here. I did not run the suite on the base SHA; by inspection the base already carries the same mapping and base-to-head differs only by this test.Screenshots / recordings
If this is a UI change, please include a screenshot or recording.
Checklist