diff --git a/src/main.rs b/src/main.rs index f606e4e..6e735a0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1680,6 +1680,13 @@ mod tests { .iter() .position(|record| record.event_type == EventType::ReviewCompleted) .expect("review completion must be recorded durably"); + let commitment_complete_index = records + .iter() + .position(|record| { + record.event_type == EventType::CommitmentTransition + && record.payload_json["action"] == "complete" + }) + .expect("review completion must complete the commitment"); let planning_transition_index = records .iter() .position(|record| { @@ -1688,7 +1695,16 @@ mod tests { && record.payload_json["action"] == "return_to_planning_after_review" }) .expect("review must return to planning after completion"); - assert_eq!(planning_transition_index, review_completed_index + 1); + assert_eq!(commitment_complete_index, review_completed_index + 1); + assert_eq!(planning_transition_index, commitment_complete_index + 1); + assert_eq!( + records[review_completed_index].commitment_id, + records[commitment_complete_index].commitment_id + ); + assert_eq!( + records[review_completed_index].payload_json["outcome"], + "completed" + ); let _ = fs::remove_file(path); } @@ -1752,6 +1768,13 @@ mod tests { .iter() .position(|record| record.event_type == EventType::ReviewCompleted) .expect("review completion must be recorded"); + let commitment_complete_index = records + .iter() + .position(|record| { + record.event_type == EventType::CommitmentTransition + && record.payload_json["action"] == "complete" + }) + .expect("review completion must complete the commitment"); let planning_transition_index = records .iter() .position(|record| { @@ -1760,7 +1783,16 @@ mod tests { && record.payload_json["action"] == "return_to_planning_after_review" }) .expect("review completion must return to planning"); - assert_eq!(planning_transition_index, review_completed_index + 1); + assert_eq!(commitment_complete_index, review_completed_index + 1); + assert_eq!(planning_transition_index, commitment_complete_index + 1); + assert_eq!( + records[review_completed_index].commitment_id, + records[commitment_complete_index].commitment_id + ); + assert_eq!( + records[review_completed_index].payload_json["outcome"], + "completed" + ); let _ = fs::remove_file(path); } diff --git a/src/session.rs b/src/session.rs index a20452b..7550eda 100644 --- a/src/session.rs +++ b/src/session.rs @@ -88,6 +88,8 @@ struct EvidenceObservedPayload { #[serde(deny_unknown_fields)] struct ReviewCompletedPayload { schema_version: u16, + commitment_id: String, + outcome: CommitmentState, rating: u8, rating_f64: f64, reviewed_window_count: u32, @@ -270,13 +272,15 @@ impl SessionController { let commitment_id = self .active_commitment .as_ref() - .map(|commitment| commitment.id.clone()); + .ok_or_else(|| anyhow!("active commitment is required to complete for review"))? + .id + .clone(); self.event_log .append( EventType::RuntimeTransition, next_runtime_state, - commitment_id, + Some(commitment_id), serde_json::to_value(RuntimeTransitionPayload { schema_version: POLICY_SCHEMA_VERSION, action: "complete_for_review".to_string(), @@ -288,7 +292,6 @@ impl SessionController { .context("log runtime transition into review")?; self.runtime_state = next_runtime_state; - self.active_commitment = None; self.active_policy = None; self.review_completed = false; Ok(()) @@ -304,8 +307,26 @@ impl SessionController { let next_runtime_state = transition_runtime(previous_runtime_state, RuntimeAction::ContinuePlanning) .context("continue planning after review runtime transition")?; + let active_commitment = self + .active_commitment + .as_ref() + .ok_or_else(|| anyhow!("active reviewed commitment is required to complete review"))?; + let commitment_id = active_commitment.id.clone(); + let previous_commitment_state = active_commitment.state.clone(); + let next_commitment_state = + if self.review_completed && previous_commitment_state == CommitmentState::Completed { + CommitmentState::Completed + } else { + transition_commitment( + previous_commitment_state.clone(), + CommitmentAction::Complete, + ) + .context("complete reviewed commitment")? + }; let completion_payload = ReviewCompletedPayload { schema_version: POLICY_SCHEMA_VERSION, + commitment_id: commitment_id.clone(), + outcome: CommitmentState::Completed, rating, rating_f64, reviewed_window_count, @@ -320,34 +341,74 @@ impl SessionController { }; if self.review_completed { - self.event_log - .append( - EventType::RuntimeTransition, - next_runtime_state, - None, - serde_json::to_value(transition_payload) - .context("serialize runtime transition payload")?, - ) - .context("log runtime transition from review to planning")?; + if previous_commitment_state == CommitmentState::Completed { + self.event_log + .append( + EventType::RuntimeTransition, + next_runtime_state, + Some(commitment_id.clone()), + serde_json::to_value(transition_payload) + .context("serialize runtime transition payload")?, + ) + .context("log runtime transition from review to planning")?; + } else { + self.event_log + .append_batch([ + PendingEventRecord { + event_type: EventType::CommitmentTransition, + runtime_state: RuntimeState::Review, + commitment_id: Some(commitment_id.clone()), + payload_json: serde_json::to_value(CommitmentTransitionPayload { + schema_version: POLICY_SCHEMA_VERSION, + commitment_id: commitment_id.clone(), + action: "complete".to_string(), + from: previous_commitment_state.clone(), + to: next_commitment_state.clone(), + }) + .context("serialize commitment transition payload")?, + }, + PendingEventRecord { + event_type: EventType::RuntimeTransition, + runtime_state: next_runtime_state, + commitment_id: Some(commitment_id.clone()), + payload_json: serde_json::to_value(transition_payload) + .context("serialize runtime transition payload")?, + }, + ]) + .context("log commitment completion and runtime transition to planning")?; + } } else { self.event_log .append_batch([ PendingEventRecord { event_type: EventType::ReviewCompleted, runtime_state: RuntimeState::Review, - commitment_id: None, + commitment_id: Some(commitment_id.clone()), payload_json: serde_json::to_value(completion_payload) .context("serialize review completed payload")?, }, + PendingEventRecord { + event_type: EventType::CommitmentTransition, + runtime_state: RuntimeState::Review, + commitment_id: Some(commitment_id.clone()), + payload_json: serde_json::to_value(CommitmentTransitionPayload { + schema_version: POLICY_SCHEMA_VERSION, + commitment_id: commitment_id.clone(), + action: "complete".to_string(), + from: previous_commitment_state.clone(), + to: next_commitment_state.clone(), + }) + .context("serialize commitment transition payload")?, + }, PendingEventRecord { event_type: EventType::RuntimeTransition, runtime_state: next_runtime_state, - commitment_id: None, + commitment_id: Some(commitment_id.clone()), payload_json: serde_json::to_value(transition_payload) .context("serialize runtime transition payload")?, }, ]) - .context("log review completion and runtime transition to planning")?; + .context("log review completion, commitment completion, and runtime transition")?; } self.runtime_state = next_runtime_state; @@ -690,11 +751,38 @@ fn hydrate_session( )); } }; - if matches!(action, RuntimeAction::ContinuePlanning) && !review_completed { - return Err(anyhow!( - "review completion is required before returning to planning at sequence {}", - record.sequence - )); + match action { + RuntimeAction::CompleteForReview => { + let commitment = active_commitment.as_ref().ok_or_else(|| { + anyhow!( + "active commitment is required to enter review at sequence {}", + record.sequence + ) + })?; + ensure_record_commitment(&record, &commitment.id)?; + } + RuntimeAction::ContinuePlanning => { + let commitment = active_commitment.as_ref().ok_or_else(|| { + anyhow!( + "active reviewed commitment is required before returning to planning at sequence {}", + record.sequence + ) + })?; + ensure_record_commitment(&record, &commitment.id)?; + if !review_completed { + return Err(anyhow!( + "review completion is required before returning to planning at sequence {}", + record.sequence + )); + } + if commitment.state != CommitmentState::Completed { + return Err(anyhow!( + "reviewed commitment must be terminal before returning to planning at sequence {}", + record.sequence + )); + } + } + _ => {} } let next = transition_runtime(runtime_state, action).with_context(|| { @@ -709,7 +797,6 @@ fn hydrate_session( runtime_state = next; match action { RuntimeAction::CompleteForReview => { - active_commitment = None; active_policy = None; review_completed = false; } @@ -753,6 +840,21 @@ fn hydrate_session( } let action = match payload.action.as_str() { "return_from_pause" => CommitmentAction::ReturnFromPause, + "complete" => { + if runtime_state != RuntimeState::Review { + return Err(anyhow!( + "commitment completion requires review runtime at sequence {}", + record.sequence + )); + } + if !review_completed { + return Err(anyhow!( + "review completion is required before commitment completion at sequence {}", + record.sequence + )); + } + CommitmentAction::Complete + } _ => { return Err(anyhow!( "unsupported commitment transition action '{}' at sequence {}", @@ -1060,9 +1162,22 @@ fn hydrate_session( record.sequence )); } - if record.commitment_id.is_some() { + ensure_record_commitment(&record, &payload.commitment_id)?; + let commitment = active_commitment.as_ref().ok_or_else(|| { + anyhow!( + "active reviewed commitment is required for review completion at sequence {}", + record.sequence + ) + })?; + if commitment.id != payload.commitment_id { return Err(anyhow!( - "review completion must not reference a commitment at sequence {}", + "review completion commitment mismatch at sequence {}", + record.sequence + )); + } + if commitment.state != CommitmentState::Active { + return Err(anyhow!( + "review completion requires active commitment at sequence {}", record.sequence )); } @@ -1082,7 +1197,15 @@ fn hydrate_session( "session replay ended with pending transition policy" )); } - if active_commitment.is_some() && active_policy.is_none() { + if runtime_state == RuntimeState::Review && active_commitment.is_none() { + return Err(anyhow!( + "session replay ended in review without active reviewed commitment" + )); + } + if active_commitment.is_some() + && active_policy.is_none() + && runtime_state != RuntimeState::Review + { return Err(anyhow!( "session replay ended with active commitment but no active policy" )); @@ -1254,6 +1377,12 @@ fn validate_evidence_observed_payload(payload: &EvidenceObservedPayload) -> Resu } fn validate_review_completed_payload(payload: &ReviewCompletedPayload) -> Result<()> { + if payload.commitment_id.trim().is_empty() { + return Err(anyhow!("review commitment id is required")); + } + if payload.outcome != CommitmentState::Completed { + return Err(anyhow!("review outcome must be completed")); + } if payload.rating > 3 { return Err(anyhow!("review rating must be between 0 and 3")); } @@ -1273,8 +1402,8 @@ fn validate_review_completed_payload(payload: &ReviewCompletedPayload) -> Result #[cfg(test)] mod tests { use super::{ - CommitmentCreatedPayload, PolicyAppliedPayload, RuntimeTransitionPayload, - TransitionStartedPayload, + CommitmentCreatedPayload, CommitmentTransitionPayload, PolicyAppliedPayload, + RuntimeTransitionPayload, TransitionStartedPayload, }; use crate::domain::{ unix_secs_now, AllowedContext, CommitmentSource, CommitmentState, EnforcementLevel, @@ -1458,6 +1587,28 @@ mod tests { .unwrap(); } + fn review_completed_payload(commitment_id: &str) -> serde_json::Value { + json!({ + "schema_version": POLICY_SCHEMA_VERSION, + "commitment_id": commitment_id, + "outcome": "completed", + "rating": 2, + "rating_f64": 2.0, + "reviewed_window_count": 1, + }) + } + + fn commitment_complete_payload(commitment_id: &str) -> serde_json::Value { + serde_json::to_value(CommitmentTransitionPayload { + schema_version: POLICY_SCHEMA_VERSION, + commitment_id: commitment_id.to_string(), + action: "complete".to_string(), + from: CommitmentState::Active, + to: CommitmentState::Completed, + }) + .unwrap() + } + fn error_chain_contains(err: &anyhow::Error, expected: &str) -> bool { err.chain() .any(|cause| cause.to_string().contains(expected)) @@ -2012,9 +2163,15 @@ mod tests { Duration::from_secs(1500), ) .unwrap(); + let commitment_id = session.active_commitment().unwrap().id.clone(); session.complete_for_review().unwrap(); assert_eq!(session.runtime_state(), RuntimeState::Review); + assert_eq!( + session.active_commitment().unwrap().id.as_str(), + commitment_id.as_str() + ); + assert!(session.active_policy().is_none()); session .complete_review_and_return_to_planning(2, 2.0, 1) @@ -2056,6 +2213,38 @@ mod tests { assert_eq!(payload.from, RuntimeState::Active); assert_eq!(payload.to, RuntimeState::Review); + let review_completed_index = records + .iter() + .position(|record| record.event_type == EventType::ReviewCompleted) + .unwrap(); + let commitment_complete_index = records + .iter() + .position(|record| { + record.event_type == EventType::CommitmentTransition + && record.payload_json["action"] == "complete" + }) + .unwrap(); + let planning_transition_index = records + .iter() + .position(|record| { + record.event_type == EventType::RuntimeTransition + && record.payload_json["action"] == "return_to_planning_after_review" + }) + .unwrap(); + assert_eq!(commitment_complete_index, review_completed_index + 1); + assert_eq!(planning_transition_index, commitment_complete_index + 1); + + let review_completed = &records[review_completed_index]; + assert_eq!( + review_completed.commitment_id.as_deref(), + Some(commitment_id.as_str()) + ); + assert_eq!( + review_completed.payload_json["commitment_id"], + commitment_id + ); + assert_eq!(review_completed.payload_json["outcome"], "completed"); + fs::remove_file(path).unwrap(); } @@ -2103,7 +2292,7 @@ mod tests { PendingEventRecord { event_type: EventType::RuntimeTransition, runtime_state: RuntimeState::Planning, - commitment_id: None, + commitment_id: Some(commitment_id.to_string()), payload_json: json!({ "schema_version": POLICY_SCHEMA_VERSION, "action": "return_to_planning_after_review", @@ -2126,18 +2315,14 @@ mod tests { fn replay_tolerates_review_completed_without_changing_runtime() { let path = temp_log_path("review-completed-no-transition"); append_review_session_without_completion(&path); + let commitment_id = "commitment-test"; { let mut log = EventLog::open(&path).unwrap(); log.append( EventType::ReviewCompleted, RuntimeState::Review, - None, - json!({ - "schema_version": POLICY_SCHEMA_VERSION, - "rating": 2, - "rating_f64": 2.25, - "reviewed_window_count": 3, - }), + Some(commitment_id.to_string()), + review_completed_payload(commitment_id), ) .unwrap(); } @@ -2145,6 +2330,10 @@ mod tests { let reopened = SessionController::new(&path).unwrap(); assert_eq!(reopened.runtime_state(), RuntimeState::Review); + assert_eq!( + reopened.active_commitment().unwrap().state, + CommitmentState::Active + ); fs::remove_file(path).unwrap(); } @@ -2168,9 +2357,11 @@ mod tests { PendingEventRecord { event_type: EventType::ReviewCompleted, runtime_state: RuntimeState::Planning, - commitment_id: None, + commitment_id: Some("commitment-test".to_string()), payload_json: json!({ "schema_version": POLICY_SCHEMA_VERSION, + "commitment_id": "commitment-test", + "outcome": "completed", "rating": 2, "rating_f64": 2.0, "reviewed_window_count": 1, @@ -2191,14 +2382,17 @@ mod tests { fn replay_rejects_malformed_review_completed() { let path = temp_log_path("review-completed-malformed"); append_review_session_without_completion(&path); + let commitment_id = "commitment-test"; { let mut log = EventLog::open(&path).unwrap(); log.append( EventType::ReviewCompleted, RuntimeState::Review, - None, + Some(commitment_id.to_string()), json!({ "schema_version": POLICY_SCHEMA_VERSION, + "commitment_id": commitment_id, + "outcome": "completed", "rating": 4, "rating_f64": 4.0, "reviewed_window_count": 1, @@ -2213,6 +2407,210 @@ mod tests { fs::remove_file(path).unwrap(); } + #[test] + fn replay_rejects_review_completed_without_commitment_id() { + let path = temp_log_path("review-completed-missing-commitment"); + append_review_session_without_completion(&path); + { + let mut log = EventLog::open(&path).unwrap(); + log.append( + EventType::ReviewCompleted, + RuntimeState::Review, + None, + json!({ + "schema_version": POLICY_SCHEMA_VERSION, + "outcome": "completed", + "rating": 2, + "rating_f64": 2.0, + "reviewed_window_count": 1, + }), + ) + .unwrap(); + } + + let err = SessionController::new(&path) + .expect_err("review completion without commitment id must fail"); + + assert!(error_chain_contains(&err, "commitment")); + fs::remove_file(path).unwrap(); + } + + #[test] + fn replay_rejects_review_completed_for_wrong_commitment() { + let path = temp_log_path("review-completed-wrong-commitment"); + append_review_session_without_completion(&path); + { + let mut log = EventLog::open(&path).unwrap(); + log.append( + EventType::ReviewCompleted, + RuntimeState::Review, + Some("commitment-other".to_string()), + review_completed_payload("commitment-other"), + ) + .unwrap(); + } + + let err = SessionController::new(&path) + .expect_err("wrong review completion commitment must fail"); + + assert!(error_chain_contains(&err, "commitment mismatch")); + fs::remove_file(path).unwrap(); + } + + #[test] + fn replay_rejects_review_completed_with_mismatched_envelope_commitment() { + let path = temp_log_path("review-completed-envelope-mismatch"); + append_review_session_without_completion(&path); + { + let mut log = EventLog::open(&path).unwrap(); + log.append( + EventType::ReviewCompleted, + RuntimeState::Review, + Some("commitment-other".to_string()), + review_completed_payload("commitment-test"), + ) + .unwrap(); + } + + let err = SessionController::new(&path) + .expect_err("review completion envelope mismatch must fail"); + + assert!(error_chain_contains(&err, "commitment id mismatch")); + fs::remove_file(path).unwrap(); + } + + #[test] + fn replay_rejects_duplicate_review_completed() { + let path = temp_log_path("review-completed-duplicate"); + let commitment_id = "commitment-test"; + append_review_session_without_completion(&path); + { + let mut log = EventLog::open(&path).unwrap(); + log.append_batch([ + PendingEventRecord { + event_type: EventType::ReviewCompleted, + runtime_state: RuntimeState::Review, + commitment_id: Some(commitment_id.to_string()), + payload_json: review_completed_payload(commitment_id), + }, + PendingEventRecord { + event_type: EventType::ReviewCompleted, + runtime_state: RuntimeState::Review, + commitment_id: Some(commitment_id.to_string()), + payload_json: review_completed_payload(commitment_id), + }, + ]) + .unwrap(); + } + + let err = SessionController::new(&path).expect_err("duplicate review completion must fail"); + + assert!(error_chain_contains(&err, "already recorded")); + fs::remove_file(path).unwrap(); + } + + #[test] + fn replay_rejects_review_to_planning_before_commitment_terminal_completion() { + let path = temp_log_path("review-planning-before-terminal"); + let commitment_id = "commitment-test"; + append_review_session_without_completion(&path); + { + let mut log = EventLog::open(&path).unwrap(); + log.append_batch([ + PendingEventRecord { + event_type: EventType::ReviewCompleted, + runtime_state: RuntimeState::Review, + commitment_id: Some(commitment_id.to_string()), + payload_json: review_completed_payload(commitment_id), + }, + PendingEventRecord { + event_type: EventType::RuntimeTransition, + runtime_state: RuntimeState::Planning, + commitment_id: Some(commitment_id.to_string()), + payload_json: json!({ + "schema_version": POLICY_SCHEMA_VERSION, + "action": "return_to_planning_after_review", + "from": "review", + "to": "planning", + }), + }, + ]) + .unwrap(); + } + + let err = SessionController::new(&path) + .expect_err("review to planning before terminal commitment must fail"); + + assert!(error_chain_contains(&err, "terminal")); + fs::remove_file(path).unwrap(); + } + + #[test] + fn replay_rejects_commitment_completion_before_review_completed() { + let path = temp_log_path("review-terminal-before-completion"); + let commitment_id = "commitment-test"; + append_review_session_without_completion(&path); + { + let mut log = EventLog::open(&path).unwrap(); + log.append( + EventType::CommitmentTransition, + RuntimeState::Review, + Some(commitment_id.to_string()), + commitment_complete_payload(commitment_id), + ) + .unwrap(); + } + + let err = SessionController::new(&path) + .expect_err("commitment completion before review completion must fail"); + + assert!(error_chain_contains(&err, "review completion")); + fs::remove_file(path).unwrap(); + } + + #[test] + fn replay_completes_review_commitment_before_returning_to_planning() { + let path = temp_log_path("review-complete-terminal"); + let commitment_id = "commitment-test"; + append_review_session_without_completion(&path); + { + let mut log = EventLog::open(&path).unwrap(); + log.append_batch([ + PendingEventRecord { + event_type: EventType::ReviewCompleted, + runtime_state: RuntimeState::Review, + commitment_id: Some(commitment_id.to_string()), + payload_json: review_completed_payload(commitment_id), + }, + PendingEventRecord { + event_type: EventType::CommitmentTransition, + runtime_state: RuntimeState::Review, + commitment_id: Some(commitment_id.to_string()), + payload_json: commitment_complete_payload(commitment_id), + }, + PendingEventRecord { + event_type: EventType::RuntimeTransition, + runtime_state: RuntimeState::Planning, + commitment_id: Some(commitment_id.to_string()), + payload_json: json!({ + "schema_version": POLICY_SCHEMA_VERSION, + "action": "return_to_planning_after_review", + "from": "review", + "to": "planning", + }), + }, + ]) + .unwrap(); + } + + let reopened = SessionController::new(&path).unwrap(); + + assert_eq!(reopened.runtime_state(), RuntimeState::Planning); + assert!(reopened.active_commitment().is_none()); + assert!(reopened.active_policy().is_none()); + fs::remove_file(path).unwrap(); + } + #[test] fn rejects_standalone_commitment_created_on_replay() { let path = temp_log_path("standalone-commitment");