Hide HTTP routers from agent tooling - #3960
Conversation
Amp-Thread-ID: https://ampcode.com/threads/T-01a0d31a-3a14-7359-ab9c-15b70e1d918e Co-authored-by: Amp <amp@ampcode.com>
✅ Deploy Preview for golemcloud canceled.
|
| .agents | ||
| .keys() | ||
| .filter_map(|agent_name| self.registered_agent_types.get(agent_name)) | ||
| .filter(|agent| { |
There was a problem hiding this comment.
Could we exclude (or explicitly reject)
HttpRouterentries at the start of MCP compilation, rather than only excluding their method names from collision detection? They are still pushed intoregistered_agent_types, and theirsecurity_schemestill participates inunique_scheme_names. A hidden router can therefore make an otherwise valid native-tool deployment fail withMcpDeploymentConflictingSecuritySchemes, or make a router-only MCP deployment pass the non-empty check and expose an empty server. Filtering the effective capability set before the empty/security checks would keep deployment behavior aligned with the runtime visibility change.
| repl_language: GuestLanguage, | ||
| targets: &[BridgeSdkTarget], | ||
| ) -> BTreeMap<GuestLanguage, ReplMetadata> { | ||
| let mut repl_metadata_by_language = BTreeMap::from([(repl_language, ReplMetadata::default())]); |
There was a problem hiding this comment.
This empty metadata entry is written by the new build-plan executor, but
gen_bridge_with_manifest_mode_filter_and_additional_collision_targetsstill returns whenplan.targetsis empty before callingwrite_repl_metadata. A router-only plan through the exportedgen_bridge/gen_external_bridgepath therefore leaves stale REPL metadata behind. Could we either write the metadata before that return or remove those older wrappers if they are no longer supported, and cover stale-file replacement at the entrypoint level?
| implemented_by: test_implementer(), | ||
| webhook_domain_and_segments: None, | ||
| }; | ||
| let definition = executable_test_tool("foo-bar", "baz"); |
There was a problem hiding this comment.
This regression currently does not exercise the new collision filter: the router capability name is
foo-bar_baz, while the native export asserted below isfoo_bar_baz, so the test succeeds even if theHttpRouterfilter is removed. Could we construct a case that actually fails without the filter, or, if native and agent naming make cross-kind collisions impossible, remove or simplify the dead collision check and test the intended invariant directly?
| } | ||
| let repl_language = target | ||
| .target_language | ||
| .expect("REPL bridge target requires a target language"); |
There was a problem hiding this comment.
Could we avoid the two new
expect("REPL bridge target requires a target language")calls? Today every production REPL target comes fromnew_repl_bridge_sdk_target(language), so the invariant holds, but it is not expressed byCustomBridgeSdkTarget:target_languageremains optional because the same type also represents ordinary custom targets, and both planning functions accept that weaker type. A future internal misrouting would therefore turn into a CLI panic. Either a dedicated REPL target type with a required language, or a contextualanyhowerror at these boundaries, would preserve the invariant without a panic.
Amp-Thread-ID: https://ampcode.com/threads/T-01a0d31a-3a14-7359-ab9c-15b70e1d918e Co-authored-by: Amp <amp@ampcode.com>
…ooling-visibility
Summary
Verification
bun run --cwd golem-service-base/tests/fixtures/http-handlers testgolem-cli,golem-worker-service, andgolem-registry-servicetestscargo fmt -p golem-cli -p golem-worker-service -p golem-registry-service -- --checkCARGO_INCREMENTAL=0 cargo clippy -p golem-cli -p golem-worker-service -p golem-registry-service --all-targets --no-deps -- -D warningsResolves GOL-569