BobSong-dev commented on PR #7512:
URL: https://github.com/apache/shenyu/pull/7512#issuecomment-6072045477
> Hi, @BobSong-dev, thanks for your PR. This is a substantial but carefully
bounded feature; my comments are mostly design questions plus a handful of
nits.嗨，感谢您的公关工作。这个功能虽然功能强大，但设计上也有些不足之处；我的意见主要集中在设计方面的问题上，还有几处需要改进的地方。
>
> #### Verification 验证
> On `8169dbdb5` (merge base `d462b0079`):在 `8169dbdb5` （合并基础分支 `d462b0079`
）上：
>
> * `./mvnw -pl shenyu-plugin/shenyu-plugin-agent-gateway -am test` -> 287
tests, 0 failures, including the new aggregation, remote-client, TLS,
credential-resolver, transport-policy and catalog-lifecycle suites. `./mvnw -pl
shenyu-plugin/shenyu-plugin-agent-gateway -am test` -> 进行了 287
次测试，没有出现任何失败情况。包括新的聚合功能、远程客户端、TLS 协议、凭证解析器、传输策略以及目录生命周期相关功能在内的所有测试环节均已完成。
> * `-pl shenyu-admin -am
-Dtest='AgentGatewayPluginConfigValidatorTest,NamespacePluginServiceTest,PluginServiceTest'`,
`-pl shenyu-common -Dtest=AgentGatewayAggregationConfigTest` and the starter's
`AgentGatewayPluginConfigurationTest` all pass. `-pl shenyu-admin -am
-Dtest='AgentGatewayPluginConfigValidatorTest,NamespacePluginServiceTest,PluginServiceTest'`
、 `-pl shenyu-common -Dtest=AgentGatewayAggregationConfigTest` 以及启动器
`AgentGatewayPluginConfigurationTest` 都通过了测试。
> * `checkstyle:check` passes for all four touched modules (`shenyu-common`,
`shenyu-admin`, `shenyu-plugin-agent-gateway`, the starter). `checkstyle:check`
通过了所有四个被触碰的模块 ( `shenyu-common` 、 `shenyu-admin` 、
`shenyu-plugin-agent-gateway` 以及启动模块)。
>
> #### Notes on the design
> 关于设计的说明
> * The trust boundary is drawn where it should be: the endpoint allowlist
and credential directory come from deployment properties and never from Admin
or the Agent; the plugin config is secret-free; `validateEndpoint` (literal
IPv4, normalized URI, no userinfo/query/fragment, https or http only for
127.0.0.1, link-local rejected) plus the per-target allowlist check removes the
SSRF surface that "aggregate a remote MCP server" usually brings; a credential
file is bound to its exact target and confined to the real mounted directory;
outbound requests carry only the service token (never Agent identity or inbound
metadata) and upstream result metadata is validated without being
re-interpreted.信任边界被绘制在正确的位置：端点允许列表和凭证目录来自部署属性，而非来自管理员或代理程序；插件配置完全保密；地址格式包括直接指定的
IPv4 地址、规范化的 URI，没有用户信息、查询参数或片段，只有针对 127.0.0.
1 的 https 或 http 功能，链接本地地址则被排除在外。此外，针对每个目标的允许列表检查消除了可能导致 SSRF 攻击的漏洞，因为“聚合远程
MCP
服务器”通常会引发此类问题；凭证文件与特定目标绑定在一起，并且仅限于实际的挂载目录中；出站请求仅携带服务令牌（从不携带代理身份或入站元数据），而上游结果的元数据在传输过程中不会被重新解析。
> * The budgets are enforced consistently (8 servers, 4 pages, 16 tools, 8
KiB request/response, 64 in-flight, per-phase timeouts), generations are leased
per operation, and cleanup failures stay quarantined rather than silently
reusing a half-closed client. The README documents the contract and the
deliberate fail-closed behaviour, which makes this much easier to
review.这些预算得到了一致的执行（8 个服务器，4 页文档，16 种工具，8KiB 的请求/响应数据量，64
个连接，每个阶段都有超时限制）。各个版本都是按照独立操作来租用的，而处理失败的情况时，系统会将其隔离处理，而不是默默重新使用已经部分关闭的客户端。README
文件详细描述了相关协议以及这种故意采用故障关闭模式的特性，这使得系统更易于审核。
>
> #### Possible Improvements 可能的改进之处
> * Scope of fail-closed: with the catalog enabled,
`AgentMcpDispatcher.dispatch` routes every MCP request through `withSnapshot`,
and `RemoteCatalogLifecycle.reserve()` clears `current` before the candidate is
built. Any rejected update (missing credential file, unreachable target,
discovery timeout, page/tool limit) therefore leaves `current == null` and
returns a generic 500 `Internal error` for every MCP request - including calls
to purely local tools that have nothing to do with aggregation. The README
frames it as "remote access unavailable until an explicit successful update",
but the blast radius here is the whole MCP entry. Would degrading to local-only
(serve the last good generation, or fall back to an empty remote directory) be
acceptable, with the remote failure surfaced through diagnostics or a distinct
error code? At minimum a distinct error code would let operators tell "remote
catalog unavailable" apart from "gateway bug".关闭失败的范围：在启用目录功
能的情况下， `AgentMcpDispatcher.dispatch` 的每个 MCP 请求都会通过 `withSnapshot`
进行处理，而在构建候选路径之前， `RemoteCatalogLifecycle.reserve()` 会清除 `current`
。因此，任何失败的更新操作（如缺少凭证文件、目标不可达、发现超时、页面/工具限制等）都会使 `current == null` 处于无效状态，并且每个 MCP
请求都会返回通用的 500 错误代码——这包括那些与聚合无关、完全基于本地工具的操作。README
文件中将这种情况描述为“在明确成功的更新完成之前，远程访问不可用”，但这里的故障影响范围实际上是整个 MCP
条目。是否可以将性能降级为仅限本地操作（仅提供最近一次有效的版本，或回退到空的远程目录），同时让远程故障能够通过诊断工具或明确的错误代码来显示？至少，一个明确的错误代码应该能让操作员区分“远程目录不可用”和“网关故障�
��
> * `AgentGatewayPluginConfiguration` keeps a public
`agentGatewayPlugin(providers, resolvers)` overload that is no longer annotated
`@Bean` and is not called anywhere (the starter tests go through the context or
the private `createPlugin`). It reads like a leftover from the refactor -
please delete it or make it the single private assembly path.
`AgentGatewayPluginConfiguration` 保留了一个公共的 `agentGatewayPlugin(providers,
resolvers)` 重载函数，该函数已经不再被注释标记，并且也没有在任何地方被调用（启动测试时，该函数会经过上下文或私有的 `createPlugin`
路径）。看起来这像是重构过程中的遗留问题——请删除它，或者将其改为唯一的私有汇编路径。
> * The outbound header `X-Spike-Subject` (`RequestScopedMcpClient:204`,
asserted at `RequestScopedMcpClientTest:269`) looks like an experiment leftover
and is not part of the documented outbound contract. If it is the agreed
handshake, please document it next to the other headers; otherwise an
`X-Shenyu-*` name would avoid the "spike" connotation in a protocol other teams
have to implement.出站头部 `X-Spike-Subject` ( `RequestScopedMcpClient:204` )，在
`RequestScopedMcpClientTest:269`
处被设置为真值，看起来像是实验遗留下来的东西，并不属于已文档化的出站协议规范。如果这是经过协商确定的握手过程，请将其与其他头部一起记录在文档中；否则，使用
`X-Shenyu-*` 这样的名称可以避免在协议中出现“峰值”现象，因为其他团队也需要实施类似的协议。
> * `RemoteToolDirectory:127` carries the comment "User confirmed a stable
namespace for every remote tool on 2026-10-05". That is a conversation artifact
rather than a rationale; reword it to state the naming rule and why the
dot-namespace was chosen. `RemoteToolDirectory:127` 中包含了这样的注释：“用户在 2026-10-05
确认了每个远程工具的稳定命名空间”。这更像是一个对话中的记录，而不是具体的理由；请重新表述这条注释，明确说明命名规则以及为何选择使用点-命名空间。
> * `RemoteServerBinding.newClient()` passes `ignored -> { }` as the
observer, so the per-response diagnostic map built in
`RequestScopedMcpClient.decodeResponse` is discarded in production. It already
contains only server/method/id/status/size, so wiring it to a debug logger (or
dropping the parameter/observer pair) would remove a false sense of
observability. `RemoteServerBinding.newClient()` 将 `ignored -> { }`
作为观察者进行传递，因此 `RequestScopedMcpClient.decodeResponse`
中内置的每响应诊断映射在运行时会被忽略。由于该映射仅包含服务器/方法/ID/状态/大小等参数，因此将其连接到调试日志器（或移除参数/观察者的配对）就能消除对可观察性的过度需求。
> * The `LocalTool`/`withLocal` path is not used by the production wiring
(`ManagedRemoteMcpCatalog.reload` passes `List.of()`), so it is currently only
exercised by tests. Either wire it or state in the javadoc that it is the
extension point for native-result local providers.路径 `LocalTool` / `withLocal`
并未被生产环境使用（ `ManagedRemoteMcpCatalog.reload` 通过了 `List.of()`
的测试），因此目前仅通过测试来验证其功能。请确保在 javadoc 中明确说明这是原生结果本地提供器的扩展点。
> * Plugin-config compatibility: `parsePluginConfig` rejects any top-level
field other than `aggregation`, and admin now validates the agent-gateway
plugin's config on create/update/import. The plugin had no config contract
before, so an operator who stored something else in that plugin's config will
get "Invalid Agent Gateway aggregation configuration" the next time they save
the plugin - even for an unrelated change such as toggling it. Empty and blank
config stay valid, so a line in the README/release notes is probably enough,
but it is a user-visible behaviour change worth calling out.插件配置兼容性：
`parsePluginConfig` 拒绝除 `aggregation`
之外的任何顶层字段。现在，在创建、更新或导入插件配置时，系统会进行验证。在此之前，插件本身并没有配置相关的约束条件。因此，如果操作员在插件配置中写入了其他内容，下次保存插件时将会出现“无效的代理网关聚合配置”错误——即使
只是进行了与配置无关的操作，比如切换选项。不过，空配置仍然有效，所以《README》或发布说明中的一行说明就足够了。不过，这一对用户可见的行为变化确实值得关注。
> * Style: checkstyle passes, but the new code is unusually heavy on inline
fully-qualified names where the file already imports siblings -
`java.util.Objects` and `com.fasterxml.jackson.*` throughout
`RemoteCatalogLifecycle`/`RemoteToolDirectory`, and
`org.apache.shenyu.admin.validation.validator.AgentGatewayPluginConfigValidator.validate(...)`
three times in `PluginServiceImpl`. Importing them, as the surrounding code
does, would read better. `RemoteToolDirectory:37-40` also has a leftover
blank-line block.风格方面：checkstyle 通过了，但是新代码中有很多内联完全限定的命名方式，而文件本身就已经导入了多个类似的类——在
`RemoteCatalogLifecycle` 、 `RemoteToolDirectory` 以及 `PluginServiceImpl`
中分别出现了三次 `java.util.Objects` 和 `com.fasterxml.jackson.*`
的命名。像周围代码那样导入这些类会让代码更易于阅读。此外， `RemoteToolDirectory:37-40` 中还有一个未处理的空行块。
Thanks for your carefully review, I will fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
