diff options
Diffstat (limited to 'makima')
| -rw-r--r-- | makima/src/bin/makima.rs | 8 | ||||
| -rw-r--r-- | makima/src/daemon/api/supervisor.rs | 21 | ||||
| -rw-r--r-- | makima/src/daemon/cli/supervisor.rs | 5 | ||||
| -rw-r--r-- | makima/src/db/models.rs | 31 | ||||
| -rw-r--r-- | makima/src/db/repository.rs | 107 | ||||
| -rw-r--r-- | makima/src/llm/mod.rs | 6 | ||||
| -rw-r--r-- | makima/src/server/handlers/contract_chat.rs | 21 | ||||
| -rw-r--r-- | makima/src/server/handlers/contracts.rs | 327 |
8 files changed, 497 insertions, 29 deletions
diff --git a/makima/src/bin/makima.rs b/makima/src/bin/makima.rs index e5b1a5e..ac577b8 100644 --- a/makima/src/bin/makima.rs +++ b/makima/src/bin/makima.rs @@ -488,9 +488,13 @@ async fn run_supervisor( } SupervisorCommand::AdvancePhase(args) => { let client = ApiClient::new(args.common.api_url, args.common.api_key)?; - eprintln!("Advancing contract to phase: {}...", args.phase); + if args.confirmed { + eprintln!("Advancing contract to phase: {} (confirmed)...", args.phase); + } else { + eprintln!("Requesting phase advance to: {} (use --confirmed to proceed)...", args.phase); + } let result = client - .supervisor_advance_phase(args.common.contract_id, &args.phase) + .supervisor_advance_phase(args.common.contract_id, &args.phase, args.confirmed) .await?; println!("{}", serde_json::to_string(&result.0)?); } diff --git a/makima/src/daemon/api/supervisor.rs b/makima/src/daemon/api/supervisor.rs index c841b21..c2da1db 100644 --- a/makima/src/daemon/api/supervisor.rs +++ b/makima/src/daemon/api/supervisor.rs @@ -233,22 +233,43 @@ impl ApiClient { } /// Advance contract to a new phase. + /// + /// When `confirmed` is false and phase_guard is enabled, returns a response with + /// `status: "pending_confirmation"` containing deliverables for user review. + /// When `confirmed` is true, proceeds with the phase transition. pub async fn supervisor_advance_phase( &self, contract_id: Uuid, phase: &str, + confirmed: bool, ) -> Result<JsonValue, ApiError> { #[derive(Serialize)] struct AdvancePhaseRequest { phase: String, + confirmed: bool, } let req = AdvancePhaseRequest { phase: phase.to_string(), + confirmed, }; self.post(&format!("/api/v1/contracts/{}/phase", contract_id), &req) .await } + /// Request phase advancement without confirmation (for phase_guard check). + /// + /// This method calls `supervisor_advance_phase` with `confirmed=false`. + /// If phase_guard is enabled, the response will have `status: "pending_confirmation"` + /// and the caller should prompt the user for confirmation before calling again with + /// `confirmed=true`. + pub async fn supervisor_request_phase_advance( + &self, + contract_id: Uuid, + phase: &str, + ) -> Result<JsonValue, ApiError> { + self.supervisor_advance_phase(contract_id, phase, false).await + } + /// Get individual task details. pub async fn supervisor_get_task(&self, task_id: Uuid) -> Result<JsonValue, ApiError> { self.get(&format!("/api/v1/mesh/tasks/{}", task_id)).await diff --git a/makima/src/daemon/cli/supervisor.rs b/makima/src/daemon/cli/supervisor.rs index 9ad7aef..cb84ffa 100644 --- a/makima/src/daemon/cli/supervisor.rs +++ b/makima/src/daemon/cli/supervisor.rs @@ -218,6 +218,11 @@ pub struct AdvancePhaseArgs { /// The phase to advance to (specify, plan, execute, review) #[arg(index = 1)] pub phase: String, + + /// Confirm the phase transition (required when phase_guard is enabled). + /// Without this flag, the command will return deliverables for review. + #[arg(long, short = 'y')] + pub confirmed: bool, } /// Arguments for mark-deliverable command. diff --git a/makima/src/db/models.rs b/makima/src/db/models.rs index a6b5b05..636d81a 100644 --- a/makima/src/db/models.rs +++ b/makima/src/db/models.rs @@ -1808,6 +1808,37 @@ pub struct ChangePhaseRequest { /// User feedback for changes (used when not confirming) #[serde(skip_serializing_if = "Option::is_none")] pub feedback: Option<String>, + /// Expected version for optimistic locking. If provided, the phase change + /// will only succeed if the current contract version matches. + #[serde(skip_serializing_if = "Option::is_none")] + pub expected_version: Option<i32>, +} + +/// Result of a phase change operation, supporting explicit conflict detection. +#[derive(Debug, Clone)] +pub enum PhaseChangeResult { + /// Phase change succeeded, returning the updated contract + Success(Contract), + /// Version conflict: the contract was modified concurrently + VersionConflict { + /// The version the client expected + expected: i32, + /// The actual current version in the database + actual: i32, + /// The current phase of the contract + current_phase: String, + }, + /// Validation failed (e.g., invalid phase transition) + ValidationFailed { + /// Human-readable reason for the failure + reason: String, + /// List of missing requirements for the phase transition + missing_requirements: Vec<String>, + }, + /// The caller is not authorized to change this contract's phase + Unauthorized, + /// The contract was not found + NotFound, } /// Response for phase transition when phase_guard is enabled diff --git a/makima/src/db/repository.rs b/makima/src/db/repository.rs index 8c7ea23..b7c5af1 100644 --- a/makima/src/db/repository.rs +++ b/makima/src/db/repository.rs @@ -11,9 +11,10 @@ use super::models::{ ContractTypeTemplateRecord, ConversationMessage, ConversationSnapshot, CreateContractRequest, CreateFileRequest, CreateTaskRequest, CreateTemplateRequest, Daemon, DaemonTaskAssignment, DaemonWithCapacity, DeliverableDefinition, File, FileSummary, FileVersion, HistoryEvent, - HistoryQueryFilters, MeshChatConversation, MeshChatMessageRecord, PhaseConfig, PhaseDefinition, - RedTeamNotification, SupervisorState, Task, TaskCheckpoint, TaskEvent, TaskSummary, - UpdateContractRequest, UpdateFileRequest, UpdateTaskRequest, UpdateTemplateRequest, + HistoryQueryFilters, MeshChatConversation, MeshChatMessageRecord, PhaseChangeResult, + PhaseConfig, PhaseDefinition, RedTeamNotification, SupervisorState, Task, TaskCheckpoint, + TaskEvent, TaskSummary, UpdateContractRequest, UpdateFileRequest, UpdateTaskRequest, + UpdateTemplateRequest, }; /// Repository error types. @@ -2676,6 +2677,9 @@ pub async fn delete_contract_for_owner( } /// Change contract phase and record event. +/// +/// This is the simple version without version checking. Use `change_contract_phase_with_version` +/// for explicit version conflict detection. pub async fn change_contract_phase_for_owner( pool: &PgPool, id: Uuid, @@ -2723,6 +2727,103 @@ pub async fn change_contract_phase_for_owner( Ok(contract) } +/// Change contract phase with explicit version checking for conflict detection. +/// +/// Uses `SELECT ... FOR UPDATE` to lock the row and prevent race conditions. +/// Returns `PhaseChangeResult::VersionConflict` if the expected version doesn't match. +pub async fn change_contract_phase_with_version( + pool: &PgPool, + id: Uuid, + owner_id: Uuid, + new_phase: &str, + expected_version: Option<i32>, +) -> Result<PhaseChangeResult, sqlx::Error> { + // Start a transaction to ensure atomicity with row locking + let mut tx = pool.begin().await?; + + // Lock the row with SELECT FOR UPDATE and get current state + let existing: Option<Contract> = sqlx::query_as::<_, Contract>( + r#" + SELECT * + FROM contracts + WHERE id = $1 AND owner_id = $2 + FOR UPDATE + "#, + ) + .bind(id) + .bind(owner_id) + .fetch_optional(&mut *tx) + .await?; + + let Some(existing) = existing else { + tx.rollback().await?; + return Ok(PhaseChangeResult::NotFound); + }; + + // Check version if provided (optimistic locking) + if let Some(expected) = expected_version { + if existing.version != expected { + tx.rollback().await?; + return Ok(PhaseChangeResult::VersionConflict { + expected, + actual: existing.version, + current_phase: existing.phase, + }); + } + } + + // Validate the phase transition is allowed + let valid_phases = existing.valid_phase_ids(); + if !valid_phases.contains(&new_phase.to_string()) { + tx.rollback().await?; + return Ok(PhaseChangeResult::ValidationFailed { + reason: format!( + "Invalid phase '{}' for contract type '{}'", + new_phase, existing.contract_type + ), + missing_requirements: vec![format!( + "Phase must be one of: {}", + valid_phases.join(", ") + )], + }); + } + + let previous_phase = existing.phase.clone(); + + // Update phase with version increment + let contract = sqlx::query_as::<_, Contract>( + r#" + UPDATE contracts + SET phase = $3, version = version + 1, updated_at = NOW() + WHERE id = $1 AND owner_id = $2 + RETURNING * + "#, + ) + .bind(id) + .bind(owner_id) + .bind(new_phase) + .fetch_one(&mut *tx) + .await?; + + // Record event + sqlx::query( + r#" + INSERT INTO contract_events (contract_id, event_type, previous_phase, new_phase) + VALUES ($1, 'phase_change', $2, $3) + "#, + ) + .bind(id) + .bind(&previous_phase) + .bind(new_phase) + .execute(&mut *tx) + .await?; + + // Commit the transaction + tx.commit().await?; + + Ok(PhaseChangeResult::Success(contract)) +} + // ============================================================================= // Contract Repository Functions // ============================================================================= diff --git a/makima/src/llm/mod.rs b/makima/src/llm/mod.rs index 212876a..a3a3daf 100644 --- a/makima/src/llm/mod.rs +++ b/makima/src/llm/mod.rs @@ -21,9 +21,9 @@ pub use mesh_tools::{parse_mesh_tool_call, MeshToolExecutionResult, MeshToolRequ pub use phase_guidance::{ check_deliverables_met, format_checklist_markdown, generate_deliverable_prompt_guidance, get_next_phase_for_contract, get_phase_checklist_for_type, get_phase_deliverables, - get_phase_deliverables_for_type, should_auto_progress, AutoProgressAction, AutoProgressDecision, - Deliverable, DeliverableCheckResult, DeliverableItem, DeliverablePriority, DeliverableStatus, - PhaseChecklist, PhaseDeliverables, TaskInfo, TaskStats, + get_phase_deliverables_for_type, get_phase_deliverables_with_config, should_auto_progress, + AutoProgressAction, AutoProgressDecision, Deliverable, DeliverableCheckResult, DeliverableItem, + DeliverablePriority, DeliverableStatus, PhaseChecklist, PhaseDeliverables, TaskInfo, TaskStats, }; pub use task_output::{ analyze_task_output, format_parsed_tasks, parse_tasks_from_breakdown, ParsedTask, diff --git a/makima/src/server/handlers/contract_chat.rs b/makima/src/server/handlers/contract_chat.rs index b5255f5..b025485 100644 --- a/makima/src/server/handlers/contract_chat.rs +++ b/makima/src/server/handlers/contract_chat.rs @@ -1812,7 +1812,8 @@ async fn handle_contract_request( }; } - // If not confirmed, return pending confirmation with phase deliverables + // If not confirmed, return requires_confirmation with phase deliverables + // This applies to ALL callers (including supervisors) - phase_guard enforcement at API level if !confirmed { // Get files created in this phase let phase_files = match repository::list_files_in_contract(pool, contract_id, owner_id).await { @@ -1842,6 +1843,20 @@ async fn handle_contract_request( Err(_) => Vec::new(), }; + // Get phase deliverables with completion status + let phase_deliverables = crate::llm::get_phase_deliverables_for_type(current_phase, &contract.contract_type); + let completed_deliverables = contract.get_completed_deliverables(current_phase); + + let deliverables: Vec<serde_json::Value> = phase_deliverables + .deliverables + .iter() + .map(|d| json!({ + "id": d.id, + "name": d.name, + "completed": completed_deliverables.contains(&d.id) + })) + .collect(); + // Build deliverables summary let deliverables_summary = format!( "Phase '{}' deliverables: {} files created, {} tasks completed.", @@ -1859,14 +1874,16 @@ async fn handle_contract_request( new_phase ), data: Some(json!({ - "status": "pending_confirmation", + "status": "requires_confirmation", "transitionId": transition_id, "currentPhase": current_phase, "nextPhase": new_phase, "deliverablesSummary": deliverables_summary, + "deliverables": deliverables, "phaseFiles": phase_files, "phaseTasks": phase_tasks, "requiresConfirmation": true, + "message": "Phase guard is enabled. User confirmation required.", "instructions": "To proceed: call advance_phase with confirmed=true. To request changes: call advance_phase with feedback='your feedback here'" })), }; 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()); + } +} |