Approval Flow by jamesrochabrun · Pull Request #1 · getcmd-dev/cmd

jamesrochabrun · 2025-05-30T09:27:55Z

Added approval flow.

Saving Always approved tools in user defaults
Showing UI for approval flow
Handling denied tool sending message to chat history with error description
Added new button to show label + icons

TODO:

Address feedback
Tests

jamesrochabrun · 2025-05-30T09:31:01Z

I was having troubles withe push flow, so I just pushed all as no verify to get initial feedback, also the app suddenly looses the ability to access Xcode workspace which makes a bit hard to test this reliable. Maybe we can have a quick chat to debug.

gsabran

Looking good!

Left a few comments.

For the UI, it seems that we'll have to use a vertical layout as it's getting compacted in the sidebar

What do you think of some layout like:

List files
{ args... }
Do you want to proceeed?
Yes, always allow (key shortcut)
Yes (key shortcut)
No (key shortcut)

gsabran · 2025-05-30T15:16:34Z

app/modules/services/LLMService/Sources/DefaultLLMService.swift


    do {
+      // Request approval before executing the tool
+      try await context.requestToolApproval(toolUse)


You probably want to put this before startExecuting (it's called in two different places). Here the tool call has already been started, and we're waiting for its result.

gsabran · 2025-05-30T15:17:53Z

app/modules/serviceInterfaces/LLMServiceInterface/Sources/LLMService.swift

+public enum LLMServiceError: Error {
+  case toolUsageDenied
+}
+
+extension LLMServiceError: LocalizedError {
+  public var errorDescription: String? {
+    switch self {
+    case .toolUsageDenied:
+      return "User denied permission to execute this tool. Please suggest an alternative approach or ask for clarification."
+    }
+  }
+}


nit: if you don't care about having a specific error type and just want a localized error, you can use AppFoundation.AppError

I think this is relevant as we are sending this message in to the chat conversation history so the LLM knows what to do next, the LLM has no concept of our UX so we need to give it hints about what is happening and how to recover

I meant that AppError can preserve your message without requiring a dedicated type, but you would not be able to do something like

catch error as LLMServiceError { } catch { }

Oh so you already have a custom error? yeah I can use that I guess

app/modules/features/Chat/Sources/Input/ChatInputViewModel.swift

gsabran · 2025-05-30T15:40:34Z

app/modules/features/Chat/Sources/Input/ChatInputViewModel.swift

+enum ApprovalResult {
+  case approved
+  case denied
+  case alwaysApprove(toolName: String)


maybe also "toolUseCancel" ?

instead of denied?

in addition

gsabran · 2025-05-30T15:41:01Z

app/modules/features/Chat/Sources/Input/ChatInputViewModel.swift


+// MARK: - ToolApprovalRequest
+
+// TODO: james - should move this `ToolFoundation`?


As long as we're not using this outside of Chat I'd say no, but if we do yes

gsabran · 2025-05-30T15:44:03Z

app/modules/features/Chat/Sources/ChatTabViewModel.swift

  }
+
+  private func storeAlwaysApprovePreference(for toolName: String) {
+    userDefaults.set(true, forKey: "\(Self.userDefaultsAlwaysApproveKey)\(toolName)")


👍 using user default is ok for now. I think eventually we'll want to move this to the Setting, so that we can display this in the settings and users can also update them from there.

yeah I can se this moving there eventually.

jamesrochabrun · 2025-05-31T07:47:47Z

I added the changes based on the feedback, I am not quite sure why yet but if I denied the usage of a tool e,g the command tool, the tool heeader message still shows "Tool use: execute_command" any pointer?

gsabran · 2025-05-31T23:09:59Z

(just pushed a merge commit to make changes easier to review)

gsabran · 2025-05-31T23:15:58Z

app/modules/services/LLMService/Sources/RequestStreamingHelper.swift

+          Task { [weak self] in
+            guard let self else { return }
+            do {
+              try await self.context.requestToolApproval(toolUse)


you made a good point about the tool use showing up in the UI. If we don't want to show it, we could ask for approval before inserting the tool use in the chat history (here and here).

But I can see this being an issue because we likely want to keep the tool use in the history, even if it is rejected, so that the next message has the rejection and the LLM can continue based of that.

What do you think of instead modifying the Tool protocol to better describe the pending approval / rejected status? We could then have a standard UI for a rejected tool use. We can do this in a follow up PR.

What do you think of instead modifying the Tool protocol to better describe the pending approval / rejected status? We could then have a standard UI for a rejected tool use. We can do this in a follow up PR.

I think this is the correct move yes.

gsabran · 2025-05-31T23:18:54Z

Not for now, but we can think about whether we need to give a reason when rejecting tool to help the LLM make progress

gsabran · 2025-06-01T03:51:45Z

Looks great overall. Nit: can we update this to:
"cmd wants to use the list files tool"
always allow: ⌘↵⇧, allow once: ⌘↵, reject: ⌘⌫

(clarifying what is being ask, putting the ⌘ key first to be on brand, slight simplification for reject, starting with the most favorable option)

Co-authored-by: Guillaume Sabran <sabranguillaume@gmail.com>

jamesrochabrun · 2025-06-01T06:51:35Z

2 pending jobs Failing

Run cd app && ./cmd.sh clean && ./cmd.sh test
Command not found: test
Error: Process completed with exit code 1.

I am also getting this error locally. cc @gsabran

gsabran · 2025-06-01T15:20:40Z

oh I got some CI credits back. Yeah! No worries about CI. Could you actually comment out

pull_request:
branches: [ "main" ]

here https://github.com/gsabran/cmd/blob/e9b18b952262c519c0232aa9b71d2c20eb0cbd06/.github/workflows/main.yml#L9-L10

So that I keep my CI credits to test automating releasing the app?

jamesrochabrun · 2025-06-01T16:28:50Z

.github/workflows/main.yml

  push:
    branches: [ "main" ]
-  pull_request:
-    branches: [ "main" ]


cc @gsabran like this?

yes perfect

jamesrochabrun · 2025-06-01T16:29:18Z

app/cmd.sh

 }

 close_xcode() {
-	if pgrep -x "Xcode" > /dev/null; then


cc @gsabran changes made by linter

jamesrochabrun · 2025-06-01T16:29:39Z

app/tools/sync-js-bundle-to-app.sh

 		# So we need to exit with a non-zero exit code to indicate that the build needs to be restarted with the correct files.
 		echo "The local server has been updated. Please restart the build." >&2
 		exit 1
-	fi	


cc @gsabran changes mad by linter

jamesrochabrun · 2025-06-01T16:29:50Z

local-server/tools/parseAnthropicResponse.test.ts

 				name: "Bash",
 				parameters: {
-					command: "cd /Users/me/command/app/modules && swift test 2>&1 | head -100",
+					command: "cd /Users/guigui/dev/Xcompanion/app/modules && swift test 2>&1 | head -100",


cc @gsabran changes made by linter

oh really? Weird I'll look into it

jamesrochabrun · 2025-06-01T16:33:13Z

I pushed some changes made by the linter, tagged you on those in case you want to take a look. Thanks!

gsabran · 2025-06-01T03:53:04Z

app/modules/coreui/DLS/Sources/CoreUI/buttons/IconsLabelButton.swift

+  let action: () -> Void
+  let systemNames: [String]
+  let label: String
+  let onHoverColor: Color
+  let padding: CGFloat
+  let cornerRadius: CGFloat


nit: private? 🤷‍♂️

gsabran · 2025-06-01T17:34:46Z

.github/workflows/main.yml

  push:
    branches: [ "main" ]
-  pull_request:
-    branches: [ "main" ]


yes perfect

gsabran reviewed May 30, 2025

View reviewed changes

jamesrochabrun requested a review from gsabran May 31, 2025 07:47

gsabran reviewed May 31, 2025

View reviewed changes

gsabran approved these changes Jun 1, 2025

View reviewed changes

gsabran and others added 11 commits May 31, 2025 22:05

fix remaining tests

41cf5fe

Adding approval flow

bec6a04

fix merge conflict

a40d088

fix merge conflict

3434e40

Update app/modules/features/Chat/Sources/Input/ChatInputViewModel.swift

c01050b

Co-authored-by: Guillaume Sabran <sabranguillaume@gmail.com>

Update app/modules/features/Chat/Sources/Input/ChatInputViewModel.swift

5fcc668

Co-authored-by: Guillaume Sabran <sabranguillaume@gmail.com>

Udpating changes for display tool

42a9be3

Gui's feedback

53bfcbe

MAking changes on approval flow

1ebc4ba

making cmd key at the beginning

9502956

lint

2fb5933

jamesrochabrun force-pushed the jroch-main branch from 8d8b39a to 2fb5933 Compare June 1, 2025 05:20

jamesrochabrun added 3 commits May 31, 2025 22:49

lint for CI

65edc08

Lint for CI in tests.ts

06e157c

yarn lint

4e3b330

CI

2c1636e

jamesrochabrun commented Jun 1, 2025

View reviewed changes

gsabran approved these changes Jun 1, 2025

View reviewed changes

jamesrochabrun merged commit bf2cf21 into main Jun 2, 2025

jamesrochabrun deleted the jroch-main branch June 2, 2025 05:09

jamesrochabrun mentioned this pull request Jun 4, 2025

Add tool approval status tracking to show pending/rejected states [Draft] #8

Merged

gsabran pushed a commit that referenced this pull request Oct 25, 2025

Add Apache2 license (#1)

79c2cae


		// MARK: - ToolApprovalRequest

		// TODO: james - should move this `ToolFoundation`?

Conversation

jamesrochabrun commented May 30, 2025

Uh oh!

jamesrochabrun commented May 30, 2025

Uh oh!

gsabran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesrochabrun commented May 31, 2025

Uh oh!

gsabran commented May 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsabran commented May 31, 2025

Uh oh!

gsabran commented Jun 1, 2025

Uh oh!

jamesrochabrun commented Jun 1, 2025

Uh oh!

gsabran commented Jun 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesrochabrun commented Jun 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants