-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: Replace terminal dangerous-command block with HITL approval and add Safe Mode in Settings #1310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
spider-yamet
wants to merge
49
commits into
eigent-ai:main
Choose a base branch
from
spider-yamet:feat/replace-strict-command-prohibition
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+2,834
−24
Open
feat: Replace terminal dangerous-command block with HITL approval and add Safe Mode in Settings #1310
Changes from all commits
Commits
Show all changes
49 commits
Select commit
Hold shift + click to select a range
178687c
Implement Replace strict command prohibition
spider-yamet ea58d8f
Revert unnecessary test implementations
spider-yamet feedb69
Revert unnecessary vite dev server configuration
spider-yamet aaf4432
Restore index.ts file from electron/main
spider-yamet ae50b21
Fix for pre-commit check
spider-yamet d92015d
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 7a4809d
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet b53d046
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 8fc5e33
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 7708dca
redesign
bytecii e0eca63
Merge branch 'main' into feat/replace-strict-command-prohibition
bytecii 778bea2
redesign & fix
bytecii f05eb7f
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 81cc43d
Merge branch 'main' into feat/replace-strict-command-prohibition
bytecii 702a819
refactor: move HitlOptions and ApprovalRequest to app.hitl package
bytecii 6518d87
refactor: move HITL models to app/hitl/config.py
bytecii 18c2af8
refactor: move _request_user_approval to TerminalToolkit and clean up
bytecii 4db02a6
refactor: clean up docstrings, use enums, and move _thread_local
bytecii fa22cae
fix: read HITL setting on the fly and send it on follow-up tasks
bytecii f980daa
Merge branch 'main' into feat/replace-strict-command-prohibition
bytecii 4239088
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 1a21afe
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 9469e7a
feat: replace per-agent Queue with per-call Future for concurrent HIT…
bytecii 395e769
docs: replace 'dangerous' with neutral wording in approval-related do…
bytecii d90a4fa
i18n: add missing approval-pending key to all locales
bytecii 355bbb7
fix: remove duplicate terminal-approval keys in en-us setting.json
bytecii cef0239
Merge branch 'main' into feat/replace-strict-command-prohibition
fengju0213 5bb5aeb
Merge branch 'main' into feat/replace-strict-command-prohibition
fengju0213 cd43f0b
fix(hitl): make approval future resolution thread-safe and correct co…
fengju0213 5c2a7ba
Merge branch 'main' into feat/replace-strict-command-prohibition
fengju0213 57bf7c7
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 6693308
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet c75e5b2
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 2459951
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet fed7e25
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet a1c4242
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 8d10a57
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 0c25112
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 28f545b
Implement fix
spider-yamet 4aef108
Fix unit test
spider-yamet ac9a436
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 3d5bdc1
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet fdc5b29
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet ec64278
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet e9ae820
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 1288b47
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 3261fdb
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet a43c905
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet 9ed63b0
Merge branch 'main' into feat/replace-strict-command-prohibition
spider-yamet File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ | |
| import shutil | ||
| import subprocess | ||
| import threading | ||
| import time | ||
| import uuid | ||
| from concurrent.futures import ThreadPoolExecutor | ||
|
|
||
| from camel.toolkits.terminal_toolkit import ( | ||
|
|
@@ -28,17 +30,25 @@ | |
|
|
||
| from app.agent.toolkit.abstract_toolkit import AbstractToolkit | ||
| from app.component.environment import env | ||
| from app.hitl.terminal_command import ( | ||
| is_dangerous_command, | ||
| validate_cd_within_working_dir, | ||
| ) | ||
| from app.model.enums import ApprovalAction | ||
| from app.service.task import ( | ||
| Action, | ||
| ActionCommandApprovalData, | ||
| ActionTerminalData, | ||
| Agents, | ||
| get_task_lock, | ||
| get_task_lock_if_exists, | ||
| process_task, | ||
| ) | ||
| from app.utils.listen.toolkit_listen import auto_listen_toolkit | ||
|
|
||
| logger = logging.getLogger("terminal_toolkit") | ||
|
|
||
|
|
||
| # App version - should match electron app version | ||
| # TODO: Consider getting this from a shared config | ||
| APP_VERSION = "0.0.88" | ||
|
|
@@ -58,7 +68,7 @@ def get_terminal_base_venv_path() -> str: | |
| class TerminalToolkit(BaseTerminalToolkit, AbstractToolkit): | ||
| agent_name: str = Agents.developer_agent | ||
| _thread_pool: ThreadPoolExecutor | None = None | ||
| _thread_local = threading.local() | ||
| _thread_local: threading.local = threading.local() | ||
|
|
||
| def __init__( | ||
| self, | ||
|
|
@@ -69,7 +79,6 @@ def __init__( | |
| use_docker_backend: bool = False, | ||
| docker_container_name: str | None = None, | ||
| session_logs_dir: str | None = None, | ||
| safe_mode: bool = True, | ||
| allowed_commands: list[str] | None = None, | ||
| clone_current_env: bool = True, | ||
| ): | ||
|
|
@@ -100,22 +109,30 @@ def __init__( | |
| max_workers=1, thread_name_prefix="terminal_toolkit" | ||
| ) | ||
|
|
||
| self._use_docker_backend = use_docker_backend | ||
| self._working_directory = working_directory | ||
|
|
||
| # safe_mode is read fresh from task_lock in shell_exec (see | ||
| # _get_terminal_approval), but we need an initial value for | ||
| # super().__init__. | ||
| task_lock = get_task_lock_if_exists(api_task_id) | ||
| terminal_approval = ( | ||
| task_lock.hitl_options.terminal_approval if task_lock else False | ||
| ) | ||
| camel_safe_mode = not terminal_approval | ||
| super().__init__( | ||
| timeout=timeout, | ||
| working_directory=working_directory, | ||
| use_docker_backend=use_docker_backend, | ||
| docker_container_name=docker_container_name, | ||
| session_logs_dir=session_logs_dir, | ||
| safe_mode=safe_mode, | ||
| safe_mode=camel_safe_mode, | ||
| allowed_commands=allowed_commands, | ||
| clone_current_env=True, | ||
| install_dependencies=[], | ||
| ) | ||
|
|
||
| # Auto-register with TaskLock for cleanup when task ends | ||
| from app.service.task import get_task_lock_if_exists | ||
|
|
||
| task_lock = get_task_lock_if_exists(api_task_id) | ||
| if task_lock: | ||
| task_lock.register_toolkit(self) | ||
| logger.info( | ||
|
|
@@ -349,7 +366,96 @@ def _run_coro_in_thread(coro, task_lock): | |
| exc_info=True, | ||
| ) | ||
|
|
||
| def shell_exec( | ||
| def _get_terminal_approval(self) -> bool: | ||
| """Read terminal_approval from task_lock on every call. | ||
|
|
||
| This ensures the setting takes effect immediately when the user | ||
| toggles it between tasks (the task_lock is updated at POST /chat). | ||
| Also syncs Camel's safe_mode so it stays consistent. | ||
| """ | ||
| task_lock = get_task_lock_if_exists(self.api_task_id) | ||
| enabled = ( | ||
| task_lock.hitl_options.terminal_approval if task_lock else False | ||
| ) | ||
| self.safe_mode = not enabled | ||
| return enabled | ||
|
|
||
| async def _request_user_approval(self, action_data) -> str | None: | ||
| """Send a command approval request to the frontend and wait. | ||
|
|
||
| Flow:: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here is the flow |
||
|
|
||
| _request_user_approval (agent coroutine) | ||
| 1. create_approval(approval_id) → Future stored, agent will await it | ||
| 2. put_queue(action_data) → drops SSE event into shared queue | ||
| 3. await future → agent suspends | ||
|
|
||
| SSE generator in chat_service.py (independently) | ||
| 4. get_queue() → picks up the event | ||
| 5. yield sse_json(...) → sends command_approval to frontend | ||
|
|
||
| Frontend | ||
| 6. shows approval card | ||
|
|
||
| User clicks "Approve Once" / "Auto Approve" / "Reject" | ||
| 7. POST /approval → resolve_approval() or resolve_all_approvals_for_agent() | ||
| 8. future.set_result(...) → agent resumes at step 3 | ||
|
|
||
| Args: | ||
| action_data (ActionCommandApprovalData): SSE payload containing | ||
| the command and agent name. ``approval_id`` is injected into | ||
| ``action_data.data`` before the event is queued. | ||
|
|
||
| Returns: | ||
| None if the command is approved (approve_once or auto_approve). | ||
| An error string if the command is rejected. | ||
| """ | ||
| task_lock = get_task_lock(self.api_task_id) | ||
| if task_lock.auto_approve.get(self.agent_name, False): | ||
| return None | ||
|
|
||
| # Each concurrent call gets its own Future keyed by approval_id. | ||
| # Use str concatenation (not f-string) so that Enum values like | ||
| # Agents.developer_agent produce "developer_agent_..." instead of | ||
| # "Agents.developer_agent_..." — the latter breaks startswith() | ||
| # matching in resolve_all_approvals_for_agent. | ||
| approval_id = self.agent_name + "_" + uuid.uuid4().hex[:12] | ||
| action_data.data["approval_id"] = approval_id | ||
| future = task_lock.create_approval(approval_id) | ||
|
|
||
| logger.info( | ||
| "[APPROVAL] Pushing approval event to SSE queue, " | ||
| "api_task_id=%s, agent=%s, approval_id=%s", | ||
| self.api_task_id, | ||
| self.agent_name, | ||
| approval_id, | ||
| ) | ||
|
|
||
| await task_lock.put_queue(action_data) | ||
|
|
||
| logger.info("[APPROVAL] Event pushed, waiting for user response") | ||
|
|
||
| approval = await future | ||
|
|
||
| logger.info("[APPROVAL] Received response: %s", approval) | ||
|
|
||
| # Re-check: another concurrent call may have set auto_approve | ||
| # while this call was waiting on its Future. | ||
| if task_lock.auto_approve.get(self.agent_name, False): | ||
| return None | ||
|
|
||
| if approval == ApprovalAction.approve_once: | ||
| return None | ||
| if approval == ApprovalAction.auto_approve: | ||
| task_lock.auto_approve[self.agent_name] = True | ||
| # Unblock all other pending approvals for this agent | ||
| task_lock.resolve_all_approvals_for_agent( | ||
| self.agent_name, ApprovalAction.auto_approve | ||
| ) | ||
| return None | ||
| return "Operation rejected by user. The task is being stopped." | ||
|
|
||
| async def shell_exec( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed to async |
||
| self, | ||
| command: str, | ||
| id: str | None = None, | ||
|
|
@@ -358,6 +464,25 @@ def shell_exec( | |
| ) -> str: | ||
| r"""Executes a shell command in blocking or non-blocking mode. | ||
|
|
||
| When HITL terminal approval is on, commands that require user | ||
| confirmation trigger an approval request before execution. | ||
|
|
||
| .. note:: Async override of a sync base method | ||
|
|
||
| Camel's ``BaseTerminalToolkit.shell_exec`` is **sync-only** (no | ||
| async variant exists in the upstream library). We override it | ||
| as ``async def`` so the HITL approval flow can ``await`` the | ||
| SSE queue and the approval-response queue — following the same | ||
| asyncio.Queue pattern used by ``HumanToolkit.ask_human_via_gui``. | ||
|
|
||
| Because the base method is sync and this override is async, the | ||
| ``listen_toolkit`` decorator applies a ``__wrapped__`` fix (see | ||
| ``toolkit_listen.py``) to ensure Camel's ``FunctionTool`` | ||
| dispatches this method via ``async_call`` on the **main** event | ||
| loop, rather than via the sync ``__call__`` path which would run | ||
| it on a background loop where cross-loop ``asyncio.Queue`` awaits | ||
| silently fail. | ||
|
|
||
| Args: | ||
| command (str): The shell command to execute. | ||
| id (str, optional): A unique identifier for the command's session. | ||
|
|
@@ -368,12 +493,28 @@ def shell_exec( | |
| Returns: | ||
| str: The output of the command execution. | ||
| """ | ||
| # Auto-generate ID if not provided | ||
| if id is None: | ||
| import time | ||
|
|
||
| id = f"auto_{int(time.time() * 1000)}" | ||
|
|
||
| if not self._use_docker_backend: | ||
| ok, err = validate_cd_within_working_dir( | ||
| command, self._working_directory | ||
| ) | ||
| if not ok: | ||
| return err or "cd not allowed." | ||
|
|
||
| terminal_approval = self._get_terminal_approval() | ||
| is_dangerous = ( | ||
| is_dangerous_command(command) if terminal_approval else False | ||
| ) | ||
| if terminal_approval and is_dangerous: | ||
| approval_data = ActionCommandApprovalData( | ||
| data={"command": command, "agent": self.agent_name} | ||
| ) | ||
| rejection = await self._request_user_approval(approval_data) | ||
| if rejection is not None: | ||
| return rejection | ||
|
|
||
| result = super().shell_exec( | ||
| id=id, command=command, block=block, timeout=timeout | ||
| ) | ||
|
|
||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # ========= Copyright 2025-2026 @ Eigent.ai All Rights Reserved. ========= | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| # ========= Copyright 2025-2026 @ Eigent.ai All Rights Reserved. ========= |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| # ========= Copyright 2025-2026 @ Eigent.ai All Rights Reserved. ========= | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| # ========= Copyright 2025-2026 @ Eigent.ai All Rights Reserved. ========= | ||
|
|
||
| from pydantic import BaseModel | ||
|
|
||
| from app.model.enums import ApprovalAction | ||
|
|
||
|
|
||
| class HitlOptions(BaseModel): | ||
| terminal_approval: bool = False | ||
|
|
||
|
|
||
| class ApprovalRequest(BaseModel): | ||
| approval: ApprovalAction | ||
| agent: str | ||
| approval_id: str = "" |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this as this is not configured. And by default if the HITL not specified, we still use the safe_mode=True for the terminaltoolkit