Autopudate#136
Conversation
![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 renames MCP tool configurations to servers, removes the unused A2A service type, introduces a --trust-hub-rbac flag to conditionally apply baseline rules, and adds target service extraction to the authorization frame. Feedback on these changes suggests updating remaining variable names, error messages, and logs to reflect the 'server' renaming for consistency. Additionally, it is recommended to use separate timeout contexts for sequential DHT provide calls to prevent starvation, and to log the detailed authorizer state at a debug level rather than error level to avoid leaking sensitive information in production logs.
| for _, tool := range rolePolicy.MCP.AllowedServers { | ||
| if tool == "*" { | ||
| return fmt.Errorf("wildcard tool '*' is not allowed in role %q", role) | ||
| } |
There was a problem hiding this comment.
Since AllowedTools has been renamed to AllowedServers, the variable name tool and the error message should be updated to refer to server instead of tool for consistency and clarity.
| for _, tool := range rolePolicy.MCP.AllowedServers { | |
| if tool == "*" { | |
| return fmt.Errorf("wildcard tool '*' is not allowed in role %q", role) | |
| } | |
| for _, server := range rolePolicy.MCP.AllowedServers { | |
| if server == "*" { | |
| return fmt.Errorf("wildcard server '*' is not allowed in role %q", role) | |
| } | |
| } |
| for _, tool := range rolePolicy.MCP.AllowedServers { | ||
| if err := builder.AddAuthorityFact(biscuit.Fact{Predicate: biscuit.Predicate{ | ||
| Name: api.FactMCPTool, | ||
| Name: api.FactMCPServer, | ||
| IDs: []biscuit.Term{biscuit.String(tool)}, | ||
| }}); err != nil { | ||
| logger.Errorw("Failed to add MCP tool fact to biscuit", "peer_id", remotePeer, "tool", tool, "error", err) |
There was a problem hiding this comment.
Since FactMCPTool was renamed to FactMCPServer and AllowedTools was renamed to AllowedServers, the variable name tool and the log message should be updated to refer to server instead of tool.
| for _, tool := range rolePolicy.MCP.AllowedServers { | |
| if err := builder.AddAuthorityFact(biscuit.Fact{Predicate: biscuit.Predicate{ | |
| Name: api.FactMCPTool, | |
| Name: api.FactMCPServer, | |
| IDs: []biscuit.Term{biscuit.String(tool)}, | |
| }}); err != nil { | |
| logger.Errorw("Failed to add MCP tool fact to biscuit", "peer_id", remotePeer, "tool", tool, "error", err) | |
| for _, server := range rolePolicy.MCP.AllowedServers { | |
| if err := builder.AddAuthorityFact(biscuit.Fact{Predicate: biscuit.Predicate{ | |
| Name: api.FactMCPServer, | |
| IDs: []biscuit.Term{biscuit.String(server)}, | |
| }}); err != nil { | |
| logger.Errorw("Failed to add MCP server fact to biscuit", "peer_id", remotePeer, "server", server, "error", err) |
| if len(role.MCP.AllowedTools) != 1 || role.MCP.AllowedTools[0] != "query_database" { | ||
| t.Errorf("unexpected allowed tools: %v", role.MCP.AllowedTools) | ||
| if len(role.MCP.AllowedServers) != 1 || role.MCP.AllowedServers[0] != "query_database" { | ||
| t.Errorf("unexpected allowed tools: %v", role.MCP.AllowedServers) |
There was a problem hiding this comment.
The error message still refers to allowed tools instead of allowed servers. It should be updated to match the renamed field.
| t.Errorf("unexpected allowed tools: %v", role.MCP.AllowedServers) | |
| t.Errorf("unexpected allowed servers: %v", role.MCP.AllowedServers) |
| provideCtx, cancel := context.WithTimeout(ctx, 5*time.Second) | ||
| defer cancel() | ||
|
|
||
| if err := r.dht.Provide(provideCtx, srvNameCID, true); err != nil { | ||
| logger.Warnf("[ServiceRegistry] DHT Provide (name) for %s: %v", info.Name, err) | ||
| } | ||
| if err := r.dht.Provide(ctx, srvTypeCID, true); err != nil { | ||
| if err := r.dht.Provide(provideCtx, srvTypeCID, true); err != nil { | ||
| logger.Warnf("[ServiceRegistry] DHT Provide (type) for %s: %v", info.Name, err) | ||
| } |
There was a problem hiding this comment.
Using a single context with a 5-second timeout for both sequential Provide calls means that if the first call is slow or times out, the second call will be starved or automatically cancelled. It is safer to use a separate context and timeout for each Provide call.
| provideCtx, cancel := context.WithTimeout(ctx, 5*time.Second) | |
| defer cancel() | |
| if err := r.dht.Provide(provideCtx, srvNameCID, true); err != nil { | |
| logger.Warnf("[ServiceRegistry] DHT Provide (name) for %s: %v", info.Name, err) | |
| } | |
| if err := r.dht.Provide(ctx, srvTypeCID, true); err != nil { | |
| if err := r.dht.Provide(provideCtx, srvTypeCID, true); err != nil { | |
| logger.Warnf("[ServiceRegistry] DHT Provide (type) for %s: %v", info.Name, err) | |
| } | |
| provideCtx1, cancel1 := context.WithTimeout(ctx, 5*time.Second) | |
| if err := r.dht.Provide(provideCtx1, srvNameCID, true); err != nil { | |
| logger.Warnf("[ServiceRegistry] DHT Provide (name) for %s: %v", info.Name, err) | |
| } | |
| cancel1() | |
| provideCtx2, cancel2 := context.WithTimeout(ctx, 5*time.Second) | |
| if err := r.dht.Provide(provideCtx2, srvTypeCID, true); err != nil { | |
| logger.Warnf("[ServiceRegistry] DHT Provide (type) for %s: %v", info.Name, err) | |
| } | |
| cancel2() |
This completely reverts the renaming of find_remote_tools, call_remote_tool, and describe_remote_tool and their JSON fields back to using the 'tool' terminology because they specifically describe and interact with MCP Tools, preserving semantic correctness.
1 participant
No description provided.