Approve/Rejection Flow #46
Conversation
|
first impression - add a section to the getting started guide in the readme so I know how to play with this! |
kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go
Outdated
Show resolved
Hide resolved
kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go
Show resolved
Hide resolved
kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go
Outdated
Show resolved
Hide resolved
| if needsApproval && trtc.Status.Status == kubechainv1alpha1.TaskRunToolCallStatusTypeAwaitingHumanApproval { | ||
| client := r.HLClient.NewHumanLayerClient() | ||
| client.SetCallID(trtc.Status.HumanLayerCallId) | ||
| client.SetAPIKey(apiKey) |
There was a problem hiding this comment.
I think the way they want you to do this might be
functionCall, _, _ := r.HLClient.NewHumanLayerClient().
SetCallID(...).
SetAPIKey(...).
GetFunctionCallStatus(ctx)they might be wrong though. nothing wrong with this pattern here
kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go
Outdated
Show resolved
Hide resolved
kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go
Outdated
Show resolved
Hide resolved
kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go
Outdated
Show resolved
Hide resolved
kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go
Outdated
Show resolved
Hide resolved
kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| if approvalGranted { | ||
| trtc.Status.Status = kubechainv1alpha1.TaskRunToolCallStatusTypePending |
There was a problem hiding this comment.
i notice we're updating fields on the status directly, most other controllers do a deepCopy at the top and call it statusUpdate - did I miss where that happens here?
I am not 100% on why, but the default templates do this so I assume there's a good reason for it. Something to do with conflicts or something
kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go
Outdated
Show resolved
Hide resolved
5fe7709 to
6986b59
Compare
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 344ed0e in 2 minutes and 48 seconds
More details
- Looked at
1261lines of code in11files - Skipped
0files when reviewing. - Skipped posting
14drafted comments based on config settings.
1. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go:174
- Draft comment:
The new pattern of returning (bool, error, bool) in functions like initializeTRTC and checkCompletedOrExisting is not self‑documenting. Consider replacing the triple return values with a dedicated struct for clarity. - 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. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go:685
- Draft comment:
Great use of DeepCopy in setStatusError; however, ensure consistency across functions (e.g. updateTRTCStatus) by always operating on copies to avoid unexpected mutation. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%
None
3. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go:820
- Draft comment:
In requestHumanApproval, the HLClientFactory availability check is a good guard; consider adding a similar consistency check or deep copy usage when updating status after posting to HumanLayer (see postToHumanLayer). - Reason this comment was not posted:
Comment looked like it was already resolved.
4. kubechain/internal/humanlayer/mock_hlclient.go:117
- Draft comment:
Mock implementations in RequestApproval and GetFunctionCallStatus correctly simulate approval/rejection. Verify that tests cover cases where neither approval nor rejection are set (i.e. pending), to ensure robustness. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
5. kubechain/internal/humanlayer/hlclient.go:140
- Draft comment:
The RealHumanLayerClientWrapper implementations correctly assemble the request. Confirm that error handling from the API client is sufficient and that the API base URL logic falls back to a default when not provided. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
6. kubechain/internal/humanlayer/hlclient.go:140
- Draft comment:
Consider validating that the API key is not empty before issuing the RequestApproval call. An empty key may lead to unexpected authorization behavior. - 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.
7. kubechain/internal/humanlayer/mock_hlclient.go:129
- Draft comment:
Document behavior when neither approval nor rejection flags are set. Callers may receive a nil response with an error code; clarify this mock behavior in comments. - 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.
8. kubechain/internal/humanlayerapi/utils.go:345
- Draft comment:
In NullableTime.UnmarshalJSON, consider logging or providing more context about the attempted formats if none succeed. This can ease debugging of time parsing issues. - 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%
This is generated code from OpenAPI Generator. The error handling is already reasonable - it returns which string couldn't be parsed. Adding logging would be overkill for a parsing utility, as the caller can log the error if needed. The current error message provides sufficient context for debugging.
The suggestion could help with debugging in production environments where seeing the attempted formats could save time. The code is handling user input which can be unpredictable.
While logging could help debugging, it's not appropriate for a low-level parsing utility to handle logging. That responsibility belongs to the caller. The current error handling is sufficient.
Delete the comment. The current error handling is appropriate for a parsing utility, and logging should be handled by the caller if needed.
9. kubechain/README.md:50
- Draft comment:
Typo: In the note, 'Ensure that the samples has default values to test it out.' should be corrected to 'Ensure that the samples have default values to test it out.' - Reason this comment was not posted:
Comment was on unchanged code.
10. kubechain/README.md:76
- Draft comment:
Typo/Redundancy: In step 3, the instruction 'Editkubechain/config/samples/kubechain_v1alpha_contactchannel_with_approval.yamland to include the SlackchannelIdyou integrated with your HumanLayer project.' could be rephrased to remove the unnecessary 'and'. Consider revising it to 'Edit ... to include the SlackchannelId...'. - 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 the comment is technically correct about the grammar, it's an extremely minor issue that doesn't affect comprehension. The instructions are still clear and understandable. This feels more like proofreading than a substantive code review comment. According to the rules, we should not make comments that are obvious or unimportant.
The grammar error could potentially make the documentation look less professional. Poor documentation can impact user experience.
While documentation quality matters, this is such a minor grammatical issue that it doesn't warrant a PR comment. It could be fixed in a general documentation pass later.
Delete this comment as it's too minor and doesn't affect functionality or understanding.
11. kubechain/README.md:81
- Draft comment:
Typo: In the sentence 'If things went well, you should get a Slack message requesting approval or rejection prior finishing up the tool call flow.', consider changing 'prior finishing up' to 'prior to finishing up' for correct grammar. - 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 the grammar correction is technically correct, our rules state that we should not make purely informative comments. The meaning is clear even with the minor grammatical error. This is a documentation change that doesn't affect functionality. The comment doesn't point out a critical issue that needs to be fixed.
The grammar error could make the documentation look unprofessional. Poor documentation can affect user experience.
While documentation quality matters, this is a minor grammatical issue that doesn't significantly impact readability or understanding. Our rules explicitly state not to make purely informative comments.
Delete this comment as it's a minor grammatical suggestion that doesn't materially impact the documentation's effectiveness.
12. kubechain/api/v1alpha1/taskruntoolcall_types.go:24
- Draft comment:
The field 'ToolCallId' (line 24) uses 'Id' instead of 'ID', which is inconsistent with fields like 'ExternalCallID'. Consider renaming it to 'ToolCallID' for consistency. - 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.
13. kubechain/config/crd/bases/kubechain.humanlayer.dev_taskruntoolcalls.yaml:107
- Draft comment:
There is a typographical error in the description for 'externalCallID'. The description is split into two lines (line 107-108) with extra indentation, which breaks up the sentence awkwardly. Consider merging it into one line, for example: 'ExternalCallID is the unique identifier for this function call in external services'. - 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%
This is a purely cosmetic issue in a YAML file. The indentation and line wrapping follow consistent patterns used throughout the file for other similar descriptions. The current format is perfectly valid YAML and follows the file's established style. The suggested change wouldn't improve functionality or clarity.
Maybe inconsistent formatting could cause issues with YAML parsers or documentation generators?
The current formatting is standard for YAML and is used consistently throughout this file. It won't cause any technical issues as it's valid YAML syntax.
This comment should be deleted as it suggests a purely cosmetic change to valid YAML formatting that follows the file's established patterns.
14. kubechain/internal/humanlayer/hlclient.go:15
- Draft comment:
Typographical error: The comment for NewHumanLayerClientFactory incorrectly mentions 'API key' and 'HUMANLAYER_API_KEY' instead of 'API base' and 'HUMANLAYER_API_BASE' as used in the function. Please update the comment to correctly reflect that the function falls back to the HUMANLAYER_API_BASE environment variable. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_VKl6GbI1oFgbRuFv
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
readme draft
| } | ||
| } | ||
|
|
||
| return fmt.Errorf("cannot parse %s as time.Time", string(src)) |
There was a problem hiding this comment.
hah the number of times i've written this function or something like it...
There was a problem hiding this comment.
Was about to comment on how this whole file diff is like an advertisement for type-safe serialization json-replacements. xD
| type ContactChannelSpec struct { | ||
| // ChannelType is the type of channel (e.g. "slack", "email") | ||
| // Type is the type of channel (e.g. "slack", "email") | ||
| // Todo - consider removing this, HumanLayer ContactChannel models don't include it |
There was a problem hiding this comment.
Maybe just a comment for the future, but I've seen some good implementations of TODO with scales on them. Like TODO(1), TODO(2), etc.
The README/developer docs end up having 3 or 4 levels of importance of TODO and this helps identify and actually address the important ones over time. Also makes it easy to know what the severity or type of TODO is when you stumble upon one when navigating the codebase.
| ) | ||
|
|
||
| func requestApproval(client humanlayer.HumanLayerClientWrapper, channelType string) *humanlayerapi.FunctionCallOutput { | ||
| if channelType == "slack" { |
There was a problem hiding this comment.
I'm always a fan of switch/match/case statements. go seems to have good support for them. Since this is an enum you can set the default switch path to throw an error and thus this section of the code would get caught early if we ever expanded the enum.
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(updatedTRTC.Status.Phase).To(Equal(kubechainv1alpha1.TaskRunToolCallPhaseSucceeded)) | ||
| Expect(updatedTRTC.Status.Status).To(Equal(kubechainv1alpha1.TaskRunToolCallStatusTypeReady)) | ||
| Expect(updatedTRTC.Status.Result).To(Equal("5")) // From our mock implementation |
There was a problem hiding this comment.
My #1 question reading this test...what are we testing? I don't see where we check "did the tool run"
I'm assuming its here, but where is this "5" set? is it buried in the setup? If the CORE thing on this test is to make sure we ran the tool, I'd expect that setup to show up in this test body
| approved, ok := status.GetApprovedOk() | ||
|
|
||
| // Check if the value was set | ||
| if ok { |
There was a problem hiding this comment.
Same statement as above. These nests look kinda gross to me.
Example of alternative (using tagless switch):
approved, ok := status.GetApprovedOk()
switch {
case !ok:
fmt.Println("Not responded yet")
case approved == nil:
fmt.Println("Approval status is nil (Not responded yet)")
case *approved:
fmt.Println("Approved")
default:
fmt.Println("Rejected")
}| recorder record.EventRecorder | ||
| server *http.Server | ||
| MCPManager mcpmanager.MCPManagerInterface | ||
| HLClientFactory humanlayer.HumanLayerClientFactory |
There was a problem hiding this comment.
Why is this renamed to Factory? Primarily for my own awareness to know what the standard is/should be for this type of thing moving forward.
There was a problem hiding this comment.
Just to better-reflect the naming of the interface, see the other comment about interface/implementation naming
| TaskRunToolCallStatus *kubechainv1alpha1.TaskRunToolCallStatus | ||
| TaskRunToolCallName string | ||
| TaskRunToolCallArgs string | ||
| ContactChannelType string // "slack" or "email" |
There was a problem hiding this comment.
Should this not just be the same enum used above?
| case "email": | ||
| channelType = kubechainv1alpha1.ContactChannelTypeEmail | ||
| default: | ||
| channelType = kubechainv1alpha1.ContactChannelTypeSlack |
There was a problem hiding this comment.
aha! The case statement I mentioned desiring above! But maybe better to default to error instead of slack for safety?
| client := humanlayerapi.NewAPIClient(config) | ||
|
|
||
| return &HumanLayerClient{client: client}, nil | ||
| return &RealHumanLayerClientFactory{client: client}, nil |
There was a problem hiding this comment.
This is looking mighty java-y (derogatory but also it's fine)
There was a problem hiding this comment.
I was hurting a little when I wrote this too :), in a convo when chatting with @dexhorthy we came to:
- Most things that are passed around among functions should just be the name of the given thing. So rather than call something
WidgetInterface, we call itWidget - Implementations of those interfaces should precede the interface name, so we might have a
MockWidgetorFancyWidget. - My choice of
Realhere was laziness, I'll admit, there are only two implementations in here, one for aMockHumanLayerClientFactoryand thisRealHumanLayerClientFactory
| } | ||
| } | ||
|
|
||
| return fmt.Errorf("cannot parse %s as time.Time", string(src)) |
There was a problem hiding this comment.
Was about to comment on how this whole file diff is like an advertisement for type-safe serialization json-replacements. xD
| // Generate call ID | ||
| callID := "call-" + uuid.New().String() | ||
| // Generate call ID - using a shorter format while maintaining uniqueness | ||
| callID := "c" + uuid.New().String()[:7] |
There was a problem hiding this comment.
we should consider a domain-safe secure-random-string (SRS) like the rest of k8s uses...could even go in the TRTC id (for example, when you create a deployment, it makes a replicaset and pod tha tboth include some random stuff
DEPLOY_NAME-REPLICASET_ID-POD_SRS
metalytics-worker-7f77479dcd-hgnzg
weird that the replicaset gets a hex id whereas the pod ID is more characters. Guessing you get same entropy with fewer string chars...
anyways theorycrafting over...will make an issue
Provides handling (and quite a bit of refactoring to the TaskRunToolCall controller) for managing Approve/Rejection states that come back from HumanLayer.