Skip to content

feat: surface tool server errors in API and UI #420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: surface tool server errors in API and UI #420

wants to merge 3 commits into from

Conversation

zanuka
Copy link
Contributor

@zanuka zanuka commented May 22, 2025 •
edited
Loading

Summary:

  • enhances backend (HandleListToolServers in Go) to include a status.error field for each tool server, surfacing reconciliation and other errors from the controller
  • updated the UI to display these errors prominently in the tool server card, so users are aware of issues (e.g., failed auth, misconfiguration) directly in the tool server list
  • added status.error field to tool server API response for reconciliation and other errors
  • improves visibility of tool server issues for users

Note: @peterj I could use help testing error display in the UI. I wasn't able to add a tool server in localhost

@zanuka
feat: surface tool server errors in API and UI
30e5a39 
- Add status.error field to tool server API response for reconciliation and other errors
- Display error messages in the tool server card in the UI
- Improves visibility of tool server issues for users

Note: Need help testing error display in the UI
@zanuka zanuka mentioned this pull request May 22, 2025
Open
EItanya
EItanya reviewed May 22, 2025
View reviewed changes
go/controller/internal/httpserver/handlers/toolservers.go Outdated
toolServerWithTools = append(toolServerWithTools, map[string]interface{}{
errorMsg := getErrorFromConditions(toolServer.Status.Conditions)
if errorMsg != nil {
log.Info("Tool server has error condition", "toolServerName", toolServer.Name, "error", *errorMsg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a debug level log if anything. The information will already be returned by the handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed... since the error is now surfaced in the API response, the log line has been updated to use the debug level instead of info. have updated

go/controller/internal/httpserver/handlers/toolservers.go Outdated
Comment on lines 63 to 64
"conditions": toolServer.Status.Conditions,
"error": errorMsg,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just return the conditions and let the UI decide?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point... I added the error field to simplify error display in the UI, but can remove it and let the UI parse the conditions array instead. Let me know which approach you prefer.

@zanuka
Refactor invoke handler and tests for new agent invocation flow
34e2749 
- Updated InvokeHandler to use only GetTeamByID and InvokeTask, returning a consistent InvokeResponse.
- Updated tests to match new handler logic, including error handling for GetTeamByID and InvokeTask.
- Removed legacy tests for session and run creation errors, as these are no longer part of the handler flow.
- Fixed all linter and build errors; all tests now pass.
@zanuka
Copy link
Contributor Author

zanuka commented May 23, 2025

@EItanya The additional updates I've made include several improvements to the tool server functionality:

  1. Aligns backend and frontend data structures by including the config field in responses
  2. Implements consistent error handling across the tool server lifecycle (create, list, delete)
  3. Maintains backward compatibility while improving type safety and error reporting
  4. Updates tests to reflect the new data structure and error handling patterns
  5. Removes outdated tests that were no longer relevant to the current handler logic

this should ensure better type safety, more reliable error handling, and a more consistent API contract between the frontend and backend.

@peterj
Copy link
Collaborator

peterj commented May 23, 2025

I was trying to test it and create an MCP server (using the github one), but without providing the token. I see this in the controller logs:

{"controller": "toolserver", "controllerGroup": "kagent.dev", "controllerKind": "ToolServer", "ToolServer": {"name":"server-github","namespace":"kagent"}, "namespace": "kagent", "name": "server-github", "reconcileID": "758ac5c6-df24-4c40-a208-1e5b7a72d2dc", "error": "failed to reconcile tool server server-github: ToolServer.kagent.dev \"server-github\" is invalid: status.conditions: Required value"}

Also, not sure how this was working previously, but if the mcp server doesn't exist/not found (e.g. "npx something-that-doesn'texist"), the status is never set on the resource.

@zanuka
Copy link
Contributor Author

zanuka commented May 29, 2025

@peterj I need to unassign myself from this task. Just not enough time to continue contributing with my existing commitments. Feel free to update this branch or close PR as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EItanya EItanya EItanya left review comments

@peterj peterj Awaiting requested review from peterj peterj is a code owner

@ilackarms ilackarms Awaiting requested review from ilackarms ilackarms is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zanuka @peterj @EItanya