From db302fd33c281289d7d6f725d07b59f7b8c5c29f Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Thu, 8 Aug 2024 21:14:45 +0800 Subject: [PATCH] fix: dubious ownership when running `git` (#3282) * switch default to eventstream runtime * remove pull docker from makefile * fix unittest * fix file store path * try deprecate server runtime * remove persist sandbox * move file utils * remove server runtime related workflow * remove unused method * attempt to remove the reliance on filestore for BE * fix async for list file * fix list_files to post * fix list files * add suffix to directory * make sure list file returns abs path; make sure other backend endpoints accpets abs path * remove server runtime test workflow * set git config in runtime * chown for workspace in client; use INIT_COMMANDS to maintain all commands that need to be run before bash start; * fix client issue; add test case for git; --------- Co-authored-by: Graham Neubig --- opendevin/runtime/client/client.py | 36 ++++++++++++-- tests/unit/test_runtime.py | 78 ++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 4 deletions(-) diff --git a/opendevin/runtime/client/client.py b/opendevin/runtime/client/client.py index 539858f411..572ba447eb 100644 --- a/opendevin/runtime/client/client.py +++ b/opendevin/runtime/client/client.py @@ -56,6 +56,14 @@ class ActionRequest(BaseModel): action: dict +ROOT_GID = 0 +INIT_COMMANDS = [ + 'git config --global user.name "opendevin"', + 'git config --global user.email "opendevin@all-hands.dev"', + "alias git='git --no-pager'", +] + + class RuntimeClient: """RuntimeClient is running inside docker sandbox. It is responsible for executing actions received from OpenDevin backend and producing observations. @@ -73,6 +81,7 @@ class RuntimeClient: self.username = username self.user_id = user_id self.pwd = work_dir # current PWD + self._initial_pwd = work_dir self._init_user(self.username, self.user_id) self._init_bash_shell(self.pwd, self.username) self.lock = asyncio.Lock() @@ -104,6 +113,8 @@ class RuntimeClient: ) logger.info(f'AgentSkills initialized: {obs}') + await self._init_bash_commands() + def _init_user(self, username: str, user_id: int) -> None: """Create user if not exists.""" # Skip root since it is already created @@ -117,11 +128,13 @@ class RuntimeClient: raise RuntimeError(f'Failed to add sudoer: {output.stderr.decode()}') logger.debug(f'Added sudoer successfully. Output: [{output.stdout.decode()}]') - # Add user + # Add user and change ownership of the initial working directory output = subprocess.run( ( f'useradd -rm -d /home/{username} -s /bin/bash ' - f'-g root -G sudo -u {user_id} {username}' + f'-g root -G sudo -u {user_id} {username} &&' + f'chown -R {username}:root {self.initial_pwd} && ' + f'chmod g+s {self.initial_pwd}' ), shell=True, capture_output=True, @@ -130,6 +143,7 @@ class RuntimeClient: raise RuntimeError( f'Failed to create user {username}: {output.stderr.decode()}' ) + logger.debug( f'Added user {username} successfully. Output: [{output.stdout.decode()}]' ) @@ -156,6 +170,20 @@ class RuntimeClient: f'Bash initialized. Working directory: {work_dir}. Output: {self.shell.before}' ) + async def _init_bash_commands(self): + logger.info(f'Initializing by running {len(INIT_COMMANDS)} bash commands...') + for command in INIT_COMMANDS: + action = CmdRunAction(command=command) + action.timeout = 300 + logger.debug(f'Executing init command: {command}') + obs: CmdOutputObservation = await self.run(action) + logger.debug( + f'Init command outputs (exit code: {obs.exit_code}): {obs.content}' + ) + assert obs.exit_code == 0 + + logger.info('Bash init commands completed') + def _get_bash_prompt_and_update_pwd(self): ps1 = self.shell.after @@ -353,11 +381,11 @@ class RuntimeClient: assert file_stat is not None # restore the original file permissions if the file already exists os.chmod(filepath, file_stat.st_mode) - os.chown(filepath, file_stat.st_uid, file_stat.st_gid) + os.chown(filepath, file_stat.st_uid, ROOT_GID) else: # set the new file permissions if the file is new os.chmod(filepath, 0o644) - os.chown(filepath, self.user_id, self.user_id) + os.chown(filepath, self.user_id, ROOT_GID) except FileNotFoundError: return ErrorObservation(f'File not found: {filepath}') diff --git a/tests/unit/test_runtime.py b/tests/unit/test_runtime.py index 63fe495977..0e1a402caf 100644 --- a/tests/unit/test_runtime.py +++ b/tests/unit/test_runtime.py @@ -1262,3 +1262,81 @@ async def test_keep_prompt(temp_dir): await runtime.close() await asyncio.sleep(1) + + +@pytest.mark.asyncio +async def test_git_operation(temp_dir, box_class): + runtime = await _load_runtime( + temp_dir, + box_class=box_class, + # Need to use non-root user to expose issues + run_as_devin=True, + ) + + # this will happen if permission of runtime is not properly configured + # fatal: detected dubious ownership in repository at '/workspace' + + # check the ownership of the current directory + action = CmdRunAction(command='ls -alh .') + logger.info(action, extra={'msg_type': 'ACTION'}) + obs = await runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert isinstance(obs, CmdOutputObservation) + assert obs.exit_code == 0 + # drwx--S--- 2 opendevin root 64 Aug 7 23:32 . + # drwxr-xr-x 1 root root 4.0K Aug 7 23:33 .. + for line in obs.content.split('\r\n'): + if ' ..' in line: + # parent directory should be owned by root + assert 'root' in line + assert 'opendevin' not in line + elif ' .' in line: + # current directory should be owned by opendevin + # and its group should be root + assert 'opendevin' in line + assert 'root' in line + + # make sure all git operations are allowed + action = CmdRunAction(command='git init') + logger.info(action, extra={'msg_type': 'ACTION'}) + obs = await runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert isinstance(obs, CmdOutputObservation) + assert obs.exit_code == 0 + + # create a file + action = CmdRunAction(command='echo "hello" > test_file.txt') + logger.info(action, extra={'msg_type': 'ACTION'}) + obs = await runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert isinstance(obs, CmdOutputObservation) + assert obs.exit_code == 0 + + # git add + action = CmdRunAction(command='git add test_file.txt') + logger.info(action, extra={'msg_type': 'ACTION'}) + obs = await runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert isinstance(obs, CmdOutputObservation) + assert obs.exit_code == 0 + + # git diff + action = CmdRunAction(command='git diff') + logger.info(action, extra={'msg_type': 'ACTION'}) + obs = await runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert isinstance(obs, CmdOutputObservation) + assert obs.exit_code == 0 + + # git commit + action = CmdRunAction(command='git commit -m "test commit"') + logger.info(action, extra={'msg_type': 'ACTION'}) + obs = await runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert isinstance(obs, CmdOutputObservation) + assert obs.exit_code == 0 + + await runtime.close() + + await runtime.close() + await asyncio.sleep(1)