Skip to content

Conversation

@f
Copy link
Owner

@f f commented Apr 9, 2025

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@f f force-pushed the feature/guard branch from 999d442 to ad05f8f Compare April 9, 2025 18:33
@f
Copy link
Owner Author

f commented Apr 9, 2025

@ayakut16 Can you please review this feature and if you have any ideas please lmk.

@f f requested a review from Copilot April 9, 2025 18:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • go.mod: Language not supported
Comments suppressed due to low confidence (1)

pkg/guard/child_process.go:60

  • After calling c.cmd.Process.Kill(), consider calling c.cmd.Wait() (or an equivalent cleanup) to ensure the process terminates properly and resources are released.
if c.cmd.Process != nil {

@f f requested a review from Copilot April 9, 2025 18:38
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • go.mod: Language not supported

@ayakut16
Copy link
Contributor

ayakut16 commented Apr 9, 2025

@ayakut16 Can you please review this feature and if you have any ideas please lmk.

Sure, I will take a look shortly 👍

Copy link
Contributor

@ayakut16 ayakut16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general just some small nits.

Just a thought on naming: I believe proxy would have been a better name in this context because guard command literally proxies another server and it can do more than guarding in the future imo (although i can't think of a usecase now).

I understand that changing a name to already existing command could be troublesome, but just wanted to express my thoughts.

Something like bridge, middleware adapter etc. might also work but maybe this is something to worry about in the future when we actually have another usecases for proxying a server.

@f
Copy link
Owner Author

f commented Apr 10, 2025

LGTM in general just some small nits.

Just a thought on naming: I believe proxy would have been a better name in this context because guard command literally proxies another server and it can do more than guarding in the future imo (although i can't think of a usecase now).

I understand that changing a name to already existing command could be troublesome, but just wanted to express my thoughts.

Something like bridge, middleware adapter etc. might also work but maybe this is something to worry about in the future when we actually have another usecases for proxying a server.

wdyt about access-control or ac in short? since the only function is to filter tools access from llms. maybe context-filter?

and maybe using % instead of * like in SQL.

and maybe instead of --allow and --deny, just list as allow|deny:tool|prompt|resource:name

e.g. at last:

mcp context-filter deny:tool:delete_% deny:tool:write_% npx -y @modelcontextprotocol/server-filesystem ~

WDYT?

@f
Copy link
Owner Author

f commented Apr 10, 2025

if we do a middleware function we can extend it to a useful one as well like:

mcp middleware deny:tool:delete_% deny:tool:write_% npx -y @modelcontextprotocol/server-filesystem ~
mcp middleware /path/to/middleware.sh npx -y @modelcontextprotocol/server-filesystem ~

or smth

@ayakut16
Copy link
Contributor

and maybe using % instead of * like in SQL.

and maybe instead of --allow and --deny, just list as allow|deny:tool|prompt|resource:name

I think the original syntax is fine;using flags and the * wildcard feels more natural and shell-like. Why do you want to change that ?

Of your naming suggestions, I like context-filter (or just mcp filter to have it catchy), but I’m also okay keeping the name guard if the focus is on allowing or denying specific features. I initially mentioned a proxy-like name for future flexibility (since it technically proxies another server), but we don’t have an immediate need for that — so it may be a case of YAGNI.

@f f merged commit 89cc824 into master Apr 12, 2025
2 checks passed
@f f deleted the feature/guard branch April 12, 2025 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants