Skip to content

Conversation

robbie-portia
Copy link
Contributor

Original Task

Task: Delete legacy PlanBuilder

Summary

Delete the legacy PlanBuilder class located in portia/plan.py. This class has been superseded by PlanBuilderV2 and is no longer needed.

Acceptance Criteria

  • The PlanBuilder class is deleted from portia/plan.py.
  • All internal references to the old PlanBuilder are removed.
  • All tests associated with the old PlanBuilder are deleted.
  • The application compiles and all tests pass.

Additional Context

Based on the analysis of the provided context, here is a summary of relevant information for the coding agent:

Summary of Relevant Context

1. Technical Context & Codebase Structure

  • The legacy PlanBuilder class to be deleted is located in the file portia/plan.py.
  • This file also contains other legacy "V1" classes such as Plan, Step, ReadOnlyStep, Variable, PlanInput, and PlanContext. The scope of this ticket is only to remove the PlanBuilder class, its internal references, and its associated tests. The other classes in portia/plan.py will be handled in separate tasks.
  • The modern replacement for PlanBuilder is PlanBuilderV2, located in portia/builder/plan_builder_v2.py.
  • The technical analysis suggests that the legacy PlanBuilder class may already be marked as deprecated.

2. Important Clarifications & Requirements

  • The user-provided document "Plan Builder V2 Migration" explicitly lists "delete the previous iteration of the PlanBuilder" as a "ST" (Short Term) task, confirming its priority.

3. Architectural Considerations

  • This ticket is one of the initial steps in a larger migration epic to remove the entire "PlanV1" system and fully adopt "PlanV2".
  • A future step in the migration plan (but not part of this ticket) will be to rename PlanBuilderV2 to PlanBuilder once all legacy components are removed.

Agent Logs
I'll help you delete the legacy `PlanBuilder` class from `portia/plan.py` along with its references and tests. Let me start by exploring the codebase structure.
Now let me search for specific references to `PlanBuilder` (not `PlanBuilderV2`) to understand all usages:
Now let me check the test file specifically for PlanBuilder:
Now let me create a comprehensive task list and start the work:
Now let me check which test files actually use PlanBuilder and need updates:
Let me check the other test files that use PlanBuilder:
Now let me start the work. First, I'll update the todo list and begin:
Now let me delete the PlanBuilder class from portia/plan.py:
Good! Now let me update the todo list and proceed to the next step:
Now let me remove PlanBuilder from portia/__init__.py:
Now let me delete the test_plan_builder.py file:
Now let me update all the test files that import PlanBuilder. Let me start by reading the relevant sections and updating them:
Now I need to find where PlanBuilder() is used in test_portia.py and replace it. Let me check those usages:
I need to understand how PlanBuilderV2 works differently. Let me check:
Let me check if PlanBuilderV2 has a step() method and build() method similar to PlanBuilder:
Let me check what methods PlanBuilderV2 has for adding steps:
I see. PlanBuilderV2 has a completely different API. It doesn't have a generic `.step()` method. Instead, it uses `.invoke_tool_step()` for tool invocations. However, the test files are using the legacy PlanBuilder to create simple plans. 

Let me check if I can just create Plan objects directly instead:
I can see the tests are creating simple plans. The easiest approach is to create Plan objects directly. Let me replace all the PlanBuilder() usages with direct Plan construction. Let me find all usages and replace them:
Now let me find the two more complex usages at lines 2706 and 2751:
Let me search for the actual lines with PlanBuilder usage:
Good! Now I need to remove the import of PlanBuilderV2 that I added since we're not using it:
Now let's handle test_portia_async.py:
Now let me find and replace PlanBuilder usages in test_portia_async.py:
Now let's handle tests/unit/test_plan.py:
Now let's handle the remaining test files. Let me check tests/integration/test_e2e.py:
Let me read more context to identify the two instances:
Let me look for test function names to identify which test each belongs to:
Good, so one is in `test_run_plan_with_large_step_input()` and the other is in `test_arun_plan_with_large_step_input()`. Let me update them with more context:
Let me use replace_all:
Now let's handle the remaining test files. Let me check tests/integration/test_execution_agent.py:
Now let's handle the remaining test files. Let me check the unit test files:
Now let me find the other PlanBuilder usages in that file:
Let me continue with the remaining occurrences in this file. Let me read the rest to understand the pattern:
Let me continue with the remaining two occurrences:

Now let's handle the integration test file:

@openhands-ai
Copy link

openhands-ai bot commented Oct 8, 2025

Looks like there are a few issues preventing this PR from being merged!

  • GitHub Actions are failing:
    • Formatting (ruff)

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #820 at branch `agent/task-20251008-133411`

Feel free to include any additional details that might help me get this PR into a better state.

You can manage your notification settings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant