-
Notifications
You must be signed in to change notification settings - Fork 108
fix: minor tweaks to enable vscode integration MCP-134 #467
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
Conversation
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.
Pull Request Overview
This PR adds VS Code extension integration support by enhancing the StreamableHttpRunner with header validation, port flexibility, and better type exports.
- Adds
httpHeaders
configuration option for server-side header validation - Enables random port assignment and exposes server address via getter
- Exports additional types (
StreamableHttpRunner
,defaultUserConfig
) for external consumption
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/common/config.ts | Adds httpHeaders field to config interface and default configuration |
src/transports/streamableHttp.ts | Implements header validation middleware and address getter for server |
src/lib.ts | Exports additional types and classes for external consumption |
tests/integration/transports/streamableHttp.test.ts | Updates tests to validate header functionality with comprehensive test cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Looks good, one suggestion
private constructor(logger: LoggerBase, timeout: number = DEVICE_ID_TIMEOUT) { | ||
this.logger = logger; | ||
this.timeout = timeout; | ||
this.getMachineId = (): Promise<string> => nodeMachineId.machineId(true); | ||
this.abortController = new AbortController(); | ||
|
||
this.deviceIdPromise = DeviceId.UnknownDeviceId; |
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.
private constructor(logger: LoggerBase, timeout: number = DEVICE_ID_TIMEOUT) { | |
this.logger = logger; | |
this.timeout = timeout; | |
this.getMachineId = (): Promise<string> => nodeMachineId.machineId(true); | |
this.abortController = new AbortController(); | |
this.deviceIdPromise = DeviceId.UnknownDeviceId; | |
private constructor({logger, timeout, deviceIdPromise, abortController}: {logger: LoggerBase, timeout: number = DEVICE_ID_TIMEOUT, deviceIdPromise: Promise<string>, abortController: AbortController}) { | |
this.logger = logger; | |
this.timeout = timeout; | |
this.abortController = abortController; | |
this.deviceIdPromise = deviceIdPromise; |
I think this is better because it'd be good to have the Promise resolve to unknown only in the end so we can always have a state of pending -> unknown
and not unknown -> pending -> unknown
(even though in practice we instantly override this in create
right now
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.
Unfortunately, we can't supply the promise through the constructor since we need the DeviceId
instance to construct the promise itself. Technically, the only reason to have it assigned here is to make the types nicer and ensure we don't have to write ?
all over the place when we know for sure the field is initialized at this point. So we have 3 options (with no preference which one to go for):
- Keep it as is
- Suppress the error that we're not initializing a required field
- Move the getDeviceId call to the constructor (and close our eyes for the fact it's introducing a side effect)
|
||
public static create(logger: LoggerBase, timeout?: number): DeviceId { |
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.
public static create(logger: LoggerBase, timeout?: number): DeviceId { | |
public static create(logger: LoggerBase, { timeout = DEVICE_ID_TIMEOUT } : { timeout: number} = {}): DeviceId { | |
const abortController = new AbortController(); | |
const deviceIdPromise = () => getDeviceId({ | |
getMachineId: () => nodeMachineId.machineId(true), | |
onError: (reason, error) => { | |
this.handleDeviceIdError(reason, String(error)); | |
}, | |
timeout: this.timeout, | |
abortSignal: abortController.signal, | |
}); | |
const instance = new DeviceId({logger, timeout, deviceIdPromise, abortController}); |
personal preference re: timeout as object because it's one of these things which we basically largely have only for test DI purposes
Proposed changes
This tweaks slightly some of the API around streamable http runner to allow it to be consumed by the vscode extension. Notably:
StreamableHttpRunner
anddefaultUserConfig
)