When rejected, stamp status comment to trtc #49
Merged
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
Contributor
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 2f64d76 in 1 minute and 46 seconds
More details
- Looked at
121lines of code in5files - Skipped
0files when reviewing. - Skipped posting
12drafted comments based on config settings.
1. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go:792
- Draft comment:
Verify that passing an empty string for non-rejected cases is intentional and add a comment explaining that 'result' is ignored unless status is ToolCallRejected. - 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_test.go:411
- Draft comment:
Ensure that the expected rejection comment is consistently verified in the test. The test now checks that Status.Result equals the rejection comment. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that the test consistently verifies the expected rejection comment. This is a request for verification, which violates the rule against asking the author to ensure behavior is intended or tested.
3. kubechain/internal/humanlayer/mock_hlclient.go:108
- Draft comment:
Check that the use of NewNullableString for StatusComment is properly handled. Consider adding inline comments for clarity on how the StatusComment field is incorporated. - 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%
The use of NewNullableString here follows the same pattern as other fields in the mock. The code is straightforward - it's just wrapping a string pointer in a nullable wrapper, which is a common pattern throughout this mock implementation. Adding inline comments would be redundant since the code is self-explanatory and follows established patterns.
Perhaps there could be edge cases around null handling that aren't being considered. Maybe the comment author saw something in the actual implementation that requires special handling.
This is a mock implementation following consistent patterns with other fields. If there were special handling requirements, they would be better addressed in the actual implementation, not the mock.
The comment should be deleted as it suggests unnecessary documentation and raises speculative concerns about code that follows established patterns.
4. kubechain/config/manager/kustomization.yaml:6
- Draft comment:
Verify that changing newName to 'controller' (instead of full repository path) is intentional and consistent with the deployment configuration. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%
The comment is asking the PR author to verify their intention, which violates the rules. However, it is also pointing out a potential inconsistency with the deployment configuration, which could be a valid concern. The comment could be rephrased to suggest checking the consistency without asking for verification.
5. kubechain/.gitignore:10
- Draft comment:
The addition of config/samples/*.play.yaml for developer playground samples is clear. Confirm that this pattern does not unintentionally ignore other necessary config files. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm that the addition of a specific file pattern does not unintentionally ignore other necessary config files. This is asking for confirmation of intention, which is against the rules.
6. kubechain/config/manager/kustomization.yaml:6
- Draft comment:
The image name was changed from 'ghcr.io/humanlayer/smallchain' to just 'controller'. Please confirm this is intended, as using a shortened name might affect deployments (e.g. if pulling from a registry). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment asks for confirmation ("Please confirm this is intended") which violates our rules. It's also speculative ("might affect deployments"). We don't have enough context to know if using a short name is actually problematic - the deployment could be configured to use a default registry. The author likely changed this intentionally.
The image name change could potentially cause real deployment issues if there's no default registry configured. This might be a legitimate concern.
Even if there could be issues, the comment violates our rules by being speculative and asking for confirmation. If there was a clear deployment problem, it would be caught during testing.
Delete the comment because it violates multiple rules - it asks for confirmation and makes speculative claims without strong evidence of an actual problem.
7. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go:707
- Draft comment:
The updateTRTCStatus function signature now includes a new 'result' parameter to capture the rejection comment. Ensure that all callers supply the intended value to avoid missing rejection info. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that all callers supply the intended value for a new parameter. This falls under the rule of not asking the author to ensure behavior is intended or tested. The comment does not provide a specific suggestion or point out a specific issue, so it should be removed.
8. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go:788
- Draft comment:
In the rejection branch of handlePendingApproval, the updateTRTCStatus call now passes status.GetComment() as the result. Please verify that status.GetComment() reliably contains the intended rejection message. - 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.
9. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller_test.go:406
- Draft comment:
The test now asserts that TRTC.Status.Result equals the rejection comment. Consider adding tests for cases when no rejection comment is provided to ensure robust handling. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
10. kubechain/internal/humanlayer/mock_hlclient.go:100
- Draft comment:
The mock client now returns a rejection comment via m.parent.StatusComment when ShouldReturnRejection is true. Verify that this mock behavior aligns with production expectations and that nil cases are handled if needed. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
11. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go:808
- Draft comment:
Typographical note: In the function signature for requestHumanApproval (lines 806-808), there appears to be an extra comma and a stray colon before the closing parenthesis. This formatting is likely unintentional and should be cleaned up to ensure the function signature follows standard Go syntax. - 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.
12. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller_test.go:409
- Draft comment:
Typo: In the rejectionComment string, the phrase 'should not be be given permission' contains a duplicate 'be'. Please remove one 'be'. - 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 typo, this is in a test file and the duplicate word is in a mock rejection message. The message content is not critical to the functionality being tested - it's just sample text. The tests would work exactly the same with or without the typo. The comment is more about prose quality than code quality.
The typo could be seen as unprofessional and might be worth fixing for code cleanliness. Test files are still part of the codebase that needs to be maintained.
While code cleanliness is good, this is an extremely minor issue in test mock data that has no functional impact. The comment adds noise without providing meaningful value.
Delete the comment. The typo is too minor and is in test mock data that doesn't affect functionality.
Workflow ID: wflow_skcc92fTXOIqcwEq
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| } | ||
|
|
||
| func (r *TaskRunToolCallReconciler) updateTRTCStatus(ctx context.Context, trtc *kubechainv1alpha1.TaskRunToolCall, trtcStatusType kubechainv1alpha1.TaskRunToolCallStatusType, trtcStatusPhase kubechainv1alpha1.TaskRunToolCallPhase, statusDetail string) (ctrl.Result, error, bool) { | ||
| func (r *TaskRunToolCallReconciler) updateTRTCStatus(ctx context.Context, trtc *kubechainv1alpha1.TaskRunToolCall, trtcStatusType kubechainv1alpha1.TaskRunToolCallStatusType, trtcStatusPhase kubechainv1alpha1.TaskRunToolCallPhase, statusDetail string, result string) (ctrl.Result, error, bool) { |
Contributor
There was a problem hiding this comment.
Document the new result parameter in updateTRTCStatus. Update function comment to note that result is used only when trtcStatusType indicates rejection.
dexhorthy
reviewed
Apr 2, 2025
| } | ||
|
|
||
| func (r *TaskRunToolCallReconciler) updateTRTCStatus(ctx context.Context, trtc *kubechainv1alpha1.TaskRunToolCall, trtcStatusType kubechainv1alpha1.TaskRunToolCallStatusType, trtcStatusPhase kubechainv1alpha1.TaskRunToolCallPhase, statusDetail string) (ctrl.Result, error, bool) { | ||
| func (r *TaskRunToolCallReconciler) updateTRTCStatus(ctx context.Context, trtc *kubechainv1alpha1.TaskRunToolCall, trtcStatusType kubechainv1alpha1.TaskRunToolCallStatusType, trtcStatusPhase kubechainv1alpha1.TaskRunToolCallPhase, statusDetail string, result string) (ctrl.Result, error, bool) { |
Contributor
There was a problem hiding this comment.
the signature of this method is getting really big - 6 params, 5 of them strings coming through now...is this really better than passing in a struct or doing it inline?
Contributor
There was a problem hiding this comment.
not a blocker just something to think about
Contributor
Author
There was a problem hiding this comment.
Yeah I had that thought as I was rolling through, will peek at this on incoming updates.
Contributor
Author
There was a problem hiding this comment.
Was leaning struct, doing inline is definitely an option though, will judge based on how it reads.
dexhorthy
approved these changes
Apr 2, 2025
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.
Important
Enhance
TaskRunToolCallReconcilerto store rejection comments on tool call rejection and update related tests.updateTRTCStatusintaskruntoolcall_controller.gonow accepts aresultparameter to store rejection comments whenTaskRunToolCallStatusTypeToolCallRejected.handlePendingApprovalupdated to pass rejection comments toupdateTRTCStatus.AwaitingHumanApproval -> ToolCallRejectedintaskruntoolcall_controller_test.goto verify rejection comments are stored inStatus.Result.config/samples/*.play.yamlto.gitignore.kustomization.yamlto changenewTagforcontrollerimage.This description was created by
for 2f64d76. It will automatically update as commits are pushed.