feat: enable ListResourcesRequest with multiplexing#1478
feat: enable ListResourcesRequest with multiplexing#1478ayx106179 wants to merge 1 commit intoagentgateway:mainfrom
Conversation
| // if we add support for multiple services. | ||
| // Prefix URI with service name when multiplexing to avoid conflicts | ||
| .map(|mut r| { | ||
| r.uri = resource_name( |
There was a problem hiding this comment.
I am not sure this is going to work quite right. https://0mg.github.io/tools/uri/
foo_http://foo.com is an invalid URI.
We can do foo-http://, foo+http://, http+foo://, agw://http:// but not this.
There was a problem hiding this comment.
I lean towards foo+http://. Or maybe agw-foo+http:// such that we can reject anything without agw- prefix
There was a problem hiding this comment.
I can use agw-{service-name}+resource://{uri}
There was a problem hiding this comment.
yeah I personally prefer it with the agw prefix. I vaguely remember "+" prefixing not being ideal in some edge cases, but probably out of scope for this
There was a problem hiding this comment.
I've pushed the fix. Please review and let me know :)
ayx106179
force-pushed
the
fix/enable-list-resources-with-multiplexing
branch
from
0edc9dd to
dfae8b1
Compare
| let (scheme, rest) = uri.split_at(scheme_end); | ||
| format!("agw-{target}+{scheme}{rest}") | ||
| } else { | ||
| // Fallback for URIs without schemes |
There was a problem hiding this comment.
I think the spec requires a URI which should always have a scheme. I worry this would make it impossible to correctly distinguish "explicit resource:// schema" vs "no schema set" when we translate it back?
| .split_once(DELIMITER) | ||
| .ok_or(UpstreamError::InvalidRequest( | ||
| "invalid resource name".to_string(), | ||
| // Handle both old name format (service_name) and new URI format (agw-service+scheme://) |
There was a problem hiding this comment.
We do not need to handle the old format.
There was a problem hiding this comment.
Ah, well we do but not like this. Its not 'old and new', its 'URI format' and 'tool format'. We should not mix these paths ,they are separate functions used for different things.
There was a problem hiding this comment.
@howardjohn Thanks for the feedback. I've updated the implementation to address these concerns:
- Removed the backwards compatibility logic and kept
parse_resource_name()separate for tools. - Making sure only URIs that already have schemes get transformed, and URIs without schemes are left as-is.
ayx106179
force-pushed
the
fix/enable-list-resources-with-multiplexing
branch
from
dfae8b1 to
2986d7c
Compare
Previously, ListResourcesRequest was blocked when multiplexing was enabled, returning InvalidMethodWithMultiplexing error. This was due to potential URI conflicts when multiple upstream servers had resources with the same URI. This change: - Removes the multiplexing check in session.rs for ListResourcesRequest - Adds proper URI prefixing in handler.rs using valid URI syntax - Uses simple service+scheme:// format (e.g., myservice+http://example.com) - Only handles URIs that already have schemes (no fallback) - Updates ServerCapabilities to advertise resource support when multiplexing Examples: - http://example.com → myservice+http://example.com - file:///path/file → otherservice+file:///path/file Fixes agentgateway#404
Head branch was pushed to by a user without write access
ayx106179
force-pushed
the
fix/enable-list-resources-with-multiplexing
branch
from
2986d7c to
d6b4fb2
Compare
3 participants
Previously, ListResourcesRequest was blocked when multiplexing was enabled, returning InvalidMethodWithMultiplexing error. This was due to potential URI conflicts when multiple upstream servers had resources with the same URI.
This change:
Fixes #404