diff --git a/src/main.rs b/src/main.rs index bee0124..f606e4e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -175,8 +175,6 @@ impl App { let mut session_controller = SessionController::new(event_log_path)?; match session_controller.runtime_state() { RuntimeState::Locked => session_controller.enter_planning()?, - // The rating screen is not durable enough to reconstruct after restart. - RuntimeState::Review => session_controller.return_to_planning_after_review()?, _ => {} } @@ -184,6 +182,7 @@ impl App { let state = match session_controller.runtime_state() { RuntimeState::Active if active_commitment.is_some() => State::InProgress, RuntimeState::Transition if active_commitment.is_some() => State::Paused, + RuntimeState::Review => State::End, _ => State::InputIntention, }; @@ -212,6 +211,11 @@ impl App { let session_start = now.checked_sub(restored_elapsed_capped).unwrap_or(now); let session_remaining = user_duration.saturating_sub(restored_elapsed); + let mut session_ratings = Vec::new(); + if session_controller.runtime_state() == RuntimeState::Review { + session_ratings.push(recoverable_review_rating()); + } + Ok(App { state, user_intention, @@ -231,7 +235,7 @@ impl App { session_start, session_stats: HashMap::new(), session_remaining, - session_ratings: Vec::new(), + session_ratings, session_ratings_index: 0, session_results: Vec::new(), acknowledged_unknown_window: None, @@ -379,9 +383,18 @@ impl App { } self.state = State::End; self.session_ratings = session_stats_as_vec(&self.session_stats); + self.ensure_recoverable_review_rating(); Ok(()) } + fn ensure_recoverable_review_rating(&mut self) { + if self.session_controller.runtime_state() == RuntimeState::Review + && self.session_ratings.is_empty() + { + self.session_ratings.push(recoverable_review_rating()); + } + } + fn cleanup(&self) { let path = shellexpand::tilde(constants::STATUS_FILE).to_string(); let _ = std::fs::remove_file(path); @@ -541,18 +554,25 @@ impl App { } fn complete_rating(&mut self) -> Result<()> { - if self.session_controller.runtime_state() == RuntimeState::Review { - self.session_controller.return_to_planning_after_review()?; - } - self.state = State::InputIntention; + self.ensure_recoverable_review_rating(); let mut session_result = SessionResult { intention: self.user_intention.clone(), duration: self.session_start.elapsed(), - session_ratings: std::mem::take(&mut self.session_ratings), + session_ratings: self.session_ratings.clone(), rating: 0, rating_f64: 0.0, }; session_result.rate(); + if self.session_controller.runtime_state() == RuntimeState::Review { + self.session_controller + .complete_review_and_return_to_planning( + session_result.rating, + session_result.rating_f64, + session_result.session_ratings.len() as u32, + )?; + } + self.state = State::InputIntention; + self.session_ratings.clear(); save_session(&session_result); self.session_results.push(session_result); Ok(()) @@ -729,6 +749,14 @@ fn session_stats_as_vec(session_stats: &HashMap, Duration>) -> Vec SessionRating { + SessionRating { + window_title: Rc::new("Recovered review summary".to_string()), + duration: Duration::from_secs(1), + rating: 0, + } +} + fn main() -> Result<()> { let result = run_app(); let raw_mode_result = disable_raw_mode(); @@ -778,6 +806,10 @@ fn handle_events(app: &mut App) -> Result<()> { fn handle_key_press(app: &mut App, key_code: KeyCode) -> Result<()> { if key_code == KeyCode::Esc && app.state != State::ViolationPrompt { + if app.state == State::End && app.session_controller.runtime_state() == RuntimeState::Review + { + return Ok(()); + } app.state = State::ShouldQuit; } @@ -1630,6 +1662,7 @@ mod tests { assert_eq!(app.state, State::End); assert_eq!(app.session_controller.runtime_state(), RuntimeState::Review); + app.session_ratings[0].rating = 2; app.complete_rating().unwrap(); assert_eq!(app.state, State::InputIntention); @@ -1642,11 +1675,26 @@ mod tests { assert_eq!(app.state, State::InProgress); assert_eq!(app.session_controller.runtime_state(), RuntimeState::Active); + let records = EventLog::open(&path).unwrap().records().unwrap(); + let review_completed_index = records + .iter() + .position(|record| record.event_type == EventType::ReviewCompleted) + .expect("review completion must be recorded durably"); + let planning_transition_index = records + .iter() + .position(|record| { + record.event_type == EventType::RuntimeTransition + && record.runtime_state == RuntimeState::Planning + && 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); + let _ = fs::remove_file(path); } #[test] - fn hydrated_review_session_returns_to_planning_and_can_start_next_commitment() { + fn hydrated_review_session_stays_recoverable_until_rating_completion() { let path = unique_event_log_path("hydrated-review-cycle"); { let mut app = App::new_with_event_log_path(&path).unwrap(); @@ -1660,15 +1708,33 @@ mod tests { let mut app = App::new_with_event_log_path(&path).unwrap(); + assert_eq!(app.state, State::End); + assert_eq!(app.session_controller.runtime_state(), RuntimeState::Review); + assert_eq!(app.session_ratings.len(), 1); + + handle_key_press(&mut app, KeyCode::Esc).unwrap(); + assert_eq!(app.state, State::End); + assert_eq!(app.session_controller.runtime_state(), RuntimeState::Review); + + app.user_intention = "write next test".to_string(); + app.user_success_condition = "next test passes".to_string(); + app.user_duration_str = "1".to_string(); + let err = app + .to_in_progress() + .expect_err("review must be completed before starting another commitment"); + assert!(err + .chain() + .any(|cause| cause.to_string().contains("activate runtime"))); + + app.session_ratings[0].rating = 3; + app.complete_rating().unwrap(); + assert_eq!(app.state, State::InputIntention); assert_eq!( app.session_controller.runtime_state(), RuntimeState::Planning ); - app.user_intention = "write next test".to_string(); - app.user_success_condition = "next test passes".to_string(); - app.user_duration_str = "1".to_string(); app.to_in_progress().unwrap(); assert_eq!(app.state, State::InProgress); @@ -1682,11 +1748,19 @@ mod tests { ); let records = EventLog::open(&path).unwrap().records().unwrap(); - assert!(records.iter().any(|record| { - record.event_type == EventType::RuntimeTransition - && record.runtime_state == RuntimeState::Planning - && record.payload_json["action"] == "return_to_planning_after_review" - })); + let review_completed_index = records + .iter() + .position(|record| record.event_type == EventType::ReviewCompleted) + .expect("review completion must be recorded"); + let planning_transition_index = records + .iter() + .position(|record| { + record.event_type == EventType::RuntimeTransition + && record.runtime_state == RuntimeState::Planning + && 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); let _ = fs::remove_file(path); } diff --git a/src/session.rs b/src/session.rs index c35a029..a20452b 100644 --- a/src/session.rs +++ b/src/session.rs @@ -84,6 +84,15 @@ struct EvidenceObservedPayload { health: EvidenceHealth, } +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +struct ReviewCompletedPayload { + schema_version: u16, + rating: u8, + rating_f64: f64, + reviewed_window_count: u32, +} + #[derive(Clone, Debug, PartialEq, Eq)] struct PendingTransitionPolicy { commitment_id: String, @@ -104,6 +113,7 @@ pub struct SessionController { runtime_state: RuntimeState, active_commitment: Option, active_policy: Option, + review_completed: bool, event_log: EventLog, } @@ -115,13 +125,14 @@ impl SessionController { event_log_path.as_ref().display() ) })?; - let (runtime_state, active_commitment, active_policy) = + let (runtime_state, active_commitment, active_policy, review_completed) = hydrate_session(&event_log).context("hydrate session from event log")?; Ok(Self { runtime_state, active_commitment, active_policy, + review_completed, event_log, }) } @@ -279,33 +290,70 @@ impl SessionController { self.runtime_state = next_runtime_state; self.active_commitment = None; self.active_policy = None; + self.review_completed = false; Ok(()) } - pub fn return_to_planning_after_review(&mut self) -> Result<()> { + pub fn complete_review_and_return_to_planning( + &mut self, + rating: u8, + rating_f64: f64, + reviewed_window_count: u32, + ) -> Result<()> { let previous_runtime_state = self.runtime_state; let next_runtime_state = transition_runtime(previous_runtime_state, RuntimeAction::ContinuePlanning) .context("continue planning after review runtime transition")?; + let completion_payload = ReviewCompletedPayload { + schema_version: POLICY_SCHEMA_VERSION, + rating, + rating_f64, + reviewed_window_count, + }; + validate_review_completed_payload(&completion_payload) + .context("validate review completed payload")?; + let transition_payload = RuntimeTransitionPayload { + schema_version: POLICY_SCHEMA_VERSION, + action: "return_to_planning_after_review".to_string(), + from: previous_runtime_state, + to: next_runtime_state, + }; - self.event_log - .append( - EventType::RuntimeTransition, - next_runtime_state, - None, - serde_json::to_value(RuntimeTransitionPayload { - schema_version: POLICY_SCHEMA_VERSION, - action: "return_to_planning_after_review".to_string(), - from: previous_runtime_state, - to: next_runtime_state, - }) - .context("serialize runtime transition payload")?, - ) - .context("log runtime transition from review to planning")?; + 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")?; + } else { + self.event_log + .append_batch([ + PendingEventRecord { + event_type: EventType::ReviewCompleted, + runtime_state: RuntimeState::Review, + commitment_id: None, + payload_json: serde_json::to_value(completion_payload) + .context("serialize review completed payload")?, + }, + PendingEventRecord { + event_type: EventType::RuntimeTransition, + runtime_state: next_runtime_state, + commitment_id: None, + payload_json: serde_json::to_value(transition_payload) + .context("serialize runtime transition payload")?, + }, + ]) + .context("log review completion and runtime transition to planning")?; + } self.runtime_state = next_runtime_state; self.active_commitment = None; self.active_policy = None; + self.review_completed = false; Ok(()) } @@ -596,11 +644,17 @@ impl SessionController { fn hydrate_session( event_log: &EventLog, -) -> Result<(RuntimeState, Option, Option)> { +) -> Result<( + RuntimeState, + Option, + Option, + bool, +)> { let mut runtime_state = RuntimeState::Locked; let mut active_commitment: Option = None; let mut active_policy: Option = None; let mut pending_transition_policy: Option = None; + let mut review_completed = false; for record in event_log.records()? { if pending_transition_policy.is_some() && record.event_type != EventType::PolicyApplied { @@ -636,6 +690,13 @@ 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 + )); + } + let next = transition_runtime(runtime_state, action).with_context(|| { format!("replay runtime transition at sequence {}", record.sequence) })?; @@ -646,12 +707,18 @@ fn hydrate_session( )); } runtime_state = next; - if matches!( - action, - RuntimeAction::CompleteForReview | RuntimeAction::ContinuePlanning - ) { - active_commitment = None; - active_policy = None; + match action { + RuntimeAction::CompleteForReview => { + active_commitment = None; + active_policy = None; + review_completed = false; + } + RuntimeAction::ContinuePlanning => { + active_commitment = None; + active_policy = None; + review_completed = false; + } + _ => {} } } EventType::CommitmentTransition => { @@ -976,12 +1043,36 @@ fn hydrate_session( } } } - ref unsupported => { - return Err(anyhow!( - "unsupported session event {:?} at sequence {}", - unsupported, - record.sequence - )); + EventType::ReviewCompleted => { + let payload: ReviewCompletedPayload = parse_payload(&record, "review completed")?; + ensure_schema_version(payload.schema_version, record.sequence)?; + validate_review_completed_payload(&payload).with_context(|| { + format!( + "validate review completed payload at sequence {}", + record.sequence + ) + })?; + if runtime_state != RuntimeState::Review + || record.runtime_state != RuntimeState::Review + { + return Err(anyhow!( + "review completion requires review runtime at sequence {}", + record.sequence + )); + } + if record.commitment_id.is_some() { + return Err(anyhow!( + "review completion must not reference a commitment at sequence {}", + record.sequence + )); + } + if review_completed { + return Err(anyhow!( + "review completion already recorded at sequence {}", + record.sequence + )); + } + review_completed = true; } } } @@ -1007,7 +1098,12 @@ fn hydrate_session( } } - Ok((runtime_state, active_commitment, active_policy)) + Ok(( + runtime_state, + active_commitment, + active_policy, + review_completed, + )) } fn apply_pending_transition_policy( @@ -1157,6 +1253,23 @@ fn validate_evidence_observed_payload(payload: &EvidenceObservedPayload) -> Resu Ok(()) } +fn validate_review_completed_payload(payload: &ReviewCompletedPayload) -> Result<()> { + if payload.rating > 3 { + return Err(anyhow!("review rating must be between 0 and 3")); + } + if !payload.rating_f64.is_finite() || !(0.0..=3.0).contains(&payload.rating_f64) { + return Err(anyhow!( + "review rating_f64 must be finite and between 0 and 3" + )); + } + if payload.rating > 0 && payload.reviewed_window_count == 0 { + return Err(anyhow!( + "reviewed window count is required when review rating is nonzero" + )); + } + Ok(()) +} + #[cfg(test)] mod tests { use super::{ @@ -1303,6 +1416,48 @@ mod tests { .unwrap(); } + fn append_review_session_without_completion(path: &PathBuf) { + let commitment_id = "commitment-test"; + let mut log = EventLog::open(path).unwrap(); + log.append_batch([ + PendingEventRecord { + event_type: EventType::RuntimeTransition, + runtime_state: RuntimeState::Planning, + commitment_id: None, + payload_json: json!({ + "schema_version": POLICY_SCHEMA_VERSION, + "action": "enter_planning", + "from": "locked", + "to": "planning", + }), + }, + PendingEventRecord { + event_type: EventType::CommitmentCreated, + runtime_state: RuntimeState::Active, + commitment_id: Some(commitment_id.to_string()), + payload_json: commitment_payload(commitment_id, CommitmentState::Active), + }, + PendingEventRecord { + event_type: EventType::PolicyApplied, + runtime_state: RuntimeState::Active, + commitment_id: Some(commitment_id.to_string()), + payload_json: policy_payload(commitment_id), + }, + PendingEventRecord { + event_type: EventType::RuntimeTransition, + runtime_state: RuntimeState::Review, + commitment_id: Some(commitment_id.to_string()), + payload_json: json!({ + "schema_version": POLICY_SCHEMA_VERSION, + "action": "complete_for_review", + "from": "active", + "to": "review", + }), + }, + ]) + .unwrap(); + } + fn error_chain_contains(err: &anyhow::Error, expected: &str) -> bool { err.chain() .any(|cause| cause.to_string().contains(expected)) @@ -1861,7 +2016,9 @@ mod tests { session.complete_for_review().unwrap(); assert_eq!(session.runtime_state(), RuntimeState::Review); - session.return_to_planning_after_review().unwrap(); + session + .complete_review_and_return_to_planning(2, 2.0, 1) + .unwrap(); assert_eq!(session.runtime_state(), RuntimeState::Planning); session @@ -1902,6 +2059,160 @@ mod tests { fs::remove_file(path).unwrap(); } + #[test] + fn replay_rejects_review_to_planning_without_review_completed() { + let path = temp_log_path("review-missing-completion"); + let commitment_id = "commitment-test"; + { + let mut log = EventLog::open(&path).unwrap(); + log.append_batch([ + PendingEventRecord { + event_type: EventType::RuntimeTransition, + runtime_state: RuntimeState::Planning, + commitment_id: None, + payload_json: json!({ + "schema_version": POLICY_SCHEMA_VERSION, + "action": "enter_planning", + "from": "locked", + "to": "planning", + }), + }, + PendingEventRecord { + event_type: EventType::CommitmentCreated, + runtime_state: RuntimeState::Active, + commitment_id: Some(commitment_id.to_string()), + payload_json: commitment_payload(commitment_id, CommitmentState::Active), + }, + PendingEventRecord { + event_type: EventType::PolicyApplied, + runtime_state: RuntimeState::Active, + commitment_id: Some(commitment_id.to_string()), + payload_json: policy_payload(commitment_id), + }, + PendingEventRecord { + event_type: EventType::RuntimeTransition, + runtime_state: RuntimeState::Review, + commitment_id: Some(commitment_id.to_string()), + payload_json: json!({ + "schema_version": POLICY_SCHEMA_VERSION, + "action": "complete_for_review", + "from": "active", + "to": "review", + }), + }, + PendingEventRecord { + event_type: EventType::RuntimeTransition, + runtime_state: RuntimeState::Planning, + commitment_id: None, + 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 without completion must be rejected"); + + assert!(error_chain_contains(&err, "review completion")); + fs::remove_file(path).unwrap(); + } + + #[test] + fn replay_tolerates_review_completed_without_changing_runtime() { + let path = temp_log_path("review-completed-no-transition"); + 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, + "rating": 2, + "rating_f64": 2.25, + "reviewed_window_count": 3, + }), + ) + .unwrap(); + } + + let reopened = SessionController::new(&path).unwrap(); + + assert_eq!(reopened.runtime_state(), RuntimeState::Review); + fs::remove_file(path).unwrap(); + } + + #[test] + fn replay_rejects_review_completed_outside_review() { + let path = temp_log_path("review-completed-outside-review"); + { + let mut log = EventLog::open(&path).unwrap(); + log.append_batch([ + PendingEventRecord { + event_type: EventType::RuntimeTransition, + runtime_state: RuntimeState::Planning, + commitment_id: None, + payload_json: json!({ + "schema_version": POLICY_SCHEMA_VERSION, + "action": "enter_planning", + "from": "locked", + "to": "planning", + }), + }, + PendingEventRecord { + event_type: EventType::ReviewCompleted, + runtime_state: RuntimeState::Planning, + commitment_id: None, + payload_json: json!({ + "schema_version": POLICY_SCHEMA_VERSION, + "rating": 2, + "rating_f64": 2.0, + "reviewed_window_count": 1, + }), + }, + ]) + .unwrap(); + } + + let err = SessionController::new(&path) + .expect_err("review completion outside review must be rejected"); + + assert!(error_chain_contains(&err, "review runtime")); + fs::remove_file(path).unwrap(); + } + + #[test] + fn replay_rejects_malformed_review_completed() { + let path = temp_log_path("review-completed-malformed"); + 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, + "rating": 4, + "rating_f64": 4.0, + "reviewed_window_count": 1, + }), + ) + .unwrap(); + } + + let err = SessionController::new(&path).expect_err("malformed review completion must fail"); + + assert!(error_chain_contains(&err, "review rating")); + fs::remove_file(path).unwrap(); + } + #[test] fn rejects_standalone_commitment_created_on_replay() { let path = temp_log_path("standalone-commitment");