-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: hard-coded CLI inspector version #544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: hard-coded CLI inspector version #544
Conversation
|
Hi @richardkmichael thanks for catching this and for bringing up the inconsistencies. If you could fix the linting error in the CI I think we can go ahead and merge the CLI version fix. I created this to track the points you brought up here and discuss them further: #588 |
e59631a to
f453366
Compare
I didn't notice, sorry! Fixed. I did this without changing the eslint config. I find underscore prefixed unused variables to be self-documenting and less error prone, but the eslint configuration only permits the terser, omitted style. Not intending to bikeshed, just mentioning in case you wonder. |
olaservo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @richardkmichael was revisiting this one and it looks like there's some conflicts - besides that I think we're good to merge this, would you mind resolving those? Thanks!
f453366 to
fb5bd5a
Compare
|
@olaservo I extracted the identity to a constant, aiming to reduce test brittleness. If you prefer a more straight-forward conflict resolution, it's just the string change in the test; I can drop the most recent commit for that resolution only. |
Make both clients use name from package.json for consistency and protocol clarity - web client changes `mcp-inspector` -> `inspector-client` (package.json) - cli client remains `inspector-cli` (package.json)
513c053 to
ae7d745
Compare
…ed that reads package.json from one level up. It works if the project is checked out locally, but not in the npm package, because the package.json of the workspace packages is not included in the inspector package. We can get that from one level up, the inpsector's own package, and so this change does that.
Motivation and Context
The CLI is hard-coded at version
0.5.1, from when it was merged in #177 f4e6f4d.It can use the
package.jsonversion, as does the client.How Has This Been Tested?
Tested by inspecting raw protocol messages.
Incorrect version on
main:{ "method": "initialize", "params": { "protocolVersion": "2025-03-26", "capabilities": {}, "clientInfo": { "name": "inspector-cli", "version": "0.5.1" } }, "jsonrpc": "2.0", "id": 0 }Corrected:
{ "method": "initialize", "params": { "protocolVersion": "2025-03-26", "capabilities": {}, "clientInfo": { "name": "inspector-cli", "version": "0.14.3" } }, "jsonrpc": "2.0", "id": 0 }Breaking Changes
Unlikely.
Types of changes
Checklist
Additional context
Broader discussion
I have few questions and comments, but Discussions aren't enabled, so I'm not sure where to raise
them --
TL;DR:
NodeNextmodules?"noUncheckedIndexedAccess": truebe used throughout? Potential errors are detected.Despite this being a very tiny change, the varying TypeScript configurations across the sub-packages
tripped me up quite a bit, so I investigated a few details. Overall, consistency throughout the
monorepo would result in less confusion when reading and working with the parts of the inspector.
Generally lower cognitive overhead -- because, personally, when the same thing is accomplished in
multiple ways, it slows me down because I wonder why and which is correct or preferrable; especially
with unfamiliar code. Maybe this is second nature to experienced TS developers, working in
difficult contexts?
For example, the
package.jsonimport in thecliandclienthere.The
cliTypeScript config usesNodeNextmodules, which requires an import attribute (... with: { type: "json" }).The
clientusesESNext(andbundlerconfiguration), which tolerates an import attribute. Sothe quickest approach would be to add the import attribute in the
clientfiles,useConnection.tsxandSidebar.tsx.However, I could not find an easy way to enforce it (detection), so one needs to know. I briefly
checked the TS compiler options and
eslint-plugin-import; a custom rule might be needed, which isa bit too crufty, IMO.
Alternately, change the CLI to
Node16(used by the server and the TypeScript SDK). This wouldeliminate the requirement of an import attribute. It seems less preferable because enforcing modern
usage is ideal. Perhaps the
servershould go toNodeNexttoo?Somewhat related, the
clicompiler config uses"noUncheckedIndexedAccess": true, which caughtthe potential bug in the destructuring of
split()(but not in theclient). If this option isenabled on the
clientandserver, there are many errors. Should it be enabled?