5 of X: Transition to AwaitingHumanApproval with real request to HL API #44

balanceiskey · 2025-03-26T02:48:51Z

Introduces handling to transition to AwaitingHumanApproval and make request to HumanLayer API

Important

Adds transition to AwaitingHumanApproval in TaskRunToolCall with HumanLayer API integration for approval requests.

Behavior:
- Adds AwaitingHumanApproval and ErrorRequestingHumanApproval statuses in taskruntoolcall_types.go.
- Implements transition to AwaitingHumanApproval in taskruntoolcall_controller.go.
- Sends approval requests to HumanLayer API using postToHumanLayer().
- Handles errors in HumanLayer requests, setting status to ErrorRequestingHumanApproval.
Controller:
- Updates TaskRunToolCallReconciler to include HLClient for HumanLayer API interactions.
- Adds getContactChannel() and getHumanLayerAPIKey() for fetching contact channel and API key.
- Modifies Reconcile() to handle approval logic and requeue if awaiting approval.
Tests:
- Adds tests in taskruntoolcall_controller_test.go for transitions to AwaitingHumanApproval and ErrorRequestingHumanApproval.
- Mocks HumanLayer client in mock_hlclient.go for testing API interactions.
Misc:
- Updates CRD in kubechain.humanlayer.dev_taskruntoolcalls.yaml to include new statuses.
- Adds sample configuration in kubechain_v1alpha_contactchannel_with_approval.yaml.

^{This description was created by}^{for 9a5a149. It will automatically update as commits are pushed.}

balanceiskey · 2025-03-26T15:19:53Z

kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go

 		r.MCPManager = mcpmanager.NewMCPServerManager()
 	}

+	if r.HLClient == nil {


A note, I'm piggybacking here and probably breaking the naming of this function, maybe worth looking at renaming?

ellipsis-dev

❌ Changes requested. Reviewed everything up to 9a5a149 in 3 minutes and 24 seconds

More details

Looked at 1412 lines of code in 9 files
Skipped 1 files when reviewing.
Skipped posting 11 drafted comments based on config settings.

1. kubechain/internal/mcpmanager/mcpmanager.go:157

Draft comment:
Prefer structured logging over fmt.Printf for error messages.
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/mcpmanager/mcpmanager.go:326

Draft comment:
Consider validating or normalizing the tool name comparison in FindServerForTool.
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. kubechain/internal/mcpmanager/mcpmanager.go:298

Draft comment:
Enhance handling of non-text content in tool call output.
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. kubechain/api/v1alpha1/taskruntoolcall_types.go:10

Draft comment:
Nice addition of the new status enum value. Ensure that consumers use these constants instead of hard-coded strings for consistency.
Reason this comment was not posted:
Comment looked like it was already resolved.

5. kubechain/config/crd/bases/kubechain.humanlayer.dev_taskruntoolcalls.yaml:140

Draft comment:
Added new enum value for status. Verify that all controllers use the constant and not literal strings.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
The comment is asking the PR author to verify that all controllers use the constant instead of literal strings. This is a request for verification, which is not allowed according to the rules. The comment does not provide a specific suggestion or point out a specific issue in the code.

6. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go:785

Draft comment:
When updating status to 'AwaitingHumanApproval', use the enum constant instead of the string literal.
Reason this comment was not posted:
Marked as duplicate.

7. kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go:801

Draft comment:
Good nil-check for HLClient; ensure that all injected dependencies are verified before use.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
This comment is asking the PR author to ensure that dependencies are verified, which is similar to asking them to double-check or ensure behavior. It doesn't provide a specific suggestion or point out a specific issue.

8. kubechain/internal/humanlayer/hlclient.go:18

Draft comment:
Nice refactoring to use an interface and wrapper. Consider adding documentation for each method to improve clarity for future developers.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 20% <= threshold 50%
The comment is suggesting adding documentation for methods, which is a general suggestion for improvement. It doesn't point out a specific issue or provide a specific code suggestion. It seems to be more of a general best practice reminder, which doesn't align with the rules provided.

9. kubechain/internal/humanlayer/mock_hlclient.go:80

Draft comment:
The mock implementation correctly stores invocation details for verification in tests.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

10. kubechain/internal/mcpmanager/mcpmanager.go:28

Draft comment:
The new MCPManagerInterface is defined properly; ensure callers use the interface abstraction to facilitate testing.
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 comment is asking the author to "ensure" something rather than pointing out a specific issue. It's making a suggestion about how callers should use the code, but not identifying any actual problem. The interface addition itself looks good, but telling callers to use it is more of a documentation point than a required code change.
The interface addition is a good practice for testing, so maybe we should keep the comment to encourage good testing practices? The comment could help future maintainers.
While testing practices are important, this comment violates our rule about not making comments that just ask authors to "ensure" things. It's not pointing out a specific issue that needs fixing.
Delete this comment as it's asking the author to "ensure" something rather than identifying a specific issue that needs to be fixed. It's more of a general suggestion than an actionable review comment.

11. kubechain/internal/mcpmanager/mcpmanager.go:369

Draft comment:
Consider using a logger instead of fmt.Printf for logging errors when marshaling input schemas.
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_26YxJShrTXuKSuYo

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.

kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go

dexhorthy · 2025-03-26T16:24:24Z

kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go

+		"tool_name": trtc.Spec.ToolRef.Name,
+		"task_run":  trtc.Spec.TaskRunRef.Name,
+		"namespace": trtc.Namespace,
+	})


yeah I think we want isntead of

approve_tool_call({ "namespace": "default", "task_run": "make-fetch-happen-task-1", "tool_name": "make-fetch-happen-server__fetch" })

we want

fetch__fetch({ url: ... offset: ... })

dexhorthy · 2025-03-26T16:25:25Z

kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go

 	}

+	// Do not pass GO, do not collect $200, if we've errored
+	if trtc.Status.Status == kubechainv1alpha1.TaskRunToolCallStatusTypeErrorRequestingHumanApproval {


if we're in a terminal state, I've tried to hoist these to the top

oh i read it wrong ignore me

dexhorthy · 2025-03-26T16:27:47Z

kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go

+
 		if needsApproval {
+
+			trtcNamespace := trtc.Namespace


as you're going, this could probably be it's own method eventually

ll be back

…equest

balanceiskey force-pushed the eng-1128-trtc-test-refactor-4 branch from dd213ff to 9a5a149 Compare March 26, 2025 15:18

balanceiskey commented Mar 26, 2025

View reviewed changes

balanceiskey marked this pull request as ready for review March 26, 2025 15:21

balanceiskey requested review from Timothy102, allisoneer and dexhorthy March 26, 2025 15:23

ellipsis-dev bot reviewed Mar 26, 2025

View reviewed changes

kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go Outdated Show resolved Hide resolved

kubechain/internal/controller/taskruntoolcall/taskruntoolcall_controller.go Show resolved Hide resolved

balanceiskey mentioned this pull request Mar 26, 2025

4 of X: Pending -> Succeeded state with MCP Tool, no approval #41

Merged

dexhorthy reviewed Mar 26, 2025

View reviewed changes

balanceiskey added 12 commits March 26, 2025 12:40

Pending -> Succeeded state with MCP Tool, no approval

2d6f39f

Commenting some currently unused fields, they'

c7540e5

ll be back

Resolve code review quirks; remove unnecessary interface

692ef21

Restore passing test w/o HL call implementation

775ca17

Mock out hlclient; restore functional version of this

5a7258d

Update tests to include API mock and use when testing humanapproval r…

0b21219

…equest

Remove debug binary file

da0665c

Add debug binary files to gitignore

8ef25e2

Remove unused attributes

c21c966

Code review tweak

1907d68

More tweaks

ece18c7

Adjust handling of kwargs

b4334b6

balanceiskey force-pushed the eng-1128-trtc-test-refactor-4 branch from 42674fe to b4334b6 Compare March 26, 2025 17:45

dexhorthy approved these changes Mar 31, 2025

View reviewed changes

balanceiskey merged commit 0799333 into humanlayer:main Mar 31, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

5 of X: Transition to AwaitingHumanApproval with real request to HL API #44

5 of X: Transition to AwaitingHumanApproval with real request to HL API #44

Uh oh!

balanceiskey commented Mar 26, 2025 •

edited by ellipsis-dev bot

Loading

Uh oh!

balanceiskey Mar 26, 2025

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

Uh oh!

Uh oh!

dexhorthy Mar 26, 2025 •

edited

Loading

Uh oh!

dexhorthy Mar 26, 2025

Uh oh!

dexhorthy Mar 26, 2025

Uh oh!

dexhorthy Mar 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

5 of X: Transition to AwaitingHumanApproval with real request to HL API #44

5 of X: Transition to AwaitingHumanApproval with real request to HL API #44

Uh oh!

Conversation

balanceiskey commented Mar 26, 2025 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

balanceiskey Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dexhorthy Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dexhorthy Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

dexhorthy Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

dexhorthy Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

balanceiskey commented Mar 26, 2025 •

edited by ellipsis-dev bot

Loading

dexhorthy Mar 26, 2025 •

edited

Loading