diff options
Diffstat (limited to 'makima/src/server/handlers/contracts.rs')
| -rw-r--r-- | makima/src/server/handlers/contracts.rs | 327 |
1 files changed, 308 insertions, 19 deletions
diff --git a/makima/src/server/handlers/contracts.rs b/makima/src/server/handlers/contracts.rs index 6c237dc..b15667d 100644 --- a/makima/src/server/handlers/contracts.rs +++ b/makima/src/server/handlers/contracts.rs @@ -13,14 +13,82 @@ use uuid::Uuid; use crate::db::models::{ AddLocalRepositoryRequest, AddRemoteRepositoryRequest, ChangePhaseRequest, ContractListResponse, ContractRepository, ContractSummary, ContractWithRelations, - CreateContractRequest, CreateManagedRepositoryRequest, UpdateContractRequest, - UpdateTaskRequest, + CreateContractRequest, CreateManagedRepositoryRequest, PhaseChangeResult, + UpdateContractRequest, UpdateTaskRequest, }; use crate::db::repository::{self, RepositoryError}; +use crate::llm::PhaseDeliverables; use crate::server::auth::Authenticated; use crate::server::messages::ApiError; use crate::server::state::SharedState; +// ============================================================================= +// Deliverable Validation +// ============================================================================= + +/// Error type for deliverable validation failures +#[derive(Debug, Clone)] +pub struct DeliverableValidationError { + /// The error message with details about valid deliverables + pub message: String, +} + +impl DeliverableValidationError { + pub fn new(message: impl Into<String>) -> Self { + Self { + message: message.into(), + } + } +} + +impl std::fmt::Display for DeliverableValidationError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.message) + } +} + +impl std::error::Error for DeliverableValidationError {} + +/// Validates that a deliverable ID is valid for the given phase deliverables. +/// +/// # Arguments +/// * `deliverable_id` - The deliverable ID to validate +/// * `phase_deliverables` - The phase deliverables configuration to validate against +/// +/// # Returns +/// * `Ok(())` if the deliverable is valid +/// * `Err(DeliverableValidationError)` if the deliverable is not valid +pub fn validate_deliverable( + deliverable_id: &str, + phase_deliverables: &PhaseDeliverables, +) -> Result<(), DeliverableValidationError> { + let valid_deliverable = phase_deliverables + .deliverables + .iter() + .any(|d| d.id == deliverable_id); + + if valid_deliverable { + Ok(()) + } else { + let valid_ids: Vec<&str> = phase_deliverables + .deliverables + .iter() + .map(|d| d.id.as_str()) + .collect(); + + Err(DeliverableValidationError::new(format!( + "Invalid deliverable '{}' for {} phase. Valid IDs: [{}]", + deliverable_id, + phase_deliverables.phase, + valid_ids.join(", ") + ))) + } +} + +// ============================================================================= +// Supervisor Repository Update Helper +// ============================================================================= + /// Helper function to update the supervisor task with repository info when a primary repo is added. /// This ensures the supervisor has access to the repository when it starts. async fn update_supervisor_with_repo_if_needed( @@ -1253,8 +1321,10 @@ pub async fn remove_task_from_contract( request_body = ChangePhaseRequest, responses( (status = 200, description = "Phase changed", body = ContractSummary), + (status = 400, description = "Validation failed", body = ApiError), (status = 401, description = "Unauthorized", body = ApiError), (status = 404, description = "Contract not found", body = ApiError), + (status = 409, description = "Version conflict", body = ApiError), (status = 503, description = "Database not configured", body = ApiError), (status = 500, description = "Internal server error", body = ApiError), ), @@ -1299,6 +1369,7 @@ pub async fn change_phase( }; // If phase_guard is enabled and not confirmed, return phase deliverables for review + // This applies to ALL callers (including supervisors) - phase_guard enforcement at API level if contract.phase_guard && !req.confirmed.unwrap_or(false) { // If user provided feedback, return it if let Some(ref feedback) = req.feedback { @@ -1340,6 +1411,20 @@ pub async fn change_phase( Err(_) => Vec::new(), }; + // Get phase deliverables with completion status + let phase_deliverables = crate::llm::get_phase_deliverables_for_type(&contract.phase, &contract.contract_type); + let completed_deliverables = contract.get_completed_deliverables(&contract.phase); + + let deliverables: Vec<serde_json::Value> = phase_deliverables + .deliverables + .iter() + .map(|d| serde_json::json!({ + "id": d.id, + "name": d.name, + "completed": completed_deliverables.contains(&d.id) + })) + .collect(); + let deliverables_summary = format!( "Phase '{}' deliverables: {} files created, {} tasks completed.", contract.phase, @@ -1350,22 +1435,32 @@ pub async fn change_phase( let transition_id = uuid::Uuid::new_v4().to_string(); return Json(serde_json::json!({ - "status": "pending_confirmation", + "status": "requires_confirmation", "transitionId": transition_id, "currentPhase": contract.phase, "nextPhase": req.phase, "deliverablesSummary": deliverables_summary, + "deliverables": deliverables, "phaseFiles": phase_files, "phaseTasks": phase_tasks, "requiresConfirmation": true, - "message": "Phase transition requires confirmation. Set confirmed=true in the request to proceed." + "message": "Phase guard is enabled. User confirmation required." })) .into_response(); } // Phase guard is disabled or user confirmed - proceed with phase change - match repository::change_contract_phase_for_owner(pool, id, auth.owner_id, &req.phase).await { - Ok(Some(updated_contract)) => { + // Use the version-checking function for explicit conflict detection + match repository::change_contract_phase_with_version( + pool, + id, + auth.owner_id, + &req.phase, + req.expected_version, + ) + .await + { + Ok(PhaseChangeResult::Success(updated_contract)) => { // Notify supervisor of phase change if let Some(supervisor_task_id) = updated_contract.supervisor_task_id { if let Ok(Some(supervisor)) = repository::get_task_for_owner(pool, supervisor_task_id, auth.owner_id).await { @@ -1394,7 +1489,7 @@ pub async fn change_phase( Some(&contract.phase), serde_json::json!({ "contractName": &contract.name, - "newPhase": &contract.phase, + "newPhase": &updated_contract.phase, }), ).await; @@ -1422,11 +1517,56 @@ pub async fn change_phase( .into_response(), } } - Ok(None) => ( + Ok(PhaseChangeResult::VersionConflict { expected, actual, current_phase }) => { + tracing::info!( + contract_id = %id, + expected_version = expected, + actual_version = actual, + current_phase = %current_phase, + "Phase change failed due to version conflict" + ); + ( + StatusCode::CONFLICT, + Json(serde_json::json!({ + "code": "VERSION_CONFLICT", + "message": "Phase change failed due to concurrent modification", + "details": { + "expected_version": expected, + "actual_version": actual, + "current_phase": current_phase + } + })), + ) + .into_response() + } + Ok(PhaseChangeResult::ValidationFailed { reason, missing_requirements }) => { + tracing::warn!( + contract_id = %id, + reason = %reason, + "Phase change validation failed" + ); + ( + StatusCode::BAD_REQUEST, + Json(serde_json::json!({ + "code": "VALIDATION_FAILED", + "message": reason, + "details": { + "missing_requirements": missing_requirements + } + })), + ) + .into_response() + } + Ok(PhaseChangeResult::NotFound) => ( StatusCode::NOT_FOUND, Json(ApiError::new("NOT_FOUND", "Contract not found")), ) .into_response(), + Ok(PhaseChangeResult::Unauthorized) => ( + StatusCode::UNAUTHORIZED, + Json(ApiError::new("UNAUTHORIZED", "Not authorized to change this contract's phase")), + ) + .into_response(), Err(e) => { tracing::error!("Failed to change phase for contract {}: {}", id, e); ( @@ -1512,20 +1652,22 @@ pub async fn mark_deliverable_complete( let target_phase = req.phase.unwrap_or_else(|| contract.phase.clone()); // Validate the deliverable ID exists for this phase/contract type - let phase_deliverables = crate::llm::get_phase_deliverables_for_type(&target_phase, &contract.contract_type); - let deliverable = phase_deliverables.deliverables.iter().find(|d| d.id == req.deliverable_id); + // Use custom phase_config if present, otherwise fall back to built-in contract types + let phase_config = contract.get_phase_config(); + let phase_deliverables = crate::llm::get_phase_deliverables_with_config( + &target_phase, + &contract.contract_type, + phase_config.as_ref(), + ); - if deliverable.is_none() { - let valid_ids: Vec<&str> = phase_deliverables.deliverables.iter().map(|d| d.id.as_str()).collect(); + // Validate deliverable exists + if let Err(validation_error) = validate_deliverable(&req.deliverable_id, &phase_deliverables) { return ( StatusCode::BAD_REQUEST, - Json(ApiError::new( - "INVALID_DELIVERABLE", - format!( - "Invalid deliverable_id '{}' for {} phase (contract type: {}). Valid IDs: {:?}", - req.deliverable_id, target_phase, contract.contract_type, valid_ids - ), - )), + Json(serde_json::json!({ + "code": "INVALID_DELIVERABLE", + "message": validation_error.message, + })), ) .into_response(); } @@ -1729,3 +1871,150 @@ async fn cleanup_contract_worktrees( } } } + +// ============================================================================= +// Tests +// ============================================================================= + +#[cfg(test)] +mod tests { + use super::*; + use crate::db::models::{DeliverableDefinition, PhaseConfig, PhaseDefinition}; + use crate::llm::{get_phase_deliverables_for_type, get_phase_deliverables_with_config}; + use std::collections::HashMap; + + #[test] + fn test_validate_deliverable_valid_simple_plan() { + let phase_deliverables = get_phase_deliverables_for_type("plan", "simple"); + let result = validate_deliverable("plan-document", &phase_deliverables); + assert!(result.is_ok()); + } + + #[test] + fn test_validate_deliverable_valid_simple_execute() { + let phase_deliverables = get_phase_deliverables_for_type("execute", "simple"); + let result = validate_deliverable("pull-request", &phase_deliverables); + assert!(result.is_ok()); + } + + #[test] + fn test_validate_deliverable_invalid_id() { + let phase_deliverables = get_phase_deliverables_for_type("plan", "simple"); + let result = validate_deliverable("nonexistent-deliverable", &phase_deliverables); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!(err.message.contains("Invalid deliverable")); + assert!(err.message.contains("nonexistent-deliverable")); + assert!(err.message.contains("plan-document")); + } + + #[test] + fn test_validate_deliverable_specification_phases() { + // Research phase + let phase_deliverables = get_phase_deliverables_for_type("research", "specification"); + assert!(validate_deliverable("research-notes", &phase_deliverables).is_ok()); + assert!(validate_deliverable("invalid", &phase_deliverables).is_err()); + + // Specify phase + let phase_deliverables = get_phase_deliverables_for_type("specify", "specification"); + assert!(validate_deliverable("requirements-document", &phase_deliverables).is_ok()); + assert!(validate_deliverable("plan-document", &phase_deliverables).is_err()); + + // Review phase + let phase_deliverables = get_phase_deliverables_for_type("review", "specification"); + assert!(validate_deliverable("release-notes", &phase_deliverables).is_ok()); + } + + #[test] + fn test_validate_deliverable_execute_type_no_deliverables() { + // Execute-only contracts have no deliverables + let phase_deliverables = get_phase_deliverables_for_type("execute", "execute"); + // Any deliverable should fail since there are none + let result = validate_deliverable("pull-request", &phase_deliverables); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!(err.message.contains("Valid IDs: []")); + } + + #[test] + fn test_validate_deliverable_with_custom_phase_config() { + // Create a custom phase config + let mut deliverables = HashMap::new(); + deliverables.insert( + "design".to_string(), + vec![ + DeliverableDefinition { + id: "architecture-doc".to_string(), + name: "Architecture Document".to_string(), + priority: "required".to_string(), + }, + DeliverableDefinition { + id: "api-spec".to_string(), + name: "API Specification".to_string(), + priority: "recommended".to_string(), + }, + ], + ); + + let phase_config = PhaseConfig { + phases: vec![ + PhaseDefinition { + id: "design".to_string(), + name: "Design".to_string(), + order: 0, + }, + PhaseDefinition { + id: "build".to_string(), + name: "Build".to_string(), + order: 1, + }, + ], + default_phase: "design".to_string(), + deliverables, + }; + + // Validate against custom config + let phase_deliverables = + get_phase_deliverables_with_config("design", "custom", Some(&phase_config)); + + // Valid custom deliverables + assert!(validate_deliverable("architecture-doc", &phase_deliverables).is_ok()); + assert!(validate_deliverable("api-spec", &phase_deliverables).is_ok()); + + // Invalid deliverable for custom config + let result = validate_deliverable("plan-document", &phase_deliverables); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!(err.message.contains("Invalid deliverable")); + assert!(err.message.contains("plan-document")); + assert!(err.message.contains("architecture-doc")); + assert!(err.message.contains("api-spec")); + } + + #[test] + fn test_validate_deliverable_error_message_format() { + let phase_deliverables = get_phase_deliverables_for_type("plan", "simple"); + let result = validate_deliverable("xyz", &phase_deliverables); + let err = result.unwrap_err(); + + // Check error message format matches the specification + assert!(err.message.contains("Invalid deliverable 'xyz'")); + assert!(err.message.contains("plan phase")); + assert!(err.message.contains("Valid IDs:")); + assert!(err.message.contains("plan-document")); + } + + #[test] + fn test_deliverable_validation_error_display() { + let err = DeliverableValidationError::new("Test error message"); + assert_eq!(format!("{}", err), "Test error message"); + } + + #[test] + fn test_validate_deliverable_unknown_phase() { + // Unknown phase should return empty deliverables + let phase_deliverables = get_phase_deliverables_for_type("unknown", "simple"); + let result = validate_deliverable("any-id", &phase_deliverables); + assert!(result.is_err()); + } +} |