Webui #831
Webui #831
Conversation
Summary of ChangesHello @vinci-grape, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a comprehensive web user interface for the MS-Agent framework, significantly improving user interaction and manageability. It establishes a FastAPI-powered backend for core functionalities like project and session management, coupled with a dynamic React frontend for an engaging user experience. The system supports real-time communication with agents via WebSockets, providing live updates on execution, logs, and progress, and offers flexible configuration options for LLMs and external services. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Web UI for the ms-agent project, featuring a FastAPI backend and a React frontend. The implementation is extensive and well-structured, providing a rich user experience for interacting with agents. My review focuses on enhancing the backend's robustness, security, and maintainability. Key recommendations include strengthening security in file handling, improving exception handling and logging, ensuring configuration consistency, and completing support for all frontend options in the backend logic. The frontend code is well-organized and effectively uses modern React practices.
| full_path = os.path.normpath(full_path) | ||
|
|
||
| # Security check: ensure file is within allowed directories | ||
| allowed_dirs = [output_dir, projects_dir] | ||
| is_allowed = any( | ||
| full_path.startswith(os.path.normpath(d)) for d in allowed_dirs) | ||
|
|
||
| if not is_allowed: | ||
| raise HTTPException( | ||
| status_code=403, | ||
| detail='Access denied: file outside allowed directories') |
There was a problem hiding this comment.
The security check to prevent path traversal is insufficient. Using os.path.normpath does not resolve symbolic links, which could allow an attacker to create a symlink within an allowed directory to read arbitrary files on the system. To mitigate this, you should use os.path.realpath() to resolve the absolute canonical path of both the requested path and the allowed directories before performing the check.
| full_path = os.path.normpath(full_path) | |
| # Security check: ensure file is within allowed directories | |
| allowed_dirs = [output_dir, projects_dir] | |
| is_allowed = any( | |
| full_path.startswith(os.path.normpath(d)) for d in allowed_dirs) | |
| if not is_allowed: | |
| raise HTTPException( | |
| status_code=403, | |
| detail='Access denied: file outside allowed directories') | |
| # Normalize path and resolve symlinks to prevent path traversal. | |
| full_path = os.path.realpath(full_path) | |
| # Security check: ensure file is within allowed directories | |
| allowed_dirs = [os.path.realpath(d) for d in [output_dir, projects_dir]] | |
| is_allowed = any(full_path.startswith(d) for d in allowed_dirs) | |
| if not is_allowed: | |
| raise HTTPException( | |
| status_code=403, | |
| detail='Access denied: file outside allowed directories') |
| async def stop(self): | ||
| """Stop the agent""" | ||
| self._stop_requested = True | ||
| self.is_running = False | ||
| if not self.process: | ||
| return | ||
|
|
||
| try: | ||
| # If already exited, nothing to do | ||
| if self.process.returncode is not None: | ||
| return | ||
|
|
||
| # Prefer terminating the whole process group to stop child processes too | ||
| try: | ||
| os.killpg(self.process.pid, signal.SIGTERM) | ||
| except Exception: | ||
| # Fallback to terminating only the parent | ||
| try: | ||
| self.process.terminate() | ||
| except Exception: | ||
| pass | ||
|
|
||
| try: | ||
| await asyncio.wait_for(self.process.wait(), timeout=5.0) | ||
| except asyncio.TimeoutError: | ||
| try: | ||
| os.killpg(self.process.pid, signal.SIGKILL) | ||
| except Exception: | ||
| try: | ||
| self.process.kill() | ||
| except Exception: | ||
| pass | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The stop method uses except Exception: pass in multiple places to handle errors during process termination. Silently ignoring all exceptions can hide underlying issues and make debugging difficult. It's better to at least log any exceptions that occur during the termination and kill process.
| else: | ||
| cmd = [python, '-m', 'ms_agent', 'run', '--config', project_path] |
There was a problem hiding this comment.
The else block provides a fallback command for unknown project types. This could lead to unexpected behavior if a new, unsupported project type is added. It would be safer to raise an exception for unhandled project types to make the system's behavior more predictable and prevent misconfiguration.
| else: | |
| cmd = [python, '-m', 'ms_agent', 'run', '--config', project_path] | |
| else: | |
| raise ValueError(f"Unsupported project type: {project_type}") |
| except Exception: | ||
| self._config = self.DEFAULT_CONFIG.copy() |
There was a problem hiding this comment.
Catching a broad Exception can mask various issues, such as permission errors or malformed JSON that isn't a JSONDecodeError. It's better to catch more specific exceptions (e.g., json.JSONDecodeError, IOError) and log the error for easier debugging.
| except Exception: | |
| self._config = self.DEFAULT_CONFIG.copy() | |
| except (json.JSONDecodeError, IOError) as e: | |
| print(f"Error loading config file: {e}") |
| provider = llm.get('provider', 'modelscope') | ||
| if provider == 'modelscope': | ||
| env_vars['MODELSCOPE_API_KEY'] = llm['api_key'] | ||
| elif provider == 'openai': | ||
| env_vars['OPENAI_API_KEY'] = llm['api_key'] | ||
| elif provider == 'anthropic': | ||
| env_vars['ANTHROPIC_API_KEY'] = llm['api_key'] |
There was a problem hiding this comment.
The get_env_vars method only sets API key environment variables for modelscope, openai, and anthropic providers. The frontend settings dialog also allows for deepseek and custom providers. This function should be updated to handle these providers to ensure their API keys are correctly passed to the agent's environment.
| provider = llm.get('provider', 'modelscope') | |
| if provider == 'modelscope': | |
| env_vars['MODELSCOPE_API_KEY'] = llm['api_key'] | |
| elif provider == 'openai': | |
| env_vars['OPENAI_API_KEY'] = llm['api_key'] | |
| elif provider == 'anthropic': | |
| env_vars['ANTHROPIC_API_KEY'] = llm['api_key'] | |
| provider = llm.get('provider', 'modelscope') | |
| if provider == 'modelscope': | |
| env_vars['MODELSCOPE_API_KEY'] = llm['api_key'] | |
| elif provider == 'openai': | |
| env_vars['OPENAI_API_KEY'] = llm['api_key'] | |
| elif provider == 'anthropic': | |
| env_vars['ANTHROPIC_API_KEY'] = llm['api_key'] | |
| elif provider == 'deepseek': | |
| env_vars['DEEPSEEK_API_KEY'] = llm['api_key'] |
| # CORS configuration | ||
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=['*'], |
There was a problem hiding this comment.
Using allow_origins=['*'] is insecure for production as it allows any website to make requests to your API. This should be restricted to the specific origin(s) of your frontend application to prevent security vulnerabilities like Cross-Site Request Forgery (CSRF). Consider loading the allowed origins from an environment variable for flexibility.
| allow_origins=['*'], | |
| allow_origins=os.getenv('CORS_ALLOWED_ORIGINS', 'http://localhost:5173').split(','), |
| return None | ||
|
|
||
| try: | ||
| import yaml |
There was a problem hiding this comment.
The import yaml statement is located inside the get_project_config method. According to PEP 8, imports should be at the top of the file. This improves readability and avoids the overhead of re-importing the module on every function call. If PyYAML is an optional dependency, this should be documented, and the ImportError should be caught specifically.
| # Initialize paths | ||
| BASE_DIR = os.path.dirname( | ||
| os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) | ||
| PROJECTS_DIR = os.path.join(BASE_DIR, 'projects') |
There was a problem hiding this comment.
There is an inconsistency in the definition of the projects directory. Here, PROJECTS_DIR is os.path.join(BASE_DIR, 'projects'), but webui/backend/api.py uses os.path.join(base_dir, 'ms-agent', 'projects'). This can lead to incorrect behavior and potential security issues. These paths should be consolidated to a single source of truth.
| PROJECTS_DIR = os.path.join(BASE_DIR, 'projects') | |
| PROJECTS_DIR = os.path.join(BASE_DIR, 'ms-agent', 'projects') |
Change Summary
upload index.html
Related issue number
Checklist
pre-commit installandpre-commit run --all-filesbefore git commit, and passed lint check.