[tool] feat: add image zoom in tool#2372
Conversation
![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 PR adds a new ImageZoomInTool for zooming into images, along with its configuration. The implementation of the tool in verl/tools/image_zoom_in_tool.py has a few critical and high-severity issues. These include a path traversal vulnerability and incorrect method return types that will cause TypeErrors. I've provided specific suggestions to address these issues to improve the correctness and security of the new tool.
| "response": "", | ||
| "reward": 0.0, | ||
| } | ||
| return instance_id, None |
There was a problem hiding this comment.
The method's type hint is -> str, but it returns a tuple (instance_id, None). This will lead to a TypeError where the function is called. The base class BaseTool.create also specifies a str return type. The method should only return the instance_id to match the declared type hint and the base class implementation.
| return instance_id, None | |
| return instance_id |
| try: | ||
| left, top, right, bottom = bbox_2d | ||
| cropped_image = image.crop((left, top, right, bottom)) | ||
| cropped_image.save(f"cropped_image_{instance_id}.png") |
There was a problem hiding this comment.
This line is vulnerable to path traversal. The create method allows instance_id to be passed as an argument. If a malicious instance_id such as ../../tmp/pwned is provided, this could allow writing files outside of the intended directory.
You should sanitize instance_id before using it in a file path, for example with os.path.basename(instance_id).
Additionally, writing files to the current working directory is a side effect that can be problematic (e.g., filling up disk space, permissions issues). It would be safer to use a temporary directory via the tempfile module.
| } | ||
| return instance_id, None | ||
|
|
||
| async def execute(self, instance_id: str, parameters: dict[str, Any], **kwargs) -> Tuple[str, float, dict]: |
There was a problem hiding this comment.
The method signature indicates a return type of Tuple[str, float, dict], but the implementation can return a dict as the first element of the tuple (on lines 97 and 102). This violates the type hint and can lead to unexpected behavior in code that relies on this type hint. Please update the type hint to Tuple[Union[str, dict], float, dict] to reflect the actual return types. You will need to add Union to the typing import.
| async def execute(self, instance_id: str, parameters: dict[str, Any], **kwargs) -> Tuple[str, float, dict]: | |
| async def execute(self, instance_id: str, parameters: dict[str, Any], **kwargs) -> Tuple[Union[str, dict], float, dict]: |
1 participant
What does this PR do?
This PR builds upon #2146 to implement an image zoom in tool that can be called during multi-turn VLM interactions. Based on DeepEyes (https://github.com/Visual-Agent/DeepEyes, https://arxiv.org/abs/2505.14362), this tool enables verification of image-related responses through dynamic zooming capabilities.
Checklist Before Starting
[{modules}] {type}: {description}(This will be checked by the CI){modules}includefsdp,megatron,sglang,vllm,rollout,trainer,ci,training_utils,recipe,hardware,deployment,ray,worker,single_controller,misc,perf,model,algo,env,tool,ckpt,doc,data,like[megatron, fsdp, doc]{type}is infeat,fix,refactor,chore,test[BREAKING]to the beginning of the title.[BREAKING][fsdp, megatron] feat: dynamic batchingTest
API and Usage Example
# Add code snippet or script demonstrating how to use thisHigh-Level Design
Specific Changes
Checklist Before Submitting
Important
Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=alwaysci-requestchannel in theverlSlack workspace.