Support ${headers.NAME} syntax to forward upstream API headers to toolsets#1725
Support ${headers.NAME} syntax to forward upstream API headers to toolsets#1725dgageot merged 1 commit intodocker:mainfrom
Conversation
There was a problem hiding this comment.
Review Summary
I've reviewed the changes for header placeholder support (${headers.NAME} syntax). The implementation looks solid overall, but I found a few issues worth noting around edge case handling and context propagation.
Found 3 issues:
- 1 edge case in placeholder parsing that could mask configuration errors
- 2 potential context propagation issues where placeholders may not resolve correctly
All issues are marked as MEDIUM severity - they represent potential problems in specific scenarios rather than critical bugs.
pkg/upstream/headers.go
Outdated
| end := strings.Index(v, "}") | ||
| if end == -1 { | ||
| b.WriteString(v) | ||
| break |
There was a problem hiding this comment.
Missing validation for empty header names in placeholders
The code calls upstream.Get(strings.TrimSpace(name)) after extracting the placeholder name. If the extracted name is empty or whitespace-only (e.g., ${headers.} or ${headers. }), TrimSpace returns "", and upstream.Get("") returns an empty string.
While this gracefully degrades by resolving to empty string (matching HTTP convention for missing headers), it could mask configuration errors. For example, a misconfigured placeholder like ${headers.} silently becomes an empty string instead of being flagged as malformed.
Suggestion: Consider adding validation that the header name is non-empty after trimming, or document this as intentional behavior:
name := strings.TrimSpace(v[len(prefix):end])
if name == "" {
// Either: return error, or document this returns empty string
}
b.WriteString(upstream.Get(name))Note: The test suite includes a test for "trimmed spaces in name" but doesn't cover the edge case of empty names after trimming.
| } | ||
|
|
||
| req.Header.Set("User-Agent", useragent.Header) | ||
| setHeaders(req, t.config.Headers) |
There was a problem hiding this comment.
Context may not contain upstream headers for placeholder resolution
The setHeaders function is called to resolve ${headers.NAME} placeholders from req.Context(). However, the context comes from the ctx parameter passed to callTool. If this tool is invoked outside the normal HTTP handler middleware chain (or if the upstream.Handler middleware wasn't applied), the context won't contain upstream headers and placeholders won't resolve.
Current flow:
- HTTP request →
upstream.Handlermiddleware → stores headers in context - Handler invokes tool via some chain →
callTool(ctx, ...) setHeaderstries to resolve placeholders fromctx
The implementation assumes the ctx parameter contains upstream headers, but this isn't validated or guaranteed.
Evidence: The test cases in api_test.go all use t.Context() (a plain testing context with no upstream headers) and don't test placeholder resolution.
Suggestion: Either:
- Add documentation that this tool requires contexts derived from HTTP requests processed by
upstream.Handler - Add integration tests showing placeholder resolution works through the full handler chain
- Add runtime validation that logs a warning when placeholders are used but upstream headers aren't available in context
| func setHeaders(req *http.Request, headers map[string]string) { | ||
| req.Header.Set("User-Agent", useragent.Header) | ||
| for k, v := range headers { | ||
| for k, v := range upstream.ResolveHeaders(req.Context(), headers) { |
There was a problem hiding this comment.
Context may not contain upstream headers for placeholder resolution
Similar to api.go, this code calls setHeaders(req, h.headers) which attempts to resolve ${headers.NAME} placeholders from req.Context(). The context is derived from the ctx parameter passed to callTool.
If the caller didn't propagate the context from an HTTP request that went through the upstream.Handler middleware, upstream headers won't be available in the context, and placeholders won't resolve.
Evidence: The test openapi_test.go:TestOpenAPITool_CustomHeaders shows static headers work, but there's no test validating ${headers.X} placeholder resolution works when invoked through an HTTP handler with the middleware chain.
Suggestion:
- Add integration tests that verify placeholder resolution through the full HTTP handler → tool invocation chain
- Document the requirement that tools using header placeholders must be invoked with contexts from HTTP requests
- Consider adding a helper that validates upstream headers are present when placeholders are detected in config
pkg/upstream/headers.go
Outdated
| } | ||
|
|
||
| // resolveValue replaces ${headers.NAME} in a single string value. | ||
| func resolveValue(v string, upstream http.Header) string { |
There was a problem hiding this comment.
I'll take a look. the agent didn't want to do it :-)
…lsets
Add a new pkg/upstream package that allows toolset header values to
reference incoming API request headers using ${headers.NAME} placeholders.
For example, a toolset config like:
headers:
Authorization: ${headers.Authorization}
will resolve the Authorization value at request time from the upstream
HTTP request that triggered the agent.
The middleware (Echo and ConnectRPC) stores the incoming request headers
in the context. At tool-call time, header values containing ${headers.X}
are resolved from that context. Static header values without placeholders
are unaffected.
Assisted-By: cagent
Allow toolset header values to reference incoming API request headers using ${headers.NAME} placeholders.
For example, a toolset config like:
will resolve the Authorization value at request time from the upstream HTTP request that triggered the agent.