fix: revert the part of patch of issue-710 to extract the content from the plan (#718)
This commit is contained in:
parent
41c6120b4f
commit
2e177bc17c
|
|
@ -181,29 +181,6 @@ def validate_and_fix_plan(plan: dict, enforce_web_search: bool = False) -> dict:
|
||||||
}
|
}
|
||||||
]
|
]
|
||||||
|
|
||||||
# ============================================================
|
|
||||||
# SECTION 3: Ensure required Plan fields are present (Issue #710 fix)
|
|
||||||
# ============================================================
|
|
||||||
# Set locale from state if not present
|
|
||||||
if "locale" not in plan or not plan.get("locale"):
|
|
||||||
plan["locale"] = "en-US" # Default locale
|
|
||||||
logger.info("Added missing locale field with default value 'en-US'")
|
|
||||||
|
|
||||||
# Ensure has_enough_context is present
|
|
||||||
if "has_enough_context" not in plan:
|
|
||||||
plan["has_enough_context"] = False # Default value
|
|
||||||
logger.info("Added missing has_enough_context field with default value 'False'")
|
|
||||||
|
|
||||||
# Ensure title is present
|
|
||||||
if "title" not in plan or not plan.get("title"):
|
|
||||||
# Try to infer title from steps or use a default
|
|
||||||
if steps and isinstance(steps[0], dict) and "title" in steps[0]:
|
|
||||||
plan["title"] = steps[0]["title"]
|
|
||||||
logger.info(f"Inferred missing title from first step: {plan['title']}")
|
|
||||||
else:
|
|
||||||
plan["title"] = "Research Plan" # Default title
|
|
||||||
logger.info("Added missing title field with default value 'Research Plan'")
|
|
||||||
|
|
||||||
return plan
|
return plan
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -351,6 +328,10 @@ def planner_node(
|
||||||
|
|
||||||
try:
|
try:
|
||||||
curr_plan = json.loads(repair_json_output(full_response))
|
curr_plan = json.loads(repair_json_output(full_response))
|
||||||
|
# Need to extract the plan from the full_response
|
||||||
|
curr_plan_content = extract_plan_content(curr_plan)
|
||||||
|
# load the current_plan
|
||||||
|
curr_plan = json.loads(repair_json_output(curr_plan_content))
|
||||||
except json.JSONDecodeError:
|
except json.JSONDecodeError:
|
||||||
logger.warning("Planner response is not a valid JSON")
|
logger.warning("Planner response is not a valid JSON")
|
||||||
if plan_iterations > 0:
|
if plan_iterations > 0:
|
||||||
|
|
@ -476,15 +457,17 @@ def human_feedback_node(
|
||||||
try:
|
try:
|
||||||
# Safely extract plan content from different types (string, AIMessage, dict)
|
# Safely extract plan content from different types (string, AIMessage, dict)
|
||||||
original_plan = current_plan
|
original_plan = current_plan
|
||||||
current_plan_content = extract_plan_content(current_plan)
|
|
||||||
logger.debug(f"Extracted plan content type: {type(current_plan_content).__name__}")
|
|
||||||
|
|
||||||
# Repair the JSON output
|
# Repair the JSON output
|
||||||
current_plan = repair_json_output(current_plan_content)
|
current_plan = repair_json_output(current_plan)
|
||||||
|
# parse the plan to dict
|
||||||
|
current_plan = json.loads(current_plan)
|
||||||
|
current_plan_content = extract_plan_content(current_plan)
|
||||||
|
|
||||||
# increment the plan iterations
|
# increment the plan iterations
|
||||||
plan_iterations += 1
|
plan_iterations += 1
|
||||||
# parse the plan
|
# parse the plan
|
||||||
new_plan = json.loads(current_plan)
|
new_plan = json.loads(current_plan_content)
|
||||||
# Validate and fix plan to ensure web search requirements are met
|
# Validate and fix plan to ensure web search requirements are met
|
||||||
configurable = Configuration.from_runnable_config(config)
|
configurable = Configuration.from_runnable_config(config)
|
||||||
new_plan = validate_and_fix_plan(new_plan, configurable.enforce_web_search)
|
new_plan = validate_and_fix_plan(new_plan, configurable.enforce_web_search)
|
||||||
|
|
|
||||||
|
|
@ -362,155 +362,6 @@ class TestValidateAndFixPlanIntegration:
|
||||||
assert result["steps"][1]["step_type"] == "processing"
|
assert result["steps"][1]["step_type"] == "processing"
|
||||||
assert result["steps"][1]["need_search"] is False
|
assert result["steps"][1]["need_search"] is False
|
||||||
|
|
||||||
|
|
||||||
class TestValidateAndFixPlanIssue710:
|
|
||||||
"""Specific tests for Issue #710 scenarios - missing required Plan fields."""
|
|
||||||
|
|
||||||
def test_missing_locale_field_added(self):
|
|
||||||
"""Test that missing locale field is added with default value."""
|
|
||||||
plan = {
|
|
||||||
"has_enough_context": True,
|
|
||||||
"title": "Test Plan",
|
|
||||||
"steps": []
|
|
||||||
}
|
|
||||||
|
|
||||||
result = validate_and_fix_plan(plan)
|
|
||||||
|
|
||||||
assert "locale" in result
|
|
||||||
assert result["locale"] == "en-US"
|
|
||||||
|
|
||||||
def test_empty_locale_field_replaced(self):
|
|
||||||
"""Test that empty locale field is replaced with default value."""
|
|
||||||
plan = {
|
|
||||||
"locale": "",
|
|
||||||
"has_enough_context": True,
|
|
||||||
"title": "Test Plan",
|
|
||||||
"steps": []
|
|
||||||
}
|
|
||||||
|
|
||||||
result = validate_and_fix_plan(plan)
|
|
||||||
|
|
||||||
assert "locale" in result
|
|
||||||
assert result["locale"] == "en-US"
|
|
||||||
|
|
||||||
def test_missing_has_enough_context_field_added(self):
|
|
||||||
"""Test that missing has_enough_context field is added with default value."""
|
|
||||||
plan = {
|
|
||||||
"locale": "en-US",
|
|
||||||
"title": "Test Plan",
|
|
||||||
"steps": []
|
|
||||||
}
|
|
||||||
|
|
||||||
result = validate_and_fix_plan(plan)
|
|
||||||
|
|
||||||
assert "has_enough_context" in result
|
|
||||||
assert result["has_enough_context"] is False
|
|
||||||
|
|
||||||
def test_missing_title_field_added_from_step(self):
|
|
||||||
"""Test that missing title field is inferred from first step."""
|
|
||||||
plan = {
|
|
||||||
"locale": "en-US",
|
|
||||||
"has_enough_context": True,
|
|
||||||
"steps": [
|
|
||||||
{
|
|
||||||
"need_search": True,
|
|
||||||
"title": "Step Title",
|
|
||||||
"description": "Step description",
|
|
||||||
"step_type": "research"
|
|
||||||
}
|
|
||||||
]
|
|
||||||
}
|
|
||||||
|
|
||||||
result = validate_and_fix_plan(plan)
|
|
||||||
|
|
||||||
assert "title" in result
|
|
||||||
assert result["title"] == "Step Title"
|
|
||||||
|
|
||||||
def test_missing_title_field_added_default(self):
|
|
||||||
"""Test that missing title field is added with default when no steps available."""
|
|
||||||
plan = {
|
|
||||||
"locale": "en-US",
|
|
||||||
"has_enough_context": True,
|
|
||||||
"steps": []
|
|
||||||
}
|
|
||||||
|
|
||||||
result = validate_and_fix_plan(plan)
|
|
||||||
|
|
||||||
assert "title" in result
|
|
||||||
assert result["title"] == "Research Plan"
|
|
||||||
|
|
||||||
def test_all_required_fields_missing(self):
|
|
||||||
"""Test that all missing required fields are added."""
|
|
||||||
plan = {
|
|
||||||
"steps": [
|
|
||||||
{
|
|
||||||
"need_search": True,
|
|
||||||
"title": "Step 1",
|
|
||||||
"description": "Description",
|
|
||||||
"step_type": "research"
|
|
||||||
}
|
|
||||||
]
|
|
||||||
}
|
|
||||||
|
|
||||||
result = validate_and_fix_plan(plan)
|
|
||||||
|
|
||||||
# All required fields should be present
|
|
||||||
assert "locale" in result
|
|
||||||
assert "has_enough_context" in result
|
|
||||||
assert "title" in result
|
|
||||||
|
|
||||||
# With appropriate defaults
|
|
||||||
assert result["locale"] == "en-US"
|
|
||||||
assert result["has_enough_context"] is False
|
|
||||||
assert result["title"] == "Step 1" # Inferred from first step
|
|
||||||
|
|
||||||
def test_issue_710_scenario_passes_pydantic_validation(self):
|
|
||||||
"""Test that fixed plan can be validated by Pydantic schema (reproduces issue #710 fix)."""
|
|
||||||
from src.prompts.planner_model import Plan as PlanModel
|
|
||||||
|
|
||||||
# Simulate the problematic plan from issue #710 that's missing required fields
|
|
||||||
plan = {
|
|
||||||
"content": '{\n "locale": "en-US",\n "has_enough_context": false,\n "title": "Test Plan",\n "steps": [\n {\n "need_search": true,\n "title": "Research Step",\n "description": "Gather data",\n "step_type": "research"\n }\n ]\n}',
|
|
||||||
"reasoning": 2368
|
|
||||||
}
|
|
||||||
|
|
||||||
# Extract just the JSON part (simulating what would happen in the actual flow)
|
|
||||||
import json
|
|
||||||
json_content = json.loads(plan["content"])
|
|
||||||
|
|
||||||
# Remove required fields to simulate the issue
|
|
||||||
del json_content["locale"]
|
|
||||||
del json_content["has_enough_context"]
|
|
||||||
del json_content["title"]
|
|
||||||
|
|
||||||
# First validate and fix
|
|
||||||
fixed_plan = validate_and_fix_plan(json_content)
|
|
||||||
|
|
||||||
# Then try Pydantic validation (should not raise)
|
|
||||||
validated = PlanModel.model_validate(fixed_plan)
|
|
||||||
|
|
||||||
# Validation should succeed
|
|
||||||
assert validated.locale == "en-US"
|
|
||||||
assert validated.has_enough_context is False
|
|
||||||
assert validated.title == "Research Step" # Inferred from first step
|
|
||||||
|
|
||||||
def test_existing_fields_preserved(self):
|
|
||||||
"""Test that existing valid fields are preserved."""
|
|
||||||
plan = {
|
|
||||||
"locale": "zh-CN",
|
|
||||||
"has_enough_context": True,
|
|
||||||
"title": "Existing Title",
|
|
||||||
"steps": []
|
|
||||||
}
|
|
||||||
|
|
||||||
result = validate_and_fix_plan(plan)
|
|
||||||
|
|
||||||
# Existing values should be preserved
|
|
||||||
assert result["locale"] == "zh-CN"
|
|
||||||
assert result["has_enough_context"] is True
|
|
||||||
assert result["title"] == "Existing Title"
|
|
||||||
|
|
||||||
|
|
||||||
class TestValidateAndFixPlanIssue650:
|
class TestValidateAndFixPlanIssue650:
|
||||||
"""Specific tests for Issue #650 scenarios."""
|
"""Specific tests for Issue #650 scenarios."""
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue