feat: Replace terminal dangerous-command block with HITL approval and add Safe Mode in Settings#1310
Conversation
|
@bytecii @Wendong-Fan Could you please review my PR? |
bytecii
left a comment
bytecii
left a comment
There was a problem hiding this comment.
IMO we can move this to camel to make a special safe mode such as strict. cc @Wendong-Fan
thanks @bytecii , the idea behind this issue is to implement a human-in-the-loop mechanism for safer code execution, which would involve the HumanToolkit. Since CAMEL serves more as a modular framework, it might be more appropriate to implement this feature in Eigent as an application layer. What do you think? |
Wendong-Fan
requested review from
4pmtong and
fengju0213
and removed request for
4pmtong
|
@Wendong-Fan @bytecii could you please review this pr? Regards |
- Move HitlOptions and ApprovalRequest models from chat.py to app/hitl/__init__.py - Remove unnecessary hasattr guard on auto_approve (always initialized) - Remove issue reference comment and docstring comments - Update all imports across controllers, services, and tests
- Move _request_user_approval from AbstractToolkit to TerminalToolkit (only terminal uses approval today) - Remove debug print statements from shell_exec - Move import time to top-level in terminal_toolkit - Fix approval endpoint docstring - Remove module docstring from terminal_command.py - Add comment for effective executable check - Update test to subclass TerminalToolkit directly
| options.project_id, | ||
| Agents.browser_agent, | ||
| working_directory=working_directory, | ||
| safe_mode=True, |
There was a problem hiding this comment.
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
| return None | ||
| return "Operation rejected by user. The task is being stopped." | ||
|
|
||
| async def shell_exec( |
- Remove unused logger from abstract_toolkit - Move _thread_local from AbstractToolkit to TerminalToolkit - Add missing threading.local() definition - Add Args/Returns to terminal_command.py functions - Use ApprovalAction.reject enum instead of raw string - Fix ApprovalAction and ActionCommandApprovalData docstrings - Add comment for auto_approve reset at task start
fengju0213
left a comment
fengju0213
left a comment
There was a problem hiding this comment.
thanks ！@spider-yamet and @bytecii left some comments below and pushed a commit
overall，looks good
| ) | ||
| return future | ||
|
|
||
| def resolve_approval(self, approval_id: str, action: str): |
There was a problem hiding this comment.
Approval callbacks may execute outside the Future owner loop/thread; the current restore paths here have a cross-thread wake-up risk, which can lead to “API succeeds but command execution does not continue”.
There was a problem hiding this comment.
We fixed the cross-thread wake-up risk by changing how approval resolution works:
- No resolution from the API thread – resolve_approval (and the other resolve entrypoints) no longer call future.set_result(...) directly. They only call _resolve_future_threadsafe.
- Resolution on the Future’s loop – _resolve_future_threadsafe gets the event loop that owns the Future via future.get_loop(), then schedules a callback on that loop with loop.call_soon_threadsafe(self._set_future_result_if_pending, future, action). The callback runs on the loop where the command is awaiting approval and only then calls future.set_result(action), so the wake-up always happens on the correct loop.
- Defensive checks and logging – We check future.done() and loop.is_closed(), handle RuntimeError from get_loop() and from call_soon_threadsafe (e.g. if the loop closes between the check and scheduling), and log warnings so “API succeeds but command execution does not continue” is visible if something goes wrong.
| # cd with no args or "cd ~" -> home; treat as potential escape | ||
| if not target or target == "~": | ||
| target = os.path.expanduser("~") | ||
| elif target == "-": | ||
| # "cd -" is previous dir; cannot validate statically, allow it | ||
| continue | ||
| try: | ||
| work_real = os.path.realpath(os.path.abspath(working_directory)) | ||
| if os.path.isabs(target): | ||
| resolved = os.path.realpath(os.path.abspath(target)) | ||
| else: | ||
| resolved = os.path.realpath( | ||
| os.path.abspath(os.path.join(work_real, target)) | ||
| ) | ||
| if os.path.commonpath([resolved, work_real]) != work_real: | ||
| return ( |
There was a problem hiding this comment.
If relative cd in compound commands is not resolved against step-by-step directory state, valid sequences (e.g., cd sub && cd ..) can be incorrectly flagged as escaping the workspace.
There was a problem hiding this comment.
We’ve addressed this by validating compound cd commands against a step-by-step directory state:
- State per sub-command – We iterate over each sub-command from split_compound_command and keep a simulated current_dir and previous_dir (both initially set to the canonical workspace path).
- Relative cd uses current dir – For each cd sub-command we resolve the target: if it’s relative we resolve it with os.path.join(current_dir, target) and then os.path.realpath(os.path.abspath(...)), so cd sub and cd .. are evaluated in the context of the directory left by the previous step.
- cd - – We treat cd - as “go to previous directory” by setting the target to previous_dir in the simulated state.
- State updates – After a valid cd we set previous_dir = current_dir and current_dir = resolved before the next sub-command, so a sequence like cd sub && cd .. is validated step-by-step and correctly stays inside the workspace instead of being flagged as escaping.
|
@fengju0213 @Wendong-Fan @bytecii I updated all the details by following the feedback, ready for review. :) |
|
could you please check @fengju0213 ? Hope we can complete this task ASAP... |
|
Could you let me know your plan, @fengju0213 ? |
|
Still waiting for review, @Wendong-Fan @bytecii.. Regards |
|
@fengju0213 Could you please review current PR? or hope you can let me know about any timeline for this, remaining for review status for several weeks.. Regards. |
Sorry for the delay. Our team has been extremely busy over the past few weeks. On the code side, we don't have any major issues at the moment. However, since this involves UI and user experience, we need our product team to review the UI part. I will ask them to provide feedback as soon as possible. Once again, apologies for the delay! |
4 participants
Related issue
Closes #1306
Summary
Replaces the Terminal Toolkit’s hard block of “dangerous” commands with a Human-in-the-Loop (HITL) approval flow and adds a Safe Mode setting so users can opt in.
Changes
HITL for dangerous commands
/chat/{id}/terminal-approvalwith the chosen option; backend enforces approval before running the command and supports “approve all in task” per task.Safe Mode in Settings
“With Safe Mode active, Eigent will pause and seek explicit approval whenever high-risk system operations are detected.”
Backend
sudo,su,reboot,shutdown), file (e.g.rm,chown,mount), disk (e.g.dd,mkfs,fdisk), process (e.g.service,systemctl), network (e.g.iptables,ifconfig), cron (e.g.crontab,at), user/kernel (e.g.useradd,modprobe), and related commands as specified.cdis validated so the agent cannot leave the designatedworking_directory.approved_all_dangerous_commandsand a queue for terminal approval; reset on new task.Frontend
localStorageand sent assafe_modein the start-task request.Other
uvis not installed so commits succeed withoutuv(message suggests installing uv for backend checks).Testing
cdoutsideworking_directoryin non-Docker mode is rejected.