-
Notifications
You must be signed in to change notification settings - Fork 0
feat(agent): ✨ Add goal-driven plan approval action #230
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,13 +11,15 @@ pub const IMPLEMENTATION_PLAN_SUPERSEDED_STATE: &str = "superseded"; | |
| pub enum PlanApprovalAction { | ||
| ApplyPlan, | ||
| ApplyPlanWithContextReset, | ||
| ApplyPlanWithGoal, | ||
| } | ||
|
|
||
| impl PlanApprovalAction { | ||
| pub fn label(&self) -> &'static str { | ||
| match self { | ||
| Self::ApplyPlan => "按计划实施", | ||
| Self::ApplyPlanWithContextReset => "清理上下文后按计划实施", | ||
| Self::ApplyPlanWithContextReset => "清理上下文后实施", | ||
| Self::ApplyPlanWithGoal => "按计划设置Goal后实施", | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -103,27 +105,44 @@ pub fn build_plan_message_metadata( | |
| } | ||
| } | ||
|
|
||
| pub fn build_approval_prompt_metadata( | ||
| pub async fn build_approval_prompt_metadata( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM] New async DB query on hot path without early return Every plan approval prompt now performs an additional async database query to check for existing goals. While this is a lightweight lookup, it adds latency to the approval flow and could be avoided by caching or deferring the check. Suggestion: Consider whether the goal-option visibility check can be performed only when the plan message is first built, or cache the existence of goals for the thread in memory. Alternatively, rely on the frontend to always send 'apply_plan_with_goal' and validate server-side only when the action is actually invoked. Risk: Slight increase in plan approval prompt construction latency (~1-5 ms per DB roundtrip). Not significant for interactive use but adds up under concurrent load. Confidence: 0.85 [From SubAgent: performance]
|
||
| pool: &sqlx::SqlitePool, | ||
| thread_id: &str, | ||
| plan_revision: u32, | ||
| plan_message_id: &str, | ||
| ) -> ApprovalPromptMetadata { | ||
| let mut options = vec![ | ||
| PlanApprovalOption { | ||
| action: PlanApprovalAction::ApplyPlan, | ||
| label: PlanApprovalAction::ApplyPlan.label().to_string(), | ||
| }, | ||
| PlanApprovalOption { | ||
| action: PlanApprovalAction::ApplyPlanWithContextReset, | ||
| label: PlanApprovalAction::ApplyPlanWithContextReset | ||
| .label() | ||
| .to_string(), | ||
| }, | ||
| ]; | ||
|
|
||
| // Only offer the goal-driven option when the thread has no existing goal | ||
| // record (any status: active, paused, budget_limited, or complete). | ||
| if crate::persistence::repo::goal_repo::find_by_thread_id(pool, thread_id) | ||
This comment was marked as outdated.
Sorry, something went wrong. |
||
| .await | ||
| .map(|opt| opt.is_none()) | ||
| .unwrap_or(false) | ||
| { | ||
| options.push(PlanApprovalOption { | ||
| action: PlanApprovalAction::ApplyPlanWithGoal, | ||
| label: PlanApprovalAction::ApplyPlanWithGoal.label().to_string(), | ||
| }); | ||
| } | ||
|
|
||
| ApprovalPromptMetadata { | ||
| kind: IMPLEMENTATION_PLAN_APPROVAL_KIND.to_string(), | ||
| plan_revision, | ||
| plan_message_id: plan_message_id.to_string(), | ||
| state: IMPLEMENTATION_PLAN_PENDING_STATE.to_string(), | ||
| options: vec![ | ||
| PlanApprovalOption { | ||
| action: PlanApprovalAction::ApplyPlan, | ||
| label: PlanApprovalAction::ApplyPlan.label().to_string(), | ||
| }, | ||
| PlanApprovalOption { | ||
| action: PlanApprovalAction::ApplyPlanWithContextReset, | ||
| label: PlanApprovalAction::ApplyPlanWithContextReset | ||
| .label() | ||
| .to_string(), | ||
| }, | ||
| ], | ||
| options, | ||
| expires_on_new_user_message: true, | ||
| approved_action: None, | ||
| } | ||
|
|
@@ -424,6 +443,24 @@ mod tests { | |
| parse_approval_prompt_metadata, parse_plan_message_metadata, plan_design_markdown, | ||
| plan_markdown, write_plan_file_to, PlanApprovalAction, | ||
| }; | ||
| use sqlx::sqlite::{SqliteConnectOptions, SqlitePoolOptions}; | ||
| use std::str::FromStr; | ||
|
|
||
| async fn setup_test_pool() -> sqlx::SqlitePool { | ||
| let options = SqliteConnectOptions::from_str("sqlite::memory:") | ||
| .expect("invalid sqlite options") | ||
| .foreign_keys(true); | ||
| let pool = SqlitePoolOptions::new() | ||
| .max_connections(1) | ||
| .connect_with(options) | ||
| .await | ||
| .expect("pool"); | ||
| sqlx::migrate!("./migrations") | ||
| .run(&pool) | ||
| .await | ||
| .expect("migrations"); | ||
| pool | ||
| } | ||
|
|
||
| #[test] | ||
| fn plan_artifact_builder_accepts_string_and_object_steps() { | ||
|
|
@@ -470,8 +507,9 @@ mod tests { | |
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn metadata_round_trip_is_stable() { | ||
| #[tokio::test] | ||
| async fn metadata_round_trip_is_stable() { | ||
| let pool = setup_test_pool().await; | ||
| let artifact = build_plan_artifact_from_tool_input( | ||
| &serde_json::json!({ | ||
| "title": "Implement checkpoint", | ||
|
|
@@ -484,11 +522,18 @@ mod tests { | |
| parse_plan_message_metadata(&serde_json::to_value(&metadata).unwrap()).unwrap(); | ||
| assert_eq!(parsed.artifact, artifact); | ||
|
|
||
| let approval = build_approval_prompt_metadata(artifact.plan_revision, "msg-plan"); | ||
| let approval = build_approval_prompt_metadata( | ||
| &pool, | ||
| "thread-no-goal", | ||
| artifact.plan_revision, | ||
| "msg-plan", | ||
| ) | ||
| .await; | ||
| let parsed_approval = | ||
| parse_approval_prompt_metadata(&serde_json::to_value(&approval).unwrap()).unwrap(); | ||
| assert_eq!(parsed_approval.plan_revision, 1); | ||
| assert_eq!(parsed_approval.options.len(), 2); | ||
| // With no existing goal, the third option (ApplyPlanWithGoal) is included. | ||
| assert_eq!(parsed_approval.options.len(), 3); | ||
| assert_eq!( | ||
| parsed_approval.options[0].action, | ||
| PlanApprovalAction::ApplyPlan | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
|
|
||
| import type { TranslationKey } from "@/i18n"; | ||
|
|
||
| export type PlanApprovalAction = "apply_plan" | "apply_plan_with_context_reset"; | ||
| export type PlanApprovalAction = "apply_plan" | "apply_plan_with_context_reset" | "apply_plan_with_goal"; | ||
|
|
||
| export type PlanStepMetadata = { | ||
| description?: string; | ||
|
|
@@ -152,7 +152,7 @@ export function parseApprovalPromptMetadata(value: unknown, t: (key: Translation | |
| const action = readStringField(optionRecord, "action"); | ||
| const label = readStringField(optionRecord, "label"); | ||
| if ( | ||
| (action !== "apply_plan" && action !== "apply_plan_with_context_reset") | ||
| (action !== "apply_plan" && action !== "apply_plan_with_context_reset" && action !== "apply_plan_with_goal") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW] Frontend data-path guard could miss new action values The frontend validation regex is duplicated in three places within the same function (option action parsing, and two Suggestion: Extract the valid action list into a constant set or array (e.g., Risk: Future additions of another action could accidentally skip one of the validation branches, resulting in inconsistent behavior where options are allowed but approved actions are rejected or vice versa. Confidence: 0.85 [From SubAgent: general]
|
||
| || !label | ||
| ) { | ||
| return null; | ||
|
|
@@ -167,6 +167,7 @@ export function parseApprovalPromptMetadata(value: unknown, t: (key: Translation | |
| approvedAction: | ||
| readStringField(record, "approvedAction") === "apply_plan" | ||
| || readStringField(record, "approvedAction") === "apply_plan_with_context_reset" | ||
| || readStringField(record, "approvedAction") === "apply_plan_with_goal" | ||
| ? (readStringField(record, "approvedAction") as PlanApprovalAction) | ||
| : null, | ||
| options: options.length > 0 | ||
|
|
||
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.