Make review completion durable

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.ensure_recoverable_review_rating();
-            self.session_controller.return_to_planning_after_review()?;
-        }
-        self.state = State::InputIntention;
         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<Rc<String>, Duration>) -> Vec<Se
     stats
 }
 
+fn recoverable_review_rating() -> 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| {
+        let review_completed_index = records
-            record.event_type == EventType::RuntimeTransition
+            .iter()
-                && record.runtime_state == RuntimeState::Planning
+            .position(|record| record.event_type == EventType::ReviewCompleted)
-                && record.payload_json["action"] == "return_to_planning_after_review"
+            .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);
     }

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<Commitment>,
     active_policy: Option<PolicySnapshot>,
+    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
+        if self.review_completed {
-            .append(
+            self.event_log
-                EventType::RuntimeTransition,
+                .append(
-                next_runtime_state,
+                    EventType::RuntimeTransition,
-                None,
+                    next_runtime_state,
-                serde_json::to_value(RuntimeTransitionPayload {
+                    None,
-                    schema_version: POLICY_SCHEMA_VERSION,
+                    serde_json::to_value(transition_payload)
-                    action: "return_to_planning_after_review".to_string(),
+                        .context("serialize runtime transition payload")?,
-                    from: previous_runtime_state,
+                )
-                    to: next_runtime_state,
+                .context("log runtime transition from review to planning")?;
-                })
+        } else {
-                .context("serialize runtime transition payload")?,
+            self.event_log
-            )
+                .append_batch([
-            .context("log runtime transition from review to planning")?;
+                    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<Commitment>, Option<PolicySnapshot>)> {
+) -> Result<(
+    RuntimeState,
+    Option<Commitment>,
+    Option<PolicySnapshot>,
+    bool,
+)> {
     let mut runtime_state = RuntimeState::Locked;
     let mut active_commitment: Option<Commitment> = None;
     let mut active_policy: Option<PolicySnapshot> = None;
     let mut pending_transition_policy: Option<PendingTransitionPolicy> = 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!(
+                match action {
-                    action,
+                    RuntimeAction::CompleteForReview => {
-                    RuntimeAction::CompleteForReview | RuntimeAction::ContinuePlanning
+                        active_commitment = None;
-                ) {
+                        active_policy = None;
-                    active_commitment = None;
+                        review_completed = false;
-                    active_policy = None;
+                    }
+                    RuntimeAction::ContinuePlanning => {
+                        active_commitment = None;
+                        active_policy = None;
+                        review_completed = false;
+                    }
+                    _ => {}
                 }
             }
             EventType::CommitmentTransition => {
@@ -976,12 +1043,36 @@ fn hydrate_session(
                     }
                 }
             }
-            ref unsupported => {
+            EventType::ReviewCompleted => {
-                return Err(anyhow!(
+                let payload: ReviewCompletedPayload = parse_payload(&record, "review completed")?;
-                    "unsupported session event {:?} at sequence {}",
+                ensure_schema_version(payload.schema_version, record.sequence)?;
-                    unsupported,
+                validate_review_completed_payload(&payload).with_context(|| {
-                    record.sequence
+                    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");