Dexter/eng 1154 task and taskrun are hard to understand 2 #55
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.
👍 Looks good to me! Reviewed everything up to f3a8e3f in 1 minute and 40 seconds
More details
- Looked at
381lines of code in5files - Skipped
0files when reviewing. - Skipped posting
20drafted comments based on config settings.
1. README.md:31
- Draft comment:
Good update to consistently refer to TaskRun. Verify that all examples now match the new spec (e.g. using 'userMessage' instead of 'message'). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify that examples match a new specification. It is asking for confirmation rather than making a specific suggestion or pointing out a specific issue.
2. kubechain/config/manager/kustomization.yaml:7
- Draft comment:
New tag updated correctly; ensure CI/CD picks up the new image version. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. kubechain/internal/controller/taskrun/taskrun_controller.go:56
- Draft comment:
Check early return in getTask when both AgentRef and UserMessage are provided; this appears intentional for user-initiated TaskRuns. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. kubechain/internal/controller/taskrun/taskrun_controller.go:134
- Draft comment:
Agent resolution: Using TaskRun's AgentRef if provided is clear. Confirm that error status is set consistently when no agent ref is found. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. kubechain/internal/controller/taskrun/taskrun_controller.go:296
- Draft comment:
Ensure proper status update after tool call completion within processToolCalls; appending tool results to context window is done correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. kubechain/internal/controller/taskrun/taskrun_controller.go:761
- Draft comment:
Child span creation for LLM request is clear; ensure that span ends in all error cases to avoid leaked spans. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. kubechain/internal/controller/taskrun/taskrun_controller_test.go:130
- Draft comment:
Test for 'ReadyForLLM' with userMessage + agentRef (no taskRef) correctly sets context; consider adding additional edge tests for missing fields. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
8. kubechain/internal/controller/taskrun/utils_test.go:167
- Draft comment:
The test utilities set up standard resources. Ensure similar setups are used consistently in other tests. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. kubechain/internal/controller/taskrun/taskrun_controller.go:56
- Draft comment:
Good use of the early return when both agentRef and userMessage are provided; the inline comment clearly explains that no Task is needed when these fields are set. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, praising the use of early return and the inline comment. It doesn't provide any actionable feedback or suggestions for improvement.
10. kubechain/internal/controller/taskrun/taskrun_controller.go:139
- Draft comment:
Error handling for missing agent reference is thorough. Consider if adding more context to the error message would improve debugging, though the current implementation is clear. - Reason this comment was not posted:
Comment looked like it was already resolved.
11. kubechain/internal/controller/taskrun/taskrun_controller.go:207
- Draft comment:
The branch handling an empty message (when both userMessage and task message are missing) is clear and correctly updates the TaskRun status to failed. No changes required. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it praises the code for handling a specific case correctly. It doesn't provide any actionable feedback or suggestions for improvement.
12. kubechain/internal/controller/taskrun/taskrun_controller.go:272
- Draft comment:
The logic in processToolCalls that iterates over tool calls and determines completeness is straightforward. It might help to document the intended retry behavior for incomplete calls. - 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/internal/controller/taskrun/taskrun_controller.go:536
- Draft comment:
The generation of unique names for TaskRunToolCall objects using a combination of the TaskRun name and a request ID is neat; ensure that name collisions remain unlikely in high-concurrency scenarios. - 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.
14. kubechain/internal/controller/taskrun/taskrun_controller_test.go:130
- Draft comment:
The test covering the transition when only userMessage and agentRef are provided (no taskRef) is comprehensive. Consider enabling or adding tests for the currently disabled (XIt) cases, such as error backoff transitions. - Reason this comment was not posted:
Comment was on unchanged code.
15. kubechain/internal/controller/taskrun/utils_test.go:16
- Draft comment:
The helper functions in utils_test.go for setting up and tearing down resources (Secrets, LLMs, Agents, Tasks, and TaskRuns) are well organized and easy to follow. - 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.
16. kubechain/internal/controller/taskrun/taskrun_controller.go:107
- Draft comment:
The status updates in various error-handling branches are consistent. Consider evaluating if additional retry/backoff logic might be appropriate when status updates fail due to conflicts. - 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.
17. README.md:363
- Draft comment:
The output snippet forkubectl get taskrunshows the task name as "hello-world-task" but the YAML specifies it as "hello-world-1". Please update the output snippet so it matches the YAML definition. - 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.
18. README.md:388
- Draft comment:
In the output snippet forkubectl describe taskrun, there is a typographical error: "ame:" should be "Name:". Additionally, the kind should be updated from "Task" to "TaskRun" to reflect the changes in the YAML. Please correct these errors. - 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.
19. kubechain/internal/controller/taskrun/taskrun_controller_test.go:61
- Draft comment:
Typographical error: please change 'dont' to "don't" in the comment so that it reads "// todo don't error if not found, don't requeue". - 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.
20. kubechain/internal/controller/taskrun/taskrun_controller_test.go:507
- Draft comment:
Typographical recommendation: In several TODO comments (e.g., lines 507, 510, 513, and 516), please replace 'i think' with 'I think' for proper capitalization. - 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.
Workflow ID: wflow_4jd5JVV1zZorjj6j
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
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.
make it possible to run taskrun without a task
remove all mention of task from the readme
next step
remove
taskReffromTaskRunand all controller logicremove Task object/controller
rename taskRun to Task