Adding directory support for MCP resources#272623
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds directory support for MCP resources, allowing the filesystem to properly handle and distinguish between files and directories. The changes improve the logic for determining whether a resource is a file or directory, and add interactive directory navigation to the quick access picker.
Key changes:
- Modified directory detection logic to check if multiple contents are returned rather than filtering by URI
- Added directory navigation support to the quick access picker with file service integration
- Updated the
equalsUrlPathcomment to note MCP may return directory names with trailing slashes
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/mcp/common/mcpResourceFilesystem.ts | Updates directory detection logic to use content count instead of URI filtering, removes unused forSameURI variable |
| src/vs/workbench/contrib/mcp/browser/mcpResourceQuickAccess.ts | Adds directory navigation support with file service and notification service integration, enables browsing directory contents in the picker |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I think we actually want to add much of this logic/handling in
|
src/vs/workbench/contrib/chat/browser/chatContextPickService.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/browser/actions/chatContextActions.ts
Outdated
Show resolved
Hide resolved
| })); | ||
|
|
||
| return store; | ||
| return Disposable.None; |
There was a problem hiding this comment.
should we not return anything here now?
There was a problem hiding this comment.
We do not have anything to dispose here. We need to dispose helper only when picker is hidden. Added a new handler for that and removed this._register for onDidAccept method.
There was a problem hiding this comment.
We should remove this return now then.
There was a problem hiding this comment.
This method is called by provide(picker: IQuickPick<IQuickPickItem, { useSeparators: true }>, token: CancellationToken, runOptions?: IQuickAccessProviderRunOptions): IDisposable which is implementing IQuickAccessProvider
| picker.busy = true; | ||
| helper.toAttachment((event.item as ResourceQuickPickItem).resource).then(async a => { | ||
| const resourceItem = event.item as ResourceQuickPickItem; | ||
| helper.toAttachment(resourceItem.resource, resourceItem.server).then(async a => { |
There was a problem hiding this comment.
What happens if it's not valid[ate]ForAttachment?
There was a problem hiding this comment.
Updated the code to allow only for valid attcachments
This reverts commit 9e4b005.
dileepyavan
left a comment
There was a problem hiding this comment.
Is this in the MCP spec somewhere? From the info shared in the original issue it looks like the MCP server does not include a trailing slash on its 'directory' #262368
I dont find this as a rule specified in official MCP documentation, i referenced this earlier from chatgpt and i think default agent mode. The only rule specified is https://modelcontextprotocol.io/specification/2025-06-18/server/resources#file%3A%2F%2F . This also contains "MAY" so unsure if it will be always the case
dileepyavan
left a comment
There was a problem hiding this comment.
Currently the helper disposes when the parent(McpAddContextContribution) disposes. Currently this might create an issue with multiple instances of the MCPResourcehelper. I think we can dispose the helper when the quick pick is closed using "onDidHide" event.
dileepyavan
left a comment
There was a problem hiding this comment.
The directory detection logic is incorrect. A resource with exactly 1 content item will be treated as a File, but the check at line 164 (
contents.length <= 1) treats single-item resources as errors when callingreaddir(). This creates inconsistent behavior wherestat()says it's a file butreaddir()rejects it as not being a directory.
These changes are no longer needed.
| detail?: string; | ||
| disabled?: boolean; | ||
| asAttachment(): IChatRequestVariableEntry | Promise<IChatRequestVariableEntry>; | ||
| validateForAttachment?(): Promise<boolean>; |
There was a problem hiding this comment.
I think we no longer need validateForAttachment now that we have the updated return type for asAttachment
There was a problem hiding this comment.
I misunderstood the earlier comment :). Intention of making it as noop is to include this scenario in asattachment. got it. will update
| if (isThenable(attachment)) { | ||
| addPromises.push(attachment.then(v => widget.attachmentModel.addContext(v))); | ||
| qp.busy = true; | ||
| const isValidForAttachment = await selected.validateForAttachment?.().finally(() => { |
There was a problem hiding this comment.
If we get rid of validateForAttachment, we would essentially selected.asAttachment here instead (keeping qp.busy=true while that's resolving).
We still want to keep addPromises in case the user closes the quickpick after selecting something and before asAttachment resolves, but if the picker is closed by that point we would do nothing if it returns noop
|
|
||
| store.add(qp.onDidHide(() => { | ||
| defer.complete(true); | ||
| pickerConfig.dispose?.(); |
There was a problem hiding this comment.
We also want to call this up on line 632 where we return true
There was a problem hiding this comment.
cancellation token is requested on DidHide event. We are already disposing on this event. Do we still need to dispose on line 632?
There was a problem hiding this comment.
Yea, if we bail out early (e.g. if the promise was cancelled before awaitable picks resolve) then we should still dispose the pickerConfig
|
|
||
| public navigateBack(): boolean { | ||
| const items = this._pickItemsStack.pop(); | ||
| if (items) { |
There was a problem hiding this comment.
We don't set inDirectory if the stack is empty -- does this work for going back to the root?
There was a problem hiding this comment.
If the stack is empty navigateBack function returns false causing the default goback handler to kick in
| } | ||
| onChange(output); | ||
|
|
||
| if (this._inDirectory.get() === undefined) { |
There was a problem hiding this comment.
The check for this._inDirectory.get() is unnecessary, and could lead to missed items if the user picks a directory while other server resources are still loading in
There was a problem hiding this comment.
correct, i think this got left over from previous changes before derived observable is implemented.
| onChange(output); | ||
|
|
||
| if (this._inDirectory.get() === undefined) { | ||
| this._resources.set(output, undefined); |
There was a problem hiding this comment.
I suggest we create a new value on each publish, otherwise the observables won't detect a change because they do an equality check on the old and new values. Because we're just updating the same map each time, this won't trigger updates.
We could have the output here be a flat (IMcpServer | IMcpResourceTemplate | IMcpResource)[] and let the consumers either filter the results or turn them into separators as necessary
There was a problem hiding this comment.
In Publish method, we are creating new Map for every update.
| })); | ||
|
|
||
| return store; | ||
| return Disposable.None; |
There was a problem hiding this comment.
We should remove this return now then.
connor4312
left a comment
There was a problem hiding this comment.
Almost there, some final more localized comments
| } | ||
| if (isThenable(attachment)) { | ||
| addPromises.push(attachment.then(v => widget.attachmentModel.addContext(v))); | ||
| addPromises.push(attachment.then(v => widget.attachmentModel.addContext(v as IChatRequestVariableEntry))); |
There was a problem hiding this comment.
In the interface we say that you can return Promise<IChatRequestVariableEntry | 'noop'>. We should either change the interface to not allow that (making the as cast unnecessary) or support it here
|
|
||
| store.add(qp.onDidHide(() => { | ||
| defer.complete(true); | ||
| pickerConfig.dispose?.(); |
There was a problem hiding this comment.
Yea, if we bail out early (e.g. if the promise was cancelled before awaitable picks resolve) then we should still dispose the pickerConfig
| observable.set({ picks, busy: true }, undefined); | ||
| }, token).finally(() => { | ||
| observable.set({ busy: false, picks: observable.get().picks }, undefined); | ||
| return { picks, busy: false }; |
There was a problem hiding this comment.
We should propagate the loading state from the helper.getPicks function too I think -- in this change we lost the 'busy' indicator that is shown while the picks are loading in from the servers
|
|
||
| // Convert all the children to IMcpResource objects | ||
| const childResources: IMcpResource[] = stat.children!.map(child => { | ||
| // const mcpUri = McpResourceURI.fromServer(server.definition, child.resource.toString()); |
| this._inDirectory.set({ server, resources: childResources }, undefined); | ||
| return true; | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
nit: would prefer to keep this try/catch more specific around the stat call to avoid hiding actual programming errors that might crop up
| const deferred = new DeferredPromise<ChatContextPickAttachment>(); | ||
| if (this._isDirectoryResource(resource)) { | ||
| //Check if directory | ||
| this.checkIfDirectoryAndPopulate(resource, server); |
There was a problem hiding this comment.
This is kind of fire and forget.
| const deferred = new DeferredPromise<ChatContextPickAttachment>(); | ||
| if (this._isDirectoryResource(resource)) { | ||
| //Check if directory | ||
| this.checkIfDirectoryAndPopulate(resource, server); | ||
| return noop; | ||
| } | ||
|
|
||
| public async toAttachment(resource: IMcpResource | IMcpResourceTemplate): Promise<IChatRequestVariableEntry | undefined> { | ||
| if (isMcpResourceTemplate(resource)) { | ||
| return this._resourceTemplateToAttachment(resource); | ||
| this._resourceTemplateToAttachment(resource).then(val => { | ||
| attachmentResult = (val as IChatRequestVariableEntry) || noop; | ||
| deferred.complete(attachmentResult); | ||
| }); | ||
| } else { | ||
| return this._resourceToAttachment(resource); | ||
| this._resourceToAttachment(resource).then(val => { | ||
| attachmentResult = (val as IChatRequestVariableEntry) || noop; | ||
| deferred.complete(attachmentResult); | ||
| }); | ||
| } | ||
| return deferred.p; |
There was a problem hiding this comment.
I don't think you need a deferred here. Also the as casts are not correct -- the methods can return undefined if the user cancels template URI creation, and that should be bubbled up to the caller.
| const deferred = new DeferredPromise<ChatContextPickAttachment>(); | |
| if (this._isDirectoryResource(resource)) { | |
| //Check if directory | |
| this.checkIfDirectoryAndPopulate(resource, server); | |
| return noop; | |
| } | |
| public async toAttachment(resource: IMcpResource | IMcpResourceTemplate): Promise<IChatRequestVariableEntry | undefined> { | |
| if (isMcpResourceTemplate(resource)) { | |
| return this._resourceTemplateToAttachment(resource); | |
| this._resourceTemplateToAttachment(resource).then(val => { | |
| attachmentResult = (val as IChatRequestVariableEntry) || noop; | |
| deferred.complete(attachmentResult); | |
| }); | |
| } else { | |
| return this._resourceToAttachment(resource); | |
| this._resourceToAttachment(resource).then(val => { | |
| attachmentResult = (val as IChatRequestVariableEntry) || noop; | |
| deferred.complete(attachmentResult); | |
| }); | |
| } | |
| return deferred.p; | |
| if (this._isDirectoryResource(resource)) { | |
| //Check if directory | |
| this.checkIfDirectoryAndPopulate(resource, server); | |
| return noop; | |
| } | |
| if (isMcpResourceTemplate(resource)) { | |
| return this._resourceTemplateToAttachment(resource) | |
| } else { | |
| return this._resourceToAttachment(resource); | |
| } |
There was a problem hiding this comment.
Agree, deferred was introduced earlier when checkIfDirectoryAndPopulate was returning a value.No longer needed.
| store.add(picker.onDidHide(() => { | ||
| helper.dispose(); | ||
| store.dispose(); | ||
| })); |
There was a problem hiding this comment.
We actually do not want to do this, and instead we want to keep returning the store. In the case of quick access, the McpResourceQuickAccess does not own the quickpick, it only gets the opportunity to populate it. If I open MCP quick access (type mcpr + space in the command palette), that triggers this MCP quick access logic. But I can then backspace and type something else. That will cause the value returned by McpResourceQuickAccess.provide to be disposed and the quickpick can later be used by some other logic.
| } | ||
| } | ||
| return { picks, busy: pickItems.isBusy }; | ||
| } catch { |
There was a problem hiding this comment.
I suggest removing the try/catch. Errors are not expected here and it will just hide anything that comes up.
| const qp = store.add(this._quickInputService.createQuickPick({ useSeparators: true })); | ||
| qp.placeholder = localize('mcp.quickaccess.placeholder', "Search for resources"); | ||
| store.add(this.applyToPick(qp, token)); | ||
| this.applyToPick(qp, token); |
There was a problem hiding this comment.
This should be disposed again now
| const isNested = await helper.navigate(resource, resourceItem.server); | ||
| if (isNested) { | ||
| // Navigation succeeded, picker will be updated by getPicks() observing _inDirectory changes | ||
| picker.show(); |
There was a problem hiding this comment.
Technically don't need the picker.show() calls here and above. It is not hidden by default when an item is accepted, unless you explicitly do so
fixes #262368