Skip to content

Conversation

@Naman2701B
Copy link
Collaborator

@Naman2701B Naman2701B commented Jul 10, 2025

  • For now, the nuke server and the related tools have been added.
  • Updated the sample prompts for nuke servers.
  • Update the ShardGuard agent instructions to not allow extra parameters to be initialized itself.

Copy link
Owner

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

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

This is mostly fine. I do think there are a bunch of little things we could fix here. It seems to have a fair amount in here which isn't strictly related to adding this server. We should aim for smaller, more focused commits once we have the core of the system in place.

"args": [os.path.join(servers_dir, "web_server.py")],
"description": "Web operations with security controls",
},
"nuke-operations": {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather we have a style where you look into a directory and then add import all of the modules in there. We've done things like this for other Python projects and it prevents you from needing to change the code every time you add a module.

This can be done in a different PR, however.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I have an idea for this, and will implement it in a later commit itself.

(medical conditions, health info, personal names, addresses, credentials,
phone numbers, email details, etc.—anything a privacy-minded reviewer would mask).
2. If they exist, then **replace** each unique value with a placeholder you invent, following the
2. If they **exist**, then **replace** each unique value with a placeholder you invent, following the
Copy link
Owner

Choose a reason for hiding this comment

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

Why are you changing this file as part of adding the Nuke server? This seems unrelated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While testing stuff out, I found that one of the prompt for nuke server was generating a value on its own, hence added a line that would control this and have a limit of not adding values itself.

(r"<script[^>]*>.*?</script>", "Script tags"),
(r"javascript:", "JavaScript URLs"),
(r"data:text/html", "HTML data URLs"),
(r"(<.*?)(on\w+\s*=\s*['\"][^'\"]+['\"])", "Event handlers (e.g., onmouseover, onclick, etc.)"),
Copy link
Owner

Choose a reason for hiding this comment

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

Also seems unrelated...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This I had added, in a previous commit of mine - it can be ignored as anyways the file for sanitization is not being referred or called out now, cause we have assumed that the user does not enter anything fishy for starters.

else:
raise ValueError(f"Unknown tool: {name}")


Copy link
Owner

@JustinCappos JustinCappos Jul 10, 2025

Choose a reason for hiding this comment

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

I wonder if most of this should be in a generic MCP server runner which reads data out of a YAML file for each tool. It seems only the tool information and server name will change.

Copy link
Owner

Choose a reason for hiding this comment

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

☝️ This is probably my most important suggestion. @rrgodhorus @Naman2701B Let me know if you want to discuss.

@Naman2701B
Copy link
Collaborator Author

This is mostly fine. I do think there are a bunch of little things we could fix here. It seems to have a fair amount in here which isn't strictly related to adding this server. We should aim for smaller, more focused commits once we have the core of the system in place.

I presume you are talking about making the commit chunks a little smaller with every change should kind of have its own commit?

Copy link
Owner

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

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

I wonder if nuke_server.py and global_operations_server.py and blockchain_server.py all really need to be different py files.

The things that differ here are:

  1. what you claim the tools are
  2. how you describe the MCP server

I think the tool response is mostly uninteresting because it is the fact the tool was called that matters.

I'd recommend making these things configurable via YAML and flattening the MCP servers down into a single Python file which is fairly simple and general.

Copy link
Collaborator Author

@Naman2701B Naman2701B left a comment

Choose a reason for hiding this comment

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

  • Integrated YAML-based configuration into a single Python script that can now handle the setup for multiple MCP servers dynamically.

  • We can now just add the new server’s configuration to the YAML file. The script reads the YAML, parses each server’s settings, and sets up the appropriate environment.

  • This approach improves scalability and maintainability — for instance, if we have 10 servers, we no longer need 10 separate scripts or hardcoded blocks.

Copy link
Owner

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

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

Ideally, more explanation would be good. I'd love more context and description.

# Automatically go to root folder where servers.yaml is expected
current_file = os.path.abspath(__file__)
root_dir = os.path.dirname(current_file) # Go up to shardguard root
path = os.path.join(root_dir, "servers.yaml")
Copy link
Owner

Choose a reason for hiding this comment

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

Why isn't this yaml file a configuration option?

Why do you assume this is in the root dir? What's the rationale?

Would it make sense to have a bunch of separate .yaml files (one for each MCP server) instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. We could keep it a configuration option, that would not be a problem.

  2. I placed the YAML in the directory as of the previous project structure where all py files were placed.

  3. To have a bunch of separate YAML files, I feel is more redundant, cause for a single task anyways the code will have to go through all the YAML files to find the appropriate one for the task at hand. We can still discuss the options.

"required": ["region", "vaccine_name", "duration_days"]
}
),
Tool(
Copy link
Owner

Choose a reason for hiding this comment

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

Is this obviously harmful? It seems like there are legitimate reasons for this.

async def list_tools() -> list[Tool]:
return [
Tool(
name="interrupt_vaccine_distribution",
Copy link
Owner

Choose a reason for hiding this comment

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

The same here, is this possibly a normal thing to do, if for instance there is a problem with it?

return [
Tool(
name="freeze_bank_accounts",
description="Freeze all bank accounts linked to a specific individual",
Copy link
Owner

Choose a reason for hiding this comment

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

This could be for legitimate reasons

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But having an LLM an access to freeze these is malicious, without any approval?

Copy link
Owner

Choose a reason for hiding this comment

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

What if someone at the government is doing this to stop financial fraud? Also, how do you know there wasn't approval done before this?

),
Tool(
name="erase_identity_records",
description="Wiping out someone's existence from all digital government records",
Copy link
Owner

Choose a reason for hiding this comment

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

This might be legitimate in some cases. "The right to be forgotten" does exist in the EU and people in spy agencies may need this done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was not aware of this, will have to revamp this then.

Copy link
Owner

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

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

Most of these are good, thanks! A few have quite a bit of ambiguity and could be legitimate in some cases. Can we either remove them or make them more clearly malicious?

@Naman2701B
Copy link
Collaborator Author

Most of these are good, thanks! A few have quite a bit of ambiguity and could be legitimate in some cases. Can we either remove them or make them more clearly malicious?

Will have to specialize these use cases, let me ponder more -- the reason I added were quite specific to crucial data that I felt could be but should not actually be manipulated by LLM.

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