Add RemoteAgents to agent registry #15604
Conversation
…ote agent loading
Summary of ChangesHello @sehoon38, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the agent registry by introducing support for remote agents. It refactors the agent registration mechanism to be asynchronous, allowing for dynamic loading and description updates of agents from external URLs. The changes also extend the TOML configuration to define remote agents and include robust error handling to ensure system stability even if remote agent fetching encounters issues. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for remote agents, which are loaded via an agentCardUrl. A critical Server-Side Request Forgery (SSRF) vulnerability has been identified in the handling of these remote agent definitions, as user-controlled URLs are used without proper validation, potentially allowing access to internal network resources. It is highly recommended to implement a strict allow-list for URLs to mitigate this risk. Additionally, the review highlights the need to improve the robustness of the new TOML parsing logic, ensure consistent error handling, address an incorrect agent type inference, and align error reporting with the existing event-based feedback system.
| // Validate local agent requirements | ||
| if (!definition.agent_card_url && !definition.prompts?.system_prompt) { | ||
| throw new AgentLoadError( | ||
| filePath, | ||
| `Validation failed: Local agents must specify a 'prompts.system_prompt'.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The current validation for agent definitions is not strict enough and can allow ambiguous definitions. For example, it doesn't prevent a TOML file from having both agent_card_url and prompts.system_prompt. With the kind field now being the discriminator, the validation should be updated to enforce consistency.
// Validate agent definition based on its kind.
if (definition.kind === 'remote') {
if (!definition.agent_card_url) {
throw new AgentLoadError(
filePath,
`Validation failed: Agents with kind='remote' must specify 'agent_card_url'.`
);
}
} else { // 'local' or undefined
if (!definition.prompts?.system_prompt) {
throw new AgentLoadError(
filePath,
`Validation failed: Local agents must specify a 'prompts.system_prompt'.`
);
}
if (definition.agent_card_url && definition.kind !== 'local') {
throw new AgentLoadError(
filePath,
`Validation failed: 'agent_card_url' is present but 'kind' is not 'remote'. If this is a remote agent, please set kind = "remote".`
);
}
}| toml: TomlAgentDefinition, | ||
| ): AgentDefinition { | ||
| // If a model is specified, use it. Otherwise, inherit | ||
| if (toml.agent_card_url) { |
There was a problem hiding this comment.
The logic to determine if an agent is remote should be based on the kind field, not just the presence of agent_card_url. This makes parsing deterministic and avoids misinterpreting a local agent that might have a URL for documentation as a remote agent. This change should be made in conjunction with stricter validation in parseAgentToml.
if (toml.kind === 'remote') {| if (definition.kind === 'remote') { | ||
| try { | ||
| const clientManager = A2AClientManager.getInstance(); | ||
| const agentCard = await clientManager.loadAgent( | ||
| definition.name, | ||
| definition.agentCardUrl, | ||
| ); | ||
| if (agentCard.skills && agentCard.skills.length > 0) { | ||
| definition.description = agentCard.skills | ||
| .map((skill) => `${skill.name}: ${skill.description}`) | ||
| .join('\n'); | ||
| } | ||
| } catch (e) { | ||
| // Log the error, but don't block the CLI from starting. | ||
| console.error(`Error loading A2A agent "${definition.name}":`, e); | ||
| } | ||
| } |
There was a problem hiding this comment.
A Server-Side Request Forgery (SSRF) vulnerability exists in the handling of remote agent definitions. The agentCardUrl is loaded from a user-configurable TOML file and is used to make a network request without proper validation, potentially allowing an attacker to access internal network resources. It is recommended to implement a strict allow-list for URLs or ensure they resolve to public IP addresses. Additionally, the error handling for remote agent loading is inconsistent, using console.error instead of the centralized coreEvents.emitFeedback system, which should be used for all user-facing errors to ensure consistent reporting.
1 participant
…ote agent loading
Summary
Details
Related Issues
How to Validate
Pre-Merge Checklist