Skip to content

fix(tools): add vision note to planning file editor #2358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
enyst wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(tools): add vision note to planning file editor #2358

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions openhands-tools/openhands/tools/planning_file_editor/definition.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,13 @@ class PlanningFileEditorObservation(FileEditorObservation):
"""


TOOL_DESCRIPTION = (
FILE_EDITOR_TOOL_DESCRIPTION
+ """

IMPORTANT RESTRICTION FOR PLANNING AGENT:
PLANNING_RESTRICTIONS = """IMPORTANT RESTRICTION FOR PLANNING AGENT:
* You can VIEW any file in the workspace using the 'view' command
* You can ONLY EDIT the PLAN.md file (all other edit operations will be rejected)
* PLAN.md is automatically initialized with section headers at the workspace root
* All editing commands (create, str_replace, insert, undo_edit) are restricted to PLAN.md only
* The PLAN.md file already contains the required section structure - you just need to fill in the content
""" # noqa
)
""" # noqa: E501


class PlanningFileEditorTool(
Expand Down Expand Up @@ -129,9 +124,25 @@ def create(
plan_path=plan_path,
)

description_lines = FILE_EDITOR_TOOL_DESCRIPTION.split("\n")
base_description = "\n".join(description_lines[:2])
remaining_description = "\n".join(description_lines[2:])

if conv_state.agent.llm.vision_is_active():
file_editor_description = (
f"{base_description}\n"
"* If `path` is an image file (.png, .jpg, .jpeg, .gif, .webp, "
".bmp), `view` displays the image content\n"
f"{remaining_description}"
)
else:
file_editor_description = FILE_EDITOR_TOOL_DESCRIPTION

tool_description = f"{file_editor_description}\n\n{PLANNING_RESTRICTIONS}"
Comment on lines +127 to +141
Copy link
Collaborator

@all-hands-bot all-hands-bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: Code Duplication

This vision description logic is now duplicated between file_editor/definition.py and planning_file_editor/definition.py. The same string splitting and conditional insertion happens in both places.

Problem: If the description format changes or the split point needs adjustment, both files must be updated. This violates DRY and increases maintenance burden.

Suggestion: Extract this into a shared helper function that both tools can use:

def build_tool_description_with_vision(
    base_description: str,
    vision_enabled: bool
) -> str:
    """Build tool description with optional vision note."""
    if not vision_enabled:
        return base_description
    
    description_lines = base_description.split("\n")
    base = "\n".join(description_lines[:2])
    remaining = "\n".join(description_lines[2:])
    
    return (
        f"{base}\n"
        "* If `path` is an image file (.png, .jpg, .jpeg, .gif, .webp, "
        ".bmp), `view` displays the image content\n"
        f"{remaining}"
    )

Then both tools can call this shared function. This would centralize the fragile string manipulation logic and make it easier to maintain.

Note: The [:2] split is fragile (magic number), but that's a pre-existing issue from file_editor - not introduced by this PR. Still worth addressing in a follow-up.

Copy link
Collaborator

@all-hands-bot all-hands-bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: Missing Test Coverage

The file_editor tool has tests for vision behavior:

  • test_file_editor_tool_image_viewing_line_with_vision_enabled()
  • test_file_editor_tool_image_viewing_line_with_vision_disabled()

This PR adds the same vision logic to planning_file_editor but doesn't add corresponding tests. Since this is a bug fix (#2357), tests would verify:

  1. The vision note appears when vision is enabled
  2. The vision note is absent when vision is disabled
  3. The PLANNING_RESTRICTIONS are still properly included in both cases

Suggestion: Add tests similar to the file_editor ones in tests/tools/planning_file_editor/test_planning_file_editor_tool.py:

def test_planning_file_editor_image_viewing_line_with_vision_enabled():
    """Test that image viewing line is included when LLM supports vision."""
    # Similar to file_editor test but verify PLANNING_RESTRICTIONS too
    
def test_planning_file_editor_image_viewing_line_with_vision_disabled():
    """Test that image viewing line is excluded when LLM doesn't support vision."""
    # Similar to file_editor test but verify PLANNING_RESTRICTIONS too


# Add working directory information to the tool description
enhanced_description = (
f"{TOOL_DESCRIPTION}\n\n"
f"{tool_description}\n\n"
f"Your current working directory: {working_dir}\n"
f"Your PLAN.md location: {plan_path}\n"
f"This plan file will be accessible to other agents in the workflow."
Expand Down
Loading