feat(a2a): implement secure acknowledgment and core registry improvements#21404
feat(a2a): implement secure acknowledgment and core registry improvements#21404alisa-alisa wants to merge 3 commits intomainfrom
Conversation
|
Hi @alisa-alisa, thank you so much for your contribution to Gemini CLI! We really appreciate the time and effort you've put into this. We're making some updates to our contribution process to improve how we track and review changes. Please take a moment to review our recent discussion post: Improving Our Contribution Process & Introducing New Guidelines. Key Update: Starting January 26, 2026, the Gemini CLI project will require all pull requests to be associated with an existing issue. Any pull requests not linked to an issue by that date will be automatically closed. Thank you for your understanding and for being a part of our community! |
|
This PR is a part of split: #21348 |
Summary of ChangesHello, 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 AgentRegistry by introducing a more robust and secure acknowledgement mechanism for remote agents and improving the accuracy of agent indexing. The changes aim to safeguard against indirect prompt injection by using content-based hashing for remote agent cards and ensure that agents, including those with overridden names, are correctly discovered and managed within the system. Additionally, it streamlines the handling of unacknowledged project agents, making them visible to the LLM while maintaining necessary user approval gates. Highlights
Changelog
Activity
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
|
0baaf47 to
a0e1717
Compare
d1a90c2 to
7e03bc0
Compare
a0e1717 to
6047d93
Compare
489fa40 to
9b131e2
Compare
1b9ccf8 to
6453c5f
Compare
1817c62 to
c61ed08
Compare
a3ef0a5 to
aca4e49
Compare
c61ed08 to
15156e3
Compare
aca4e49 to
5d088c8
Compare
15156e3 to
51aa3cf
Compare
5d088c8 to
6576088
Compare
51aa3cf to
2104246
Compare
6576088 to
e8be953
Compare
11f1e5a to
ec8bf97
Compare
0abe06e to
f40f810
Compare
861ccac to
16c5c0b
Compare
f40f810 to
62d0ecf
Compare
62d0ecf to
7f2ab59
Compare
7f2ab59 to
9f5c35d
Compare
|
Size Change: +568 B (0%) Total Size: 26.1 MB
ℹ️ View Unchanged
|
adamfweidman
left a comment
There was a problem hiding this comment.
YOLO mode already works when using gemini-cli. If this doesn't work with the a2a server, the a2a-server itself probably doesn't have the policyEngine correctly configured. Can we make changes there rather than here?
| ): Promise<AgentCard> { | ||
| if (this.clients.has(name) && this.agentCards.has(name)) { | ||
| throw new Error(`Agent with name '${name}' is already loaded.`); | ||
| const existingCard = this.agentCards.get(name); |
There was a problem hiding this comment.
What happens if multiple agents try to get registered with the same name?
There was a problem hiding this comment.
They will clash. See my reply above.
There was a problem hiding this comment.
right. I think the fix here is just to check name and url and only throw if both don't overlap
| hash: string, | ||
| ): Promise<boolean> { | ||
| await this.load(); | ||
| return this.isAcknowledgedSync(projectPath, agentName, hash); |
There was a problem hiding this comment.
nit: unnecessary helper function
There was a problem hiding this comment.
It is used below.
There was a problem hiding this comment.
Sorry, Unnecessary not used. It's defined and called in one place right above it (function is only 2 lines) so stylistically I think it's not necessary. Wdyt.
| if (isAcknowledged) { | ||
| agentsToRegister.push(agent); | ||
| } else { | ||
| agentsToRegister.push(agent); |
There was a problem hiding this comment.
shouldn't we only push if this isAcknowledged first? Without acknowldgment we allow arbitrary execution of code to get the necessary token on startup.
There was a problem hiding this comment.
We change it in this PR.
With the old logic, I can't use an agent until I trust it, but I can't trust it (via the UI) until the system shows it to me. For a CLI user, this is fine because they see a "New agents discovered" notification. But for curl or the A2A server, the tool just appears to be missing.
We need to work around that somehow, the PR is my workaround that. Do you have another preference for the flow?
Summary:
New Logic: "I see a new agent. It’s untrusted, so I'll show it to you but ask for permission before running it." -> my example works.
Old Logic: "I see a new agent. It’s untrusted, so I'll pretend it doesn't exist until you go into the UI and manually approve it." -> my example fails with 'Tool Not Found'.
| const isYolo = this.config.getApprovalMode() === ApprovalMode.YOLO; | ||
| const isAcknowledged = | ||
| definition.kind === 'local' && | ||
| (!definition.metadata?.hash || |
There was a problem hiding this comment.
What is definition.metadata?.hash. Lets rely on previous logic.
There was a problem hiding this comment.
In the old logic, if an agent wasn't trusted, it wasn't even added to the tool list. It was invisible until we acknowledged it. This is the new logic that checks the new stuff.
hash is just additional layer to verify the version of the agent that was trusted (fingerprint of the file content) meaning "I trust this agent" ="I trust this specific version of the agent.".
I can remove it and just trust w/e is there? Even if the file is changed?
There was a problem hiding this comment.
One way to refactor can be:
const isAcknowledged = definition.kind === 'local' &&
ackService.isAcknowledgedSync(project, definition.name, definition.metadata?.hash);
(move the check). wdyt?
There was a problem hiding this comment.
The current behavior is correct — unacknowledged agents shouldn't be registered at all, which is already handled in loadAgents(). This check in addAgentPolicy() is only needed because the PR changes the upstream filtering to register unacknowledged agents, which we shouldn't do (it triggers network requests to untrusted URLs for remote agents). Let's revert the loadAgents() change and drop this block.
There was a problem hiding this comment.
Please help me to understand how to make it work in a2a without any changes. Because the existing approach will not work.
| 'TestAgent', | ||
| 'http://test.agent/card', | ||
| ); | ||
| const card2 = await manager.loadAgent( |
There was a problem hiding this comment.
I think this should throw, people could have agents defined across different projects with names that clash.
There was a problem hiding this comment.
The A2A server uses a singleton A2AClientManager (to reuse gRPC/HTTP connections across the whole process), but it creates a fresh AgentRegistry for every single task to ensure context isolation.
Basically...it will throw non-stop
We can make the manager URL-aware? In addition to name? Will this work?
|
Brand new idea: #22389 |
Summary
Building on the recently submitted Branch 3 infrastructure, this PR enhances the A2A and general agent discovery systems with robust trust validation, improved registry policies, and idempotent client management.
File-based Changes
packages/core/src/agents/acknowledgedAgents.tsisAcknowledgedSyncmethod.registry.tsloadAgentsto register all discovered agents, regardless of trust status.ASK_USERpolicy for unacknowledged agents usingisAcknowledgedSync.GEMINI_YOLO_MODE(ApprovalMode.YOLO) check.ALLOW.a2a-client-manager.tsloadAgentidempotent.packages/a2a-server/src/config/settings.tsexperimental.enableAgentsto theSettingsinterface..gemini/settings.json.config.tsloadConfigto propagate theenableAgentsflag from settings to the core registry..gemini/agentsdirectory, rather than being restricted to its internal Coder Agent.Testing
Automated Tests
acknowledgedAgents.test.ts: Added tests forisAcknowledgedSyncfunctionality.registry_acknowledgement.test.ts: Verified that unacknowledged agents are registered withASK_USERand transitioned toALLOWonce trusted.a2a-client-manager.test.ts: Updated tests to verify thatloadAgentis idempotent and correctly returns cached instances.registry.test.ts: Added verification for remote agent registration in YOLO mode.Manual Test (gRPC V0 Compatibility)
Prepare the Go Server (a2a-go repo)
examples/helloworld/server/grpc/main.go."0.1"ina2a/core.go.go run examples/helloworld/server/grpc/main.go(listens on port 9001).Prepare the Gemini CLI
.gemini/agents/grpc-test-agent.mdwith URL:http://localhost:9001/.well-known/agent-card.json."enableAgents": truein theexperimentalblock of.gemini/settings.json.Verify
Related Issues
Closes #22199