pull tool annotations onto CR for visibility #116
Closed
Conversation
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
dexhorthy
commented
May 21, 2025
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 25d8c50 in 2 minutes and 7 seconds. Click for details.
- Reviewed
367lines of code in10files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. acp/internal/mcpmanager/mcpmanager.go:159
- Draft comment:
Avoid using fmt.Printf for error reporting; use a structured logger (or controller-runtime's logging) for better log integration. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. acp/internal/mcpmanager/mcpmanager.go:169
- Draft comment:
Consider using the manager's logging facility instead of fmt.Printf for cleanup errors in the error branches when closing the client. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. acp/internal/mcpmanager/mcpmanager_test.go:472
- Draft comment:
Consider expanding unit tests to simulate error cases in ConnectServer (e.g., secret resolution failures) to improve coverage of error paths. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. acp/config/samples/mcp_github.yaml:1
- Draft comment:
There's a trailing whitespace after the apiVersion value on line 1. Please remove the extra space to keep the YAML clean. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While trailing whitespace is generally undesirable, it's an extremely minor issue. Most modern editors automatically strip trailing whitespace. This kind of nitpick doesn't require human attention and could be better handled by automated formatting tools or pre-commit hooks. It doesn't affect functionality or readability. The trailing whitespace could cause issues with some YAML parsers or diff tools. It's a valid style consistency concern. While technically correct, this is too minor of an issue to warrant a PR comment. If whitespace is important, it should be enforced through automation rather than manual review. Delete this comment as it's too trivial and would be better handled by automated tooling.
Workflow ID: wflow_tjZ1N0jD16JNNi7v
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| } | ||
|
|
||
| // Create annotations based on tool properties | ||
| annotations := &acp.MCPToolAnnotations{ |
Contributor
There was a problem hiding this comment.
Potential nil pointer dereference: tool.Annotations is used without checking if it's nil. Consider adding a nil check before accessing its fields.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Important
Add annotations to MCP tools for metadata about behavior, update CRD, deepcopy functions, and tests, and update dependencies.
MCPToolAnnotationsstruct inmcpserver_types.goto provide metadata about tool behavior.MCPToolstruct to includeAnnotationsfield inmcpserver_types.go.acp.humanlayer.dev_mcpservers.yamlto include annotations properties.DeepCopyfunctions forMCPToolAnnotationsinzz_generated.deepcopy.go.DeepCopyIntoforMCPToolto handleAnnotationsinzz_generated.deepcopy.go.mcpserver_controller_test.go.MockMCPClientinmcpmanager_test.goto handle annotations.go.modandgo.sumto usegithub.com/mark3labs/mcp-go v0.29.0and other dependency updates.This description was created by
for 25d8c50. You can customize this summary. It will automatically update as commits are pushed.