feat: add isError when error downstream response#301
Merged
iFurySt merged 9 commits intoAmoyLab:mainfrom Feb 28, 2026
Merged
Conversation
Contributor
Reviewer's GuideAdds error-aware tool call results by marking downstream HTTP error responses in MCP results and improves MCPConfig JSON unmarshalling error messages with contextual wrapping. Sequence diagram for TextHandler handling downstream HTTP response with isError flagsequenceDiagram
participant Client
participant TextHandler
participant HTTPClient as DownstreamHTTPService
participant MCP as MCPResultFactory
Client->>TextHandler: Handle(resp, tool, tmplConfig)
TextHandler->>HTTPClient: Perform HTTP request
HTTPClient-->>TextHandler: *http.Response resp
alt TemplateConfigured
TextHandler->>TextHandler: Render response body template
TextHandler-->>TextHandler: rendered string
else NoTemplate
TextHandler-->>TextHandler: use raw response body as rendered
end
TextHandler->>TextHandler: isError = resp.StatusCode >= 400
TextHandler->>MCP: NewCallToolResultTextWithError(rendered, isError)
MCP-->>TextHandler: *CallToolResult (Content, IsError)
TextHandler-->>Client: *CallToolResult
Class diagram for CallToolResult and error-aware text constructorclassDiagram
class CallToolResult {
[]Content Content
bool IsError
}
class Content {
}
class TextContent {
string Type
string Text
}
Content <|-- TextContent
class MCPFactory {
+NewCallToolResultText(text string) *CallToolResult
+NewCallToolResultTextWithError(text string, isError bool) *CallToolResult
}
MCPFactory ..> CallToolResult
MCPFactory ..> TextContent
class TextHandler {
+Handle(resp *http.Response, tool *ToolConfig, tmplConfig *TemplateConfig) (*CallToolResult, error)
}
TextHandler ..> MCPFactory
TextHandler ..> CallToolResult
class MCPConfigModel {
string Name
string Tenant
string Routers
string Servers
string Tools
string Prompts
string McpServers
+ToMCPConfig() (*MCPConfig, error)
}
class MCPConfig {
string Name
string Tenant
[]Router Routers
[]Server Servers
[]Tool Tools
[]Prompt Prompts
[]McpServer McpServers
}
MCPConfigModel ..> MCPConfig : ToMCPConfig
MCPConfigModel ..> Router
MCPConfigModel ..> Server
MCPConfigModel ..> Tool
MCPConfigModel ..> Prompt
MCPConfigModel ..> McpServer
class Router
class Server
class Tool
class Prompt
class McpServer
class ErrorWrappingHelper {
+wrapError(context string, err error) error
}
MCPConfigModel ..> ErrorWrappingHelper
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the new
wrapErrorhelper, the format string and argument order don’t match the intended message ("failed to unmarshal MCP %s configuration '%s' (tenant: '%s')"currently usesm.Name, m.Tenant, contextbut reads as if it should bem.Name, context, m.Tenant). - To avoid duplicating logic, consider having
NewCallToolResultTextWithErrordelegate toNewCallToolResultTextand then setIsError, instead of rebuilding theContentslice manually.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new `wrapError` helper, the format string and argument order don’t match the intended message (`"failed to unmarshal MCP %s configuration '%s' (tenant: '%s')"` currently uses `m.Name, m.Tenant, context` but reads as if it should be `m.Name, context, m.Tenant`).
- To avoid duplicating logic, consider having `NewCallToolResultTextWithError` delegate to `NewCallToolResultText` and then set `IsError`, instead of rebuilding the `Content` slice manually.
## Individual Comments
### Comment 1
<location path="internal/mcp/storage/model.go" line_range="41" />
<code_context>
}
+ wrapError := func(context string, err error) error {
+ return fmt.Errorf("failed to unmarshal MCP %s configuration '%s' (tenant: '%s'): %w", m.Name, m.Tenant, context, err)
+ }
if len(m.Routers) > 0 {
</code_context>
<issue_to_address>
**issue (bug_risk):** The error message format string appears to mismatch the argument order, which can lead to confusing logs.
The format string `"failed to unmarshal MCP %s configuration '%s' (tenant: '%s'): %w"` with arguments `(m.Name, m.Tenant, context, err)` currently renders as “MCP <Name> configuration '<Tenant>' (tenant: '<context>')”, which mislabels the tenant/context. Please reorder placeholders or arguments so that `context` describes what failed, `m.Name` is the MCP name, and `m.Tenant` is the tenant—for example: `"failed to unmarshal MCP configuration '%s' (%s, tenant: '%s'): %w", m.Name, context, m.Tenant, err`.
</issue_to_address>
### Comment 2
<location path="pkg/mcp/server_types.go" line_range="398-408" />
<code_context>
+// @param text the text content
+// @param isError indicates if the result is an error
+// @return *CallToolResult the CallToolResult object with the text content and error flag
+func NewCallToolResultTextWithError(text string, isError bool) *CallToolResult {
+ return &CallToolResult{
+ Content: []Content{
+ &TextContent{
+ Type: TextContentType,
+ Text: text,
+ },
+ },
+ IsError: isError,
+ }
+}
</code_context>
<issue_to_address>
**suggestion:** NewCallToolResultTextWithError duplicates construction logic already present in NewCallToolResultText.
To avoid duplication and keep constructors aligned, you could delegate to `NewCallToolResultText` and then set `IsError`:
```go
func NewCallToolResultTextWithError(text string, isError bool) *CallToolResult {
res := NewCallToolResultText(text)
res.IsError = isError
return res
}
```
This keeps text result construction centralized for future changes.
```suggestion
func NewCallToolResultTextWithError(text string, isError bool) *CallToolResult {
res := NewCallToolResultText(text)
res.IsError = isError
return res
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
iFurySt
approved these changes
Feb 28, 2026
Member
|
LGTM. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Propagate HTTP error states from downstream responses into MCP tool results while improving MCP configuration unmarshalling errors.
New Features:
Enhancements: