-
Notifications
You must be signed in to change notification settings - Fork 2
fix(tools): align Plan tool handler with schema definition #23
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.
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
Conversation
Greptile OverviewGreptile SummaryThis PR successfully aligns the Plan tool's schema definition with its handler implementation in Key ChangesSchema Updates (
Handler Updates (
ConcernsContainer Mode Incompatibility: The Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| src/cortex-engine/src/tools/handlers/plan.rs | Comprehensive refactor aligning handler with schema - adds rich plan metadata structures and markdown generation |
| src/cortex-app-server/src/tools/definitions.rs | Schema updated to include agent_analyses, architecture, tech_stack, and other rich planning fields |
Sequence Diagram
sequenceDiagram
participant LLM
participant ToolRouter
participant PlanHandler
participant TUI
LLM->>ToolRouter: Plan tool call with rich params
Note over LLM: {title, description, tasks,<br/>agent_analyses, architecture,<br/>tech_stack, risks, etc.}
ToolRouter->>PlanHandler: execute(arguments, context)
PlanHandler->>PlanHandler: Parse JSON to PlanArgs struct
Note over PlanHandler: Validates agent_analyses<br/>and tasks are present
PlanHandler->>PlanHandler: Validate max 50 tasks
PlanHandler->>PlanHandler: Convert PlanTaskInput → PlanTask
Note over PlanHandler: Map status strings to enum<br/>(pending/in_progress/completed)
PlanHandler->>PlanHandler: Convert AgentAnalysisInput → AgentAnalysis
PlanHandler->>PlanHandler: Generate rich markdown output
Note over PlanHandler: Includes sections:<br/>Architecture, Tech Stack, Tasks,<br/>Expert Analyses, Risks,<br/>Success Criteria, Timeline
PlanHandler->>PlanHandler: Build plan_data JSON
Note over PlanHandler: Full structured plan<br/>with status: "pending_approval"
PlanHandler-->>ToolRouter: ToolResult with markdown + metadata
ToolRouter-->>LLM: Success with formatted plan
LLM-->>TUI: Display plan for user approval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 1 comment
| } | ||
| }, | ||
| "required": ["title", "description", "tasks"] | ||
| "required": ["title", "description", "tasks", "agent_analyses"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema now requires agent_analyses, but src/cortex-app-server/src/tools/planning.rs:62-94 (container mode implementation) doesn't handle this parameter - it only processes title, description, tasks, and estimated_changes. This will cause failures when LLMs call the Plan tool in container mode with the new required agent_analyses field.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cortex-app-server/src/tools/definitions.rs
Line: 477:477
Comment:
The schema now requires `agent_analyses`, but `src/cortex-app-server/src/tools/planning.rs:62-94` (container mode implementation) doesn't handle this parameter - it only processes `title`, `description`, `tasks`, and `estimated_changes`. This will cause failures when LLMs call the Plan tool in container mode with the new required `agent_analyses` field.
How can I resolve this? If you propose a fix, please make it concise.The Plan tool had a critical mismatch between its schema definition and handler implementation: - Schema expected: title, description, tasks, agent_analyses (required) - Handler processed: only 'todos' string parameter This caused function calls to fail when LLMs generated Plan tool calls based on the schema but the handler couldn't process the rich parameters. Changes: - Updated PlanHandler to properly parse all schema parameters - Added support for architecture, tech_stack, use_cases, risks, success_criteria, timeline, and estimated_changes fields - Convert tasks and agent_analyses to internal formats - Generate rich markdown output with all plan sections - Aligned app-server definitions to match engine schema This ensures consistent Plan tool behavior across the codebase.
a699771 to
358838d
Compare
Address Greptile review feedback - the container mode Plan implementation in planning.rs was missing support for the new required field agent_analyses and other optional fields. Changes: - Added agent_analyses parameter handling (required) - Added optional fields: architecture, tech_stack, use_cases, risks, success_criteria, timeline - Build plan_data JSON with all fields for consistency
✅ Issue AddressedThe concern raised by Greptile about Looking at the current state:
All tests pass: The PR is ready for review and merge. |
Summary
The Plan tool had a critical mismatch between its schema definition and handler implementation:
title,description,tasks,agent_analyses(all required)todosstring parameterThis caused function calls to fail when LLMs generated Plan tool calls based on the schema but the handler couldn't process the rich parameters.
Changes
cortex-engine:
src/cortex-engine/src/tools/handlers/plan.rsPlanHandlerto properly parse all schema parametersPlanArgs,PlanTaskInput,AgentAnalysisInputarchitecture- High-level architecture descriptiontech_stack- Technologies and tools to be useduse_cases- User stories and use casesrisks- Identified risks with levels and mitigationssuccess_criteria- Definition of done criteriatimeline- Estimated completion timelineestimated_changes- Scope estimatecortex-app-server:
src/cortex-app-server/src/tools/definitions.rsTesting
cargo check -p cortex-engine -p cortex-app-serverpassesImpact
This ensures consistent Plan tool behavior across the codebase and fixes the broken function call flow for the Plan tool.