diff --git a/docs/forge-hardening-execution-plan.json b/docs/forge-hardening-execution-plan.json new file mode 100644 index 0000000000..55fd717120 --- /dev/null +++ b/docs/forge-hardening-execution-plan.json @@ -0,0 +1,148 @@ +{ + "$schema": "plan-v1", + "id": "forge-hardening-execution", + "title": "Forge Hardening Execution", + "goal": "Implement Forge architecture, template, and quality improvements with test-first, task-scoped commits and a PR.", + "context": { + "tech_stack": [ + "TypeScript", + "React", + "Vitest", + "Rust", + "Tauri" + ], + "constraints": [ + "Tests first for each task", + "One commit per task", + "Keep app/daemon behavior parity", + "Do not break existing Forge contracts" + ], + "references": [ + { + "path": "src/features/forge/components/Forge.tsx", + "description": "Forge UI orchestration and polling" + }, + { + "path": "src/features/forge/hooks/useForgeExecution.ts", + "description": "Execution loop and phase orchestration" + }, + { + "path": "src-tauri/src/shared/forge_execute_core.rs", + "description": "Execution backend source of truth" + }, + { + "path": "src-tauri/src/bin/codex_monitor_daemon/rpc/workspace.rs", + "description": "Daemon RPC workspace dispatcher" + } + ] + }, + "tasks": [ + { + "id": "task-1", + "name": "Wire Forge Daemon RPC Routing", + "description": "Add missing Forge RPC method routing in daemon dispatcher and cover it with RPC-level tests so remote mode can invoke every forge_* command through JSON-RPC.", + "depends_on": [], + "files": [ + "src-tauri/src/bin/codex_monitor_daemon/rpc/workspace.rs", + "src-tauri/src/bin/codex_monitor_daemon.rs" + ], + "verification": [ + "New daemon RPC tests fail before implementation and pass after.", + "forge_list_plans and forge_get_plan_prompt routes are exercised via rpc::handle_rpc_request.", + "No regression in existing daemon tests for workspace/codex methods." + ] + }, + { + "id": "task-2", + "name": "Enforce AI Review Evidence Gate", + "description": "Implement machine-enforced AI review completion by requiring a structured per-task review report artifact and validating it in Forge checks before final task completion.", + "depends_on": [ + "task-1" + ], + "files": [ + "src-tauri/src/shared/forge_execute_core.rs", + "src-tauri/resources/forge/templates/test-first-loop/prompts/execute.md", + "src-tauri/resources/forge/templates/test-first-loop/phases.json" + ], + "verification": [ + "Forge execute core tests cover missing report, non-zero findings, and zero-findings success paths.", + "ai-review phase can no longer pass with empty/no evidence artifact.", + "Existing ai-review happy-path test remains green with valid artifact." + ] + }, + { + "id": "task-3", + "name": "Add Risk-Adaptive Template", + "description": "Add a new forge template optimized for efficiency (risk-adaptive-loop) with tighter phase flow and script/prompt coverage so teams can choose a faster default model when appropriate.", + "depends_on": [ + "task-2" + ], + "files": [ + "src-tauri/resources/forge/templates/risk-adaptive-loop/template.json", + "src-tauri/resources/forge/templates/risk-adaptive-loop/phases.json", + "src-tauri/resources/forge/templates/risk-adaptive-loop/prompts/plan.md", + "src-tauri/resources/forge/templates/risk-adaptive-loop/prompts/execute.md", + "src-tauri/resources/forge/templates/risk-adaptive-loop/scripts/post-plan.mjs", + "src-tauri/resources/forge/templates/risk-adaptive-loop/scripts/post-step.mjs", + "src/features/forge/scripts/riskAdaptiveLoopScripts.test.ts" + ], + "verification": [ + "Script tests prove phase initialization and prompt phase progression for the new template.", + "Bundled template listing includes risk-adaptive-loop metadata.", + "Template manifest files and entrypoints are complete and installable." + ] + }, + { + "id": "task-4", + "name": "Harden Forge Polling", + "description": "Refine Forge panel polling to prevent stale async results from mutating state after workspace/plan switches and centralize polling intervals into constants.", + "depends_on": [ + "task-3" + ], + "files": [ + "src/features/forge/components/Forge.tsx", + "src/features/forge/components/Forge.plans.test.tsx" + ], + "verification": [ + "Component tests validate stale polling responses are ignored after selection/workspace change.", + "Polling intervals are no longer duplicated magic numbers.", + "No regression in plan-selection and execution-toggle tests." + ] + }, + { + "id": "task-5", + "name": "Bound Execution Retries and Waits", + "description": "Add explicit timeout and retry-budget controls in useForgeExecution for phase-final polling and failed phase checks to avoid infinite loops and runaway runs.", + "depends_on": [ + "task-4" + ], + "files": [ + "src/features/forge/hooks/useForgeExecution.ts", + "src/features/forge/hooks/useForgeExecution.test.ts" + ], + "verification": [ + "Hook tests fail before and pass after for timeout and max-check-retry behavior.", + "Execution returns user-visible errors when budgets are exceeded.", + "Existing Forge execution UI tests stay green." + ] + }, + { + "id": "task-6", + "name": "Fresh Skill Sync and Icon Fallback Cleanup", + "description": "Improve template skill synchronization so changed source skills refresh in .agents/skills and align frontend phase icon fallback IDs with valid icon names.", + "depends_on": [ + "task-5" + ], + "files": [ + "src-tauri/src/shared/forge_templates_core.rs", + "src/services/tauri.ts", + "src/services/tauri.test.ts" + ], + "verification": [ + "New rust test proves sync updates stale files when source content changes.", + "Phase metadata fallback icon id is valid and consistent.", + "tauri forge phase-view tests pass with updated fallback behavior." + ] + } + ] +} diff --git a/src-tauri/resources/forge/templates/test-first-loop/phases.json b/src-tauri/resources/forge/templates/test-first-loop/phases.json index b380669493..6f539ed254 100644 --- a/src-tauri/resources/forge/templates/test-first-loop/phases.json +++ b/src-tauri/resources/forge/templates/test-first-loop/phases.json @@ -52,7 +52,7 @@ "iconId": "folder-review", "order": 6, "goal": "Run an AI quality review pass and enforce a strict zero-findings completion gate.", - "description": "Execute an AI review over the final change set and treat any finding as a release blocker for this phase. Completion checks: (1) AI review is run against the final diff, (2) all findings are resolved and rechecked, and (3) the phase is marked completed only with zero findings. If any finding remains, phase status must stay non-completed (`failed` or `blocked`) and execution must stop until fixes are applied and the review is rerun clean.", + "description": "Execute an AI review over the final change set and treat any finding as a release blocker for this phase. Completion checks: (1) AI review is run against the final diff, (2) all findings are resolved and rechecked, (3) `plans//ai-review/.json` is written with schema `forge-ai-review-v1`, matching `taskId`, and `findings: []`, and (4) the phase is marked completed only with zero findings. If any finding remains, phase status must stay non-completed (`failed` or `blocked`) and execution must stop until fixes are applied and the review is rerun clean.", "checks": [] } ] diff --git a/src-tauri/resources/forge/templates/test-first-loop/prompts/execute.md b/src-tauri/resources/forge/templates/test-first-loop/prompts/execute.md index 13806a6406..9d7ef2c2ad 100644 --- a/src-tauri/resources/forge/templates/test-first-loop/prompts/execute.md +++ b/src-tauri/resources/forge/templates/test-first-loop/prompts/execute.md @@ -75,6 +75,17 @@ If any completion check is unmet, do not mark the phase `completed`; use `in_pro - Forge finalizes phase/task completion statuses after checks. - If checks fail, Forge reopens the phase and you retry. - If checks pass on the last phase, Forge creates the task commit and records `commit_sha`. + - For `ai-review`, Forge requires a report file at `plans/{{plan_id}}/ai-review/{{current_task_id}}.json` with: + +```json +{ + "schema": "forge-ai-review-v1", + "taskId": "{{current_task_id}}", + "findings": [] +} +``` + + - If any finding remains, include each finding in `findings` and keep phase status non-completed (`blocked` or `failed`). 5. Do NOT run `git` commands yourself in execute mode. - Do NOT run `git add`, `git commit`, `git commit --amend`, or `git push`. - Do NOT set `commit_sha` in `state.json`; Forge manages it. diff --git a/src-tauri/src/bin/codex_monitor_daemon.rs b/src-tauri/src/bin/codex_monitor_daemon.rs index 47006eb34f..0dad2be63d 100644 --- a/src-tauri/src/bin/codex_monitor_daemon.rs +++ b/src-tauri/src/bin/codex_monitor_daemon.rs @@ -1760,6 +1760,61 @@ mod tests { let _ = std::fs::remove_dir_all(&tmp); }); } + + #[test] + fn rpc_forge_list_bundled_templates_routes_to_daemon_state() { + run_async_test(async { + let tmp = make_temp_dir("rpc-forge-list-bundled"); + let state = test_state(&tmp); + + let result = rpc::handle_rpc_request( + &state, + "forge_list_bundled_templates", + json!({}), + "daemon-test".to_string(), + ) + .await + .expect("forge_list_bundled_templates should succeed"); + + let templates = result.as_array().expect("array result"); + assert!( + templates.iter().any(|entry| { + entry + .get("id") + .and_then(Value::as_str) + .is_some_and(|id| id == "ralph-loop") + }), + "expected forge_list_bundled_templates to include ralph-loop" + ); + let _ = std::fs::remove_dir_all(&tmp); + }); + } + + #[test] + fn rpc_forge_list_plans_routes_to_workspace_handler() { + run_async_test(async { + let tmp = make_temp_dir("rpc-forge-list-plans"); + let workspace_id = "ws-forge-plans"; + let workspace_dir = tmp.join("workspace"); + std::fs::create_dir_all(workspace_dir.join("plans")).expect("create workspace plans dir"); + + let state = test_state(&tmp); + insert_workspace(&state, workspace_id, &workspace_dir.to_string_lossy()).await; + + let result = rpc::handle_rpc_request( + &state, + "forge_list_plans", + json!({ "workspaceId": workspace_id }), + "daemon-test".to_string(), + ) + .await + .expect("forge_list_plans should succeed"); + + let plans = result.as_array().expect("array result"); + assert!(plans.is_empty(), "expected no plans in empty plans directory"); + let _ = std::fs::remove_dir_all(&tmp); + }); + } } fn main() { diff --git a/src-tauri/src/bin/codex_monitor_daemon/rpc/workspace.rs b/src-tauri/src/bin/codex_monitor_daemon/rpc/workspace.rs index d26b8c36d5..82803049c5 100644 --- a/src-tauri/src/bin/codex_monitor_daemon/rpc/workspace.rs +++ b/src-tauri/src/bin/codex_monitor_daemon/rpc/workspace.rs @@ -242,6 +242,173 @@ pub(super) async fn try_handle( }; Some(serde_json::to_value(response).map_err(|err| err.to_string())) } + "forge_list_bundled_templates" => { + let templates = match state.forge_list_bundled_templates().await { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + Some(serde_json::to_value(templates).map_err(|err| err.to_string())) + } + "forge_get_installed_template" => { + let workspace_id = match parse_string(params, "workspaceId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let template = match state.forge_get_installed_template(workspace_id).await { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + Some(serde_json::to_value(template).map_err(|err| err.to_string())) + } + "forge_install_template" => { + let workspace_id = match parse_string(params, "workspaceId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let template_id = match parse_string(params, "templateId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let lock = match state.forge_install_template(workspace_id, template_id).await { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + Some(serde_json::to_value(lock).map_err(|err| err.to_string())) + } + "forge_uninstall_template" => { + let workspace_id = match parse_string(params, "workspaceId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + Some( + state + .forge_uninstall_template(workspace_id) + .await + .map(|_| json!({ "ok": true })), + ) + } + "forge_list_plans" => { + let workspace_id = match parse_string(params, "workspaceId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let plans = match state.forge_list_plans(workspace_id).await { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + Some(serde_json::to_value(plans).map_err(|err| err.to_string())) + } + "forge_get_plan_prompt" => { + let workspace_id = match parse_string(params, "workspaceId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + Some( + state + .forge_get_plan_prompt(workspace_id) + .await + .map(Value::String), + ) + } + "forge_prepare_execution" => { + let workspace_id = match parse_string(params, "workspaceId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let plan_id = match parse_string(params, "planId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + Some( + state + .forge_prepare_execution(workspace_id, plan_id) + .await + .map(|_| json!({ "ok": true })), + ) + } + "forge_reset_execution_progress" => { + let workspace_id = match parse_string(params, "workspaceId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let plan_id = match parse_string(params, "planId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + Some( + state + .forge_reset_execution_progress(workspace_id, plan_id) + .await + .map(|_| json!({ "ok": true })), + ) + } + "forge_get_next_phase_prompt" => { + let workspace_id = match parse_string(params, "workspaceId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let plan_id = match parse_string(params, "planId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let next = match state.forge_get_next_phase_prompt(workspace_id, plan_id).await { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + Some(serde_json::to_value(next).map_err(|err| err.to_string())) + } + "forge_get_phase_status" => { + let workspace_id = match parse_string(params, "workspaceId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let plan_id = match parse_string(params, "planId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let task_id = match parse_string(params, "taskId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let phase_id = match parse_string(params, "phaseId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let status = match state + .forge_get_phase_status(workspace_id, plan_id, task_id, phase_id) + .await + { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + Some(serde_json::to_value(status).map_err(|err| err.to_string())) + } + "forge_run_phase_checks" => { + let workspace_id = match parse_string(params, "workspaceId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let plan_id = match parse_string(params, "planId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let task_id = match parse_string(params, "taskId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let phase_id = match parse_string(params, "phaseId") { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + let result = match state + .forge_run_phase_checks(workspace_id, plan_id, task_id, phase_id) + .await + { + Ok(value) => value, + Err(err) => return Some(Err(err)), + }; + Some(serde_json::to_value(result).map_err(|err| err.to_string())) + } "file_read" => { let request = match parse_file_read_request(params) { Ok(value) => value, diff --git a/src-tauri/src/shared/forge_execute_core.rs b/src-tauri/src/shared/forge_execute_core.rs index c5ee1081a4..046551eac3 100644 --- a/src-tauri/src/shared/forge_execute_core.rs +++ b/src-tauri/src/shared/forge_execute_core.rs @@ -16,6 +16,7 @@ use crate::utils::{git_env_path, resolve_git_binary}; const CHECK_TIMEOUT_SECONDS_DEFAULT: u64 = 10 * 60; const HOOK_TIMEOUT_SECONDS: u64 = 2 * 60; const GIT_COMMAND_TIMEOUT_SECONDS: u64 = 90; +const AI_REVIEW_REPORT_SCHEMA: &str = "forge-ai-review-v1"; #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -649,6 +650,119 @@ async fn run_phase_check(workspace_root: &Path, check: &RunnablePhaseCheck) -> F } } +fn run_ai_review_report_check(paths: &ForgeExecutionPaths, task_id: &str) -> ForgePhaseCheckResultV1 { + let start = Instant::now(); + let report_path = paths + .plan_dir + .join("ai-review") + .join(format!("{task_id}.json")); + + let failure = |stderr: String, duration_ms: i64| ForgePhaseCheckResultV1 { + id: "ai-review-report".to_string(), + title: "AI review report has zero findings".to_string(), + exit_code: 2, + duration_ms, + stdout: String::new(), + stderr, + timed_out: false, + }; + + if !report_path.is_file() { + return failure( + format!( + "Missing AI review report: {}. Expected JSON with findings: [].", + report_path.display() + ), + start.elapsed().as_millis() as i64, + ); + } + + let raw = match fs::read_to_string(&report_path) { + Ok(value) => value, + Err(err) => { + return failure( + format!("Failed to read AI review report {}: {err}", report_path.display()), + start.elapsed().as_millis() as i64, + ) + } + }; + let report: Value = match serde_json::from_str(&raw) { + Ok(value) => value, + Err(err) => { + return failure( + format!("Invalid JSON in AI review report {}: {err}", report_path.display()), + start.elapsed().as_millis() as i64, + ) + } + }; + + let schema = report + .get("schema") + .and_then(Value::as_str) + .map(str::trim) + .unwrap_or(""); + if schema != AI_REVIEW_REPORT_SCHEMA { + return failure( + format!( + "AI review report schema must be {AI_REVIEW_REPORT_SCHEMA} (got: {}).", + if schema.is_empty() { "" } else { schema } + ), + start.elapsed().as_millis() as i64, + ); + } + + let report_task_id = report + .get("taskId") + .and_then(Value::as_str) + .map(str::trim) + .unwrap_or(""); + if report_task_id != task_id { + return failure( + format!( + "AI review report taskId mismatch (expected {task_id}, got {}).", + if report_task_id.is_empty() { + "" + } else { + report_task_id + } + ), + start.elapsed().as_millis() as i64, + ); + } + + let findings = match report.get("findings").and_then(Value::as_array) { + Some(value) => value, + None => { + return failure( + "AI review report must include a findings array.".to_string(), + start.elapsed().as_millis() as i64, + ) + } + }; + if !findings.is_empty() { + return failure( + format!( + "AI review report contains {} finding(s); resolve all findings before marking ai-review complete.", + findings.len() + ), + start.elapsed().as_millis() as i64, + ); + } + + ForgePhaseCheckResultV1 { + id: "ai-review-report".to_string(), + title: "AI review report has zero findings".to_string(), + exit_code: 0, + duration_ms: start.elapsed().as_millis() as i64, + stdout: format!( + "Verified zero findings in {}.", + report_path.to_string_lossy() + ), + stderr: String::new(), + timed_out: false, + } +} + fn task_has_commit_sha(task: &StateTaskV2) -> bool { task.commit_sha .as_ref() @@ -992,6 +1106,14 @@ pub(crate) async fn forge_run_phase_checks_core( .iter() .all(|result| !result.timed_out && result.exit_code == 0); + if phase_id == "ai-review" { + let ai_review_result = run_ai_review_report_check(&paths, task_id); + if ai_review_result.timed_out || ai_review_result.exit_code != 0 { + ok = false; + } + results.push(ai_review_result); + } + if let Some(task) = state.tasks.get_mut(task_index) { if let Some(phase) = task.phases.get_mut(phase_index) { if ok { @@ -1317,6 +1439,30 @@ await fs.writeFile(ctx.generatedExecutePromptPath, 'generated prompt from post-s .expect("phase in task") } + fn write_ai_review_report( + workspace: &Path, + plan_id: &str, + task_id: &str, + findings: &[&str], + ) { + let report_path = workspace + .join("plans") + .join(plan_id) + .join("ai-review") + .join(format!("{task_id}.json")); + if let Some(parent) = report_path.parent() { + std::fs::create_dir_all(parent).expect("create ai-review report directory"); + } + write_json( + &report_path, + json!({ + "schema": "forge-ai-review-v1", + "taskId": task_id, + "findings": findings, + }), + ); + } + #[test] fn get_next_phase_prompt_regenerates_even_when_cached_prompt_exists() { run_async_test(async { @@ -1565,6 +1711,68 @@ await fs.writeFile(ctx.generatedExecutePromptPath, 'fresh prompt from post-step\ }); } + #[test] + fn run_phase_checks_final_ai_review_requires_report_artifact() { + run_async_test(async { + let fixture = setup_six_phase_workspace( + "in_progress", + [ + "completed", + "completed", + "completed", + "completed", + "completed", + "pending", + ], + 0, + ); + init_git_repo(&fixture.root); + + let result = forge_run_phase_checks_core(&fixture.root, "alpha", "task-1", "ai-review") + .await + .expect("run phase checks"); + assert!(!result.ok); + assert!(result.results.iter().any(|check| check.id == "ai-review-report")); + + let task = load_state_task(&fixture.root, "alpha", "task-1"); + assert_eq!(task.status, "in_progress"); + assert_eq!(task.commit_sha, None); + assert_eq!(phase_status(&task, "ai-review"), "in_progress"); + }); + } + + #[test] + fn run_phase_checks_final_ai_review_fails_when_report_has_findings() { + run_async_test(async { + let fixture = setup_six_phase_workspace( + "in_progress", + [ + "completed", + "completed", + "completed", + "completed", + "completed", + "pending", + ], + 0, + ); + init_git_repo(&fixture.root); + write_ai_review_report(&fixture.root, "alpha", "task-1", &["missing test for edge case"]); + + let result = forge_run_phase_checks_core(&fixture.root, "alpha", "task-1", "ai-review") + .await + .expect("run phase checks"); + assert!(!result.ok); + assert!(result.results.iter().any(|check| check.id == "ai-review-report")); + assert!(!result.results.iter().any(|check| check.id == "forge-commit")); + + let task = load_state_task(&fixture.root, "alpha", "task-1"); + assert_eq!(task.status, "in_progress"); + assert_eq!(task.commit_sha, None); + assert_eq!(phase_status(&task, "ai-review"), "in_progress"); + }); + } + #[test] fn run_phase_checks_final_ai_review_success_completes_task_and_records_commit() { run_async_test(async { @@ -1581,6 +1789,7 @@ await fs.writeFile(ctx.generatedExecutePromptPath, 'fresh prompt from post-step\ 0, ); init_git_repo(&fixture.root); + write_ai_review_report(&fixture.root, "alpha", "task-1", &[]); let result = forge_run_phase_checks_core(&fixture.root, "alpha", "task-1", "ai-review") .await diff --git a/src-tauri/src/shared/forge_templates_core.rs b/src-tauri/src/shared/forge_templates_core.rs index 1206e14bb8..7cd4b2549c 100644 --- a/src-tauri/src/shared/forge_templates_core.rs +++ b/src-tauri/src/shared/forge_templates_core.rs @@ -274,7 +274,11 @@ pub(crate) fn sync_agent_skills_into_repo_agents_dir_core(workspace_root: &Path) .map_err(|_| "Invalid .agent/skills file path.".to_string())?; let dest_path = codex_skills_root.join(rel); if dest_path.exists() { - continue; + let src_contents = fs::read(&src_path).map_err(|err| err.to_string())?; + let dest_contents = fs::read(&dest_path).map_err(|err| err.to_string())?; + if src_contents == dest_contents { + continue; + } } copy_file(&src_path, &dest_path)?; } @@ -506,6 +510,27 @@ mod tests { let _ = std::fs::remove_dir_all(&workspace); } + #[test] + fn sync_agent_skills_updates_existing_repo_skill_when_source_changes() { + let workspace = temp_workspace_root(); + let source_path = workspace.join(".agent").join("skills").join("plan").join("SKILL.md"); + let dest_path = workspace.join(".agents").join("skills").join("plan").join("SKILL.md"); + + std::fs::create_dir_all(source_path.parent().expect("source parent")) + .expect("create source parent"); + std::fs::create_dir_all(dest_path.parent().expect("dest parent")) + .expect("create dest parent"); + + std::fs::write(&source_path, "new skill instructions\n").expect("write source"); + std::fs::write(&dest_path, "stale skill instructions\n").expect("write destination"); + + sync_agent_skills_into_repo_agents_dir_core(&workspace).expect("sync skills"); + + let dest_contents = std::fs::read_to_string(&dest_path).expect("read destination"); + assert_eq!(dest_contents, "new skill instructions\n"); + let _ = std::fs::remove_dir_all(&workspace); + } + #[test] fn read_manifest_rejects_unknown_schema() { let root = temp_bundled_templates_root(); diff --git a/src/features/forge/components/Forge.plans.test.tsx b/src/features/forge/components/Forge.plans.test.tsx index 0b8f29ee0b..dd5088faa3 100644 --- a/src/features/forge/components/Forge.plans.test.tsx +++ b/src/features/forge/components/Forge.plans.test.tsx @@ -410,6 +410,114 @@ describe("Forge plans", () => { expect(screen.getByRole("menuitemradio", { name: "Alpha (alpha)" })).toBeTruthy(); }); + it("ignores stale in-flight plan responses after workspace switch", async () => { + type Deferred = { + promise: Promise; + resolve: (value: T) => void; + }; + + function deferred(): Deferred { + let resolve: ((value: T) => void) | null = null; + const promise = new Promise((innerResolve) => { + resolve = innerResolve; + }); + return { + promise, + resolve: (value: T) => { + if (!resolve) { + throw new Error("deferred resolve not initialized"); + } + resolve(value); + }, + }; + } + + const ws1Plans = deferred(); + const listPlans = vi + .fn() + .mockImplementation(async (workspaceId) => { + if (workspaceId === "ws-1") { + return ws1Plans.promise; + } + return [ + { + id: "beta", + title: "Beta", + goal: "Beta goal", + tasks: [ + { + id: "task-1", + name: "Task 1", + status: "pending", + }, + ], + currentTaskId: null, + planPath: "plans/beta.json", + updatedAtMs: 0, + }, + ]; + }); + + const plansClient: ForgePlansClient = { + listPlans, + getPlanPrompt: async () => "", + prepareExecution: async () => {}, + resetExecutionProgress: async () => {}, + getNextPhasePrompt: async () => null, + getPhaseStatus: async () => ({ status: "pending", commitSha: null }), + runPhaseChecks: async () => ({ ok: true, results: [] }), + interruptTurn: async () => ({}), + connectWorkspace: async () => {}, + startThread: async () => ({ result: { thread: { id: "thread-1" } } }), + sendUserMessage: async () => ({}), + }; + + const rendered = render( + , + ); + + rendered.rerender( + , + ); + + ws1Plans.resolve([ + { + id: "alpha", + title: "Alpha", + goal: "Alpha goal", + tasks: [ + { + id: "task-1", + name: "Task 1", + status: "pending", + }, + ], + currentTaskId: null, + planPath: "plans/alpha.json", + updatedAtMs: 0, + }, + ]); + + await waitFor(() => { + expect(listPlans).toHaveBeenCalledWith("ws-1"); + expect(listPlans).toHaveBeenCalledWith("ws-2"); + }); + + fireEvent.click(screen.getByRole("button", { name: /Click to select/i })); + expect(screen.getByRole("menuitemradio", { name: "Beta (beta)" })).toBeTruthy(); + expect(screen.queryByRole("menuitemradio", { name: "Alpha (alpha)" })).toBeNull(); + }); + it("starts a new plan thread in plan mode and injects the plan prompt", async () => { const connectWorkspace = vi.fn(async () => {}); const startThread = vi.fn(async () => ({ result: { thread: { id: "thread-123" } } })); diff --git a/src/features/forge/components/Forge.tsx b/src/features/forge/components/Forge.tsx index 6a07d95035..b50c066476 100644 --- a/src/features/forge/components/Forge.tsx +++ b/src/features/forge/components/Forge.tsx @@ -138,6 +138,8 @@ const EMPTY_PHASE_VIEW: ForgePhaseView = { type ForgePhaseChipState = "is-complete" | "is-current" | "is-pending"; type ForgeRunningInfo = { taskId: string; phaseId: string } | null; +const FORGE_PLANS_POLL_INTERVAL_MS = 2000; +const FORGE_PHASE_VIEW_POLL_INTERVAL_MS = 2000; function formatPlanLabel(plan: ForgeWorkspacePlan): string { const title = plan.title?.trim() ?? ""; @@ -468,7 +470,14 @@ export function Forge({ const [installedTemplate, setInstalledTemplate] = useState(null); const [workspacePlans, setWorkspacePlans] = useState([]); const [isResettingProgress, setIsResettingProgress] = useState(false); - const plansInFlightRef = useRef(false); + const plansInFlightRef = useRef>(new Set()); + const plansRequestSeqRef = useRef(0); + const latestPlansRequestByWorkspaceRef = useRef>(new Map()); + const activeWorkspaceIdRef = useRef(activeWorkspaceId); + + useEffect(() => { + activeWorkspaceIdRef.current = activeWorkspaceId; + }, [activeWorkspaceId]); useEffect(() => { let cancelled = false; @@ -513,17 +522,33 @@ export function Forge({ const refreshPlans = useCallback( async (workspaceId: string) => { - if (plansInFlightRef.current) { + const normalizedWorkspaceId = workspaceId.trim(); + if (!normalizedWorkspaceId) { + return; + } + + const inFlight = plansInFlightRef.current; + if (inFlight.has(normalizedWorkspaceId)) { return; } - plansInFlightRef.current = true; + inFlight.add(normalizedWorkspaceId); + + const requestId = plansRequestSeqRef.current + 1; + plansRequestSeqRef.current = requestId; + latestPlansRequestByWorkspaceRef.current.set(normalizedWorkspaceId, requestId); + try { - const next = await listPlans(workspaceId); + const next = await listPlans(normalizedWorkspaceId); + const latestRequestId = latestPlansRequestByWorkspaceRef.current.get(normalizedWorkspaceId); + const latestWorkspaceId = (activeWorkspaceIdRef.current ?? "").trim(); + if (latestRequestId !== requestId || latestWorkspaceId !== normalizedWorkspaceId) { + return; + } setWorkspacePlans(next); } catch (error) { console.warn("Failed to load Forge plans.", { error }); } finally { - plansInFlightRef.current = false; + inFlight.delete(normalizedWorkspaceId); } }, [listPlans], @@ -533,6 +558,8 @@ export function Forge({ let cancelled = false; if (!activeWorkspaceId) { setWorkspacePlans([]); + plansInFlightRef.current.clear(); + latestPlansRequestByWorkspaceRef.current.clear(); return () => { cancelled = true; }; @@ -546,7 +573,7 @@ export function Forge({ }; void loadPlans(); - const interval = window.setInterval(loadPlans, 2000); + const interval = window.setInterval(loadPlans, FORGE_PLANS_POLL_INTERVAL_MS); return () => { cancelled = true; window.clearInterval(interval); @@ -665,7 +692,7 @@ export function Forge({ }; void refreshPhaseView(); - const interval = window.setInterval(refreshPhaseView, 2000); + const interval = window.setInterval(refreshPhaseView, FORGE_PHASE_VIEW_POLL_INTERVAL_MS); return () => { cancelled = true; window.clearInterval(interval); diff --git a/src/features/forge/hooks/useForgeExecution.test.ts b/src/features/forge/hooks/useForgeExecution.test.ts new file mode 100644 index 0000000000..173b792a22 --- /dev/null +++ b/src/features/forge/hooks/useForgeExecution.test.ts @@ -0,0 +1,148 @@ +// @vitest-environment jsdom +import { act, renderHook } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { + ForgeNextPhasePrompt, + ForgePhaseStatus, + ForgeRunPhaseChecksResponse, +} from "../../../services/tauri"; +import { useForgeExecution } from "./useForgeExecution"; + +type HookArgs = Parameters[0]; + +function buildBaseArgs(overrides: Partial = {}): HookArgs { + return { + workspaceId: "ws-1", + knownTaskIds: ["task-1"], + connectWorkspace: async () => {}, + prepareExecution: async () => {}, + getNextPhasePrompt: async () => null, + getPhaseStatus: async () => ({ status: "pending", commitSha: null }), + runPhaseChecks: async () => ({ ok: true, results: [] }), + interruptTurn: async () => ({}), + startThread: async () => ({ result: { thread: { id: "thread-1" } } }), + sendUserMessage: async () => ({ result: { turn: { id: "turn-1" } } }), + ...overrides, + }; +} + +afterEach(() => { + vi.useRealTimers(); +}); + +describe("useForgeExecution execution limits", () => { + it("fails execution when phase status never reaches a terminal state before timeout", async () => { + vi.useFakeTimers(); + + const getNextPhasePrompt = vi + .fn() + .mockResolvedValue({ + planId: "alpha", + taskId: "task-1", + phaseId: "implementation", + isLastPhase: false, + promptText: "phase prompt", + } satisfies ForgeNextPhasePrompt); + + const getPhaseStatus = vi + .fn() + .mockResolvedValue({ status: "pending", commitSha: null } satisfies ForgePhaseStatus); + + const runPhaseChecks = vi + .fn() + .mockResolvedValue({ ok: true, results: [] } satisfies ForgeRunPhaseChecksResponse); + + const { result } = renderHook(() => + useForgeExecution({ + ...buildBaseArgs({ + getNextPhasePrompt, + getPhaseStatus, + runPhaseChecks, + }), + executionLimits: { + phaseStatusPollIntervalMs: 10, + phaseStatusTimeoutMs: 40, + maxPhaseCheckFailures: 2, + }, + } as HookArgs & { + executionLimits: { + phaseStatusPollIntervalMs: number; + phaseStatusTimeoutMs: number; + maxPhaseCheckFailures: number; + }; + }), + ); + + await act(async () => { + void result.current.startExecution("alpha"); + }); + + await act(async () => { + await vi.advanceTimersByTimeAsync(800); + }); + + expect(result.current.lastError).toContain("timed out"); + expect(runPhaseChecks).not.toHaveBeenCalled(); + + await act(async () => { + await result.current.pauseExecution(); + }); + }); + + it("fails execution after max repeated phase-check failures", async () => { + vi.useFakeTimers(); + + const phase: ForgeNextPhasePrompt = { + planId: "alpha", + taskId: "task-1", + phaseId: "implementation", + isLastPhase: false, + promptText: "phase prompt", + }; + const getNextPhasePrompt = vi + .fn() + .mockResolvedValue(phase); + const getPhaseStatus = vi + .fn() + .mockResolvedValue({ status: "completed", commitSha: null } satisfies ForgePhaseStatus); + const runPhaseChecks = vi + .fn() + .mockResolvedValue({ ok: false, results: [] } satisfies ForgeRunPhaseChecksResponse); + + const { result } = renderHook(() => + useForgeExecution({ + ...buildBaseArgs({ + getNextPhasePrompt, + getPhaseStatus, + runPhaseChecks, + }), + executionLimits: { + phaseStatusPollIntervalMs: 10, + phaseStatusTimeoutMs: 200, + maxPhaseCheckFailures: 2, + }, + } as HookArgs & { + executionLimits: { + phaseStatusPollIntervalMs: number; + phaseStatusTimeoutMs: number; + maxPhaseCheckFailures: number; + }; + }), + ); + + await act(async () => { + void result.current.startExecution("alpha"); + }); + + await act(async () => { + await vi.advanceTimersByTimeAsync(1500); + }); + + expect(result.current.lastError).toContain("reached max check failures"); + expect(runPhaseChecks).toHaveBeenCalledTimes(2); + + await act(async () => { + await result.current.pauseExecution(); + }); + }); +}); diff --git a/src/features/forge/hooks/useForgeExecution.ts b/src/features/forge/hooks/useForgeExecution.ts index b3a0d5f425..20083b4c99 100644 --- a/src/features/forge/hooks/useForgeExecution.ts +++ b/src/features/forge/hooks/useForgeExecution.ts @@ -50,6 +50,11 @@ type ForgeExecutionArgs = { ) => Promise; onSelectThread?: (workspaceId: string, threadId: string) => void; collaborationMode?: Record | null; + executionLimits?: { + phaseStatusPollIntervalMs?: number; + phaseStatusTimeoutMs?: number; + maxPhaseCheckFailures?: number; + }; }; type ForgeRunningInfo = { @@ -66,6 +71,15 @@ type ForgeExecutionState = { }; const POLL_INTERVAL_MS = 1200; +const PHASE_STATUS_TIMEOUT_MS = 10 * 60 * 1000; +const MAX_PHASE_CHECK_FAILURES = 3; + +function resolvePositiveNumber(value: number | undefined, fallback: number): number { + if (typeof value !== "number" || !Number.isFinite(value) || value <= 0) { + return fallback; + } + return value; +} function isFinalPhaseStatus(status: ForgeExecutionStatusLike): boolean { const normalized = status.trim().toLowerCase(); @@ -163,6 +177,7 @@ export function useForgeExecution({ sendUserMessage, onSelectThread, collaborationMode = null, + executionLimits, }: ForgeExecutionArgs): ForgeExecutionState { const [isExecuting, setIsExecuting] = useState(false); const [runningInfo, setRunningInfo] = useState(null); @@ -187,6 +202,23 @@ export function useForgeExecution({ } return next; }, [knownTaskIds]); + const phaseStatusPollIntervalMs = resolvePositiveNumber( + executionLimits?.phaseStatusPollIntervalMs, + POLL_INTERVAL_MS, + ); + const phaseStatusTimeoutMs = resolvePositiveNumber( + executionLimits?.phaseStatusTimeoutMs, + PHASE_STATUS_TIMEOUT_MS, + ); + const maxPhaseCheckFailures = Math.max( + 1, + Math.floor( + resolvePositiveNumber( + executionLimits?.maxPhaseCheckFailures, + MAX_PHASE_CHECK_FAILURES, + ), + ), + ); const clearExecutionState = useCallback(() => { setIsExecuting(false); @@ -244,7 +276,15 @@ export function useForgeExecution({ taskId: string, phaseId: string, ): Promise => { + const startedAt = Date.now(); while (isActive()) { + if (Date.now() - startedAt >= phaseStatusTimeoutMs) { + throw new Error( + `Phase ${taskId}/${phaseId} timed out waiting for terminal status after ${Math.ceil( + phaseStatusTimeoutMs / 1000, + )}s.`, + ); + } const phaseStatus = await getPhaseStatus( workspace, normalizedPlanId, @@ -257,7 +297,7 @@ export function useForgeExecution({ if (isFinalPhaseStatus(phaseStatus.status)) { return phaseStatus.status; } - await wait(POLL_INTERVAL_MS); + await wait(phaseStatusPollIntervalMs); } return null; }; @@ -276,6 +316,7 @@ export function useForgeExecution({ let activeThreadId: string | null = null; let activeThreadTaskId: string | null = null; const threadByTaskId = new Map(); + const checkFailuresByTaskPhase = new Map(); const pendingKnownTaskIds = normalizedKnownTaskIds ? new Set(normalizedKnownTaskIds) : null; @@ -406,9 +447,18 @@ export function useForgeExecution({ phaseId, ); if (!checks.ok) { - await wait(POLL_INTERVAL_MS); + const failureKey = `${taskId}:${phaseId}`; + const failures = (checkFailuresByTaskPhase.get(failureKey) ?? 0) + 1; + checkFailuresByTaskPhase.set(failureKey, failures); + if (failures >= maxPhaseCheckFailures) { + throw new Error( + `Phase ${taskId}/${phaseId} reached max check failures (${maxPhaseCheckFailures}).`, + ); + } + await wait(phaseStatusPollIntervalMs); continue; } + checkFailuresByTaskPhase.delete(`${taskId}:${phaseId}`); } } catch (error) { const message = @@ -439,6 +489,9 @@ export function useForgeExecution({ sendUserMessage, startThread, normalizedKnownTaskIds, + phaseStatusPollIntervalMs, + phaseStatusTimeoutMs, + maxPhaseCheckFailures, workspaceId, ], ); diff --git a/src/services/tauri.test.ts b/src/services/tauri.test.ts index 222b309e7b..3dcebd6fb6 100644 --- a/src/services/tauri.test.ts +++ b/src/services/tauri.test.ts @@ -441,6 +441,58 @@ describe("tauri invoke wrappers", () => { }); }); + it("uses a valid default icon id when forge phase metadata iconId is missing", async () => { + const invokeMock = vi.mocked(invoke); + invokeMock.mockImplementation(async (command: string, args?: unknown) => { + if (command === "is_macos_debug_build") { + return false; + } + if (command !== "read_workspace_file") { + return undefined; + } + + const path = + typeof args === "object" && args !== null && "path" in args + ? (args as { path?: unknown }).path + : undefined; + if (path === "plans/alpha/state.json") { + return { + content: JSON.stringify({ + tasks: [ + { + id: "task-1", + phases: [{ id: "implementation", status: "pending" }], + }, + ], + }), + truncated: false, + }; + } + if (path === ".agent/templates/test-first-loop/phases.json") { + return { + content: JSON.stringify({ + schema: "forge-phases-v1", + phases: [{ id: "implementation", title: "Implementation", order: 1 }], + }), + truncated: false, + }; + } + + throw new Error(`unexpected path: ${String(path)}`); + }); + + await expect(forgeLoadPhaseView("ws-1", "alpha", "test-first-loop")).resolves.toEqual({ + phases: [ + { id: "implementation", title: "Implementation", iconId: "file", order: 1 }, + ], + taskPhaseStatusByTaskId: { + "task-1": { + implementation: "pending", + }, + }, + }); + }); + it("keeps failed phase statuses from state for downstream blocking UI", async () => { const invokeMock = vi.mocked(invoke); invokeMock.mockImplementation(async (command: string, args?: unknown) => { diff --git a/src/services/tauri.ts b/src/services/tauri.ts index 4987cfacfe..2b891410ed 100644 --- a/src/services/tauri.ts +++ b/src/services/tauri.ts @@ -1035,7 +1035,7 @@ const EMPTY_FORGE_PHASE_VIEW: ForgePhaseView = { taskPhaseStatusByTaskId: {}, }; -const DEFAULT_FORGE_PHASE_ICON_ID = "check_circle"; +const DEFAULT_FORGE_PHASE_ICON_ID = "file"; function normalizeForgePhaseViewStatus(value: unknown): ForgePhaseViewStatus { if (typeof value !== "string") {