Add code operators refer to sandbox#1030
Conversation
Summary of ChangesHello @ririqing0614, 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 introduces new capabilities for evaluating generated code within a sandboxed environment. It adds a Highlights
Changelog
Activity
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
|
ChongjinHuang
requested a deployment
to
protected
GitHub Actions
Waiting
![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 code sandbox execution feature, which is a valuable addition. The use of process isolation for security is a good design choice. I've identified a few areas for improvement, including a critical bug in image data handling, some opportunities to improve resource management, and suggestions to enhance code maintainability and correctness. All original comments remain valid and are included for your review.
| init_captured_figures.append(base64.b64encode( | ||
| BytesIO(image.tobytes()).getvalue()).decode('utf-8')) |
There was a problem hiding this comment.
There is a bug in how images are being re-encoded to base64. image.tobytes() returns raw pixel data, not a formatted image file (like PNG or JPEG). When this base64 string is decoded later, it will not be a renderable image. You should save the PIL image to a memory buffer in a specific format to get the correct byte representation.
For example:
with BytesIO() as buf:
image.save(buf, format='PNG')
b64_str = base64.b64encode(buf.getvalue()).decode('utf-8')
init_captured_figures.append(b64_str)|
|
||
| return image | ||
|
|
||
| except (base64.binascii.Error, Exception) as e: |
There was a problem hiding this comment.
Catching the generic Exception is too broad. It can hide bugs and make debugging harder by catching unexpected errors. It's better to catch only the specific exceptions you expect from the try block, such as base64.binascii.Error and Image.UnidentifiedImageError from Pillow.
| except (base64.binascii.Error, Exception) as e: | |
| except (base64.binascii.Error, Image.UnidentifiedImageError) as e: |
|
|
||
| def exec_code(self, code_piece: str) -> None: | ||
| # Security check | ||
| if regex.search(r'(\s|^)?(input|os\.system|subprocess)\(', code_piece): |
There was a problem hiding this comment.
The regex used for the security check is a weak blacklist that can be easily bypassed (e.g., using __import__). While process isolation is the main security layer, this weak check might provide a false sense of security. It would be better to either remove it or replace it with a more robust sandboxing approach that restricts available built-ins and modules within the exec scope.
| else: | ||
| # Non-isolation mode (for backward compatibility) | ||
| runtime = runtime_class(messages) if runtime_class else self.runtime_class(messages) | ||
|
|
||
| try: | ||
| if get_answer_from_stdout: | ||
| program_io = io.StringIO() | ||
| with redirect_stdout(program_io): | ||
| runtime.exec_code('\n'.join(code)) | ||
| program_io.seek(0) | ||
| result = program_io.read() | ||
| elif answer_symbol: | ||
| runtime.exec_code('\n'.join(code)) | ||
| result = runtime._global_vars.get(answer_symbol, '') | ||
| elif answer_expr: | ||
| runtime.exec_code('\n'.join(code)) | ||
| result = runtime.eval_code(answer_expr) | ||
| else: | ||
| if len(code) > 1: | ||
| runtime.exec_code('\n'.join(code[:-1])) | ||
| result = runtime.eval_code(code[-1]) | ||
| else: | ||
| runtime.exec_code('\n'.join(code)) | ||
| result = '' | ||
|
|
||
| # Check for captured figures | ||
| captured_figures = runtime.captured_figures | ||
| if captured_figures: | ||
| result = { | ||
| 'text': result, | ||
| 'images': captured_figures | ||
| } if result else {'images': captured_figures} | ||
| else: | ||
| result = {'text': result} if result else {} | ||
|
|
||
| report = 'Done' | ||
|
|
||
| except Exception as e: | ||
| error_msg = f'{type(e).__name__}: {str(e)}' | ||
| result = { | ||
| 'error': error_msg, | ||
| 'traceback': traceback.format_exc() | ||
| } | ||
| report = f'Error: {error_msg}' | ||
|
|
||
| return result, report |
There was a problem hiding this comment.
The code in this else block (for non-isolated execution) is very similar to the logic inside PersistentWorker._handle_execute. This duplication increases maintenance overhead, as any bug fixes or changes will need to be applied in two places. Consider refactoring to reuse the same execution logic for both isolated and non-isolated paths.
| except TimeoutError as error: | ||
| LOG.warning(error) | ||
| all_exec_results.append(('', 'Timeout Error')) | ||
| timeout_cnt += 1 |
There was a problem hiding this comment.
This except TimeoutError block appears to be unreachable. The self.execute() method handles timeouts internally by catching queue.Empty and returns a dictionary with an error status, rather than raising a TimeoutError. This block seems to be a remnant of a previous implementation and should be removed to avoid confusion.
| def __del__(self): | ||
| '''Clean up resources.''' | ||
| if self.persistent_worker: | ||
| self.persistent_worker.terminate() | ||
|
|
There was a problem hiding this comment.
Relying on __del__ for resource cleanup is unreliable as its execution is not guaranteed. A better approach is to provide an explicit terminate() method and implement the context manager protocol (__enter__/__exit__). This allows for deterministic cleanup of the worker process using a with statement.
I'd suggest replacing __del__ with a terminate() method and advising users to call it explicitly or use a with block.
| def __del__(self): | |
| '''Clean up resources.''' | |
| if self.persistent_worker: | |
| self.persistent_worker.terminate() | |
| def terminate(self): | |
| '''Clean up resources.''' | |
| if self.persistent_worker: | |
| self.persistent_worker.terminate() | |
| self.persistent_worker = None |
1 participant
📌 PR 内容 / PR Description
✅ 变更类型 / Type of Change