fix: tighten readable diff review flow

Author: mohammedahmed18

crates/loc-cli/src/commands.rs

@@ -3829,7 +3829,13 @@ fn push(args: &[String], json: bool) -> i32 {
         let spinner_config =
             spinner_config_for_command("push", &target_label, json, stderr_is_terminal);
         let report = match with_terminal_spinner(spinner_config.clone(), || {
-            run_push_target_command(&mut store, &state_root, target.clone(), options.clone())
+            run_push_target_command(
+                &mut store,
+                &state_root,
+                target.clone(),
+                options.clone(),
+                None,
+            )
         }) {
             Ok(report) => report,
             Err(error) => {
@@ -3855,7 +3861,13 @@ fn push(args: &[String], json: bool) -> i32 {
                     let mut approved = options.clone();
                     approved.assume_yes = true;
                     match with_terminal_spinner(spinner_config, || {
-                        run_push_target_command(&mut store, &state_root, target.clone(), approved)
+                        run_push_target_command(
+                            &mut store,
+                            &state_root,
+                            target.clone(),
+                            approved,
+                            Some(&report),
+                        )
                     }) {
                         Ok(report) => report,
                         Err(error) => {
@@ -3993,9 +4005,19 @@ fn run_push_target_command(
     state_root: &Path,
     target_path: PathBuf,
     options: PushOptions,
+    expected_confirmation_preview: Option<&PushReport>,
 ) -> Result<PushReport, PushCommandError> {
     let preview = run_push_with_state_root(store, &target_path, options.clone(), Some(state_root))
         .map_err(PushCommandError::from_diff)?;
+    if let Some(expected) = expected_confirmation_preview {
+        if !push_confirmation_preview_matches_displayed(expected, &preview) {
+            return Err(PushCommandError::new(
+                "push_plan_changed",
+                "push plan changed after the confirmation preview; rerun `loc push` to review the current diff",
+                4,
+            ));
+        }
+    }
     if preview.pipeline_action != "proceed_to_apply" {
         return Ok(preview);
     }
@@ -4087,6 +4109,19 @@ fn push_preview_plan_matches(cli_preview: &PushReport, daemon_preview: &PushRepo
         && cli_preview.guardrail == daemon_preview.guardrail
 }
 
+fn push_confirmation_preview_matches_displayed(
+    displayed: &PushReport,
+    refreshed: &PushReport,
+) -> bool {
+    displayed.path == refreshed.path
+        && displayed.mount_id == refreshed.mount_id
+        && displayed.entity_id == refreshed.entity_id
+        && displayed.validation == refreshed.validation
+        && displayed.plan == refreshed.plan
+        && displayed.guardrail == refreshed.guardrail
+        && displayed.readable_diff == refreshed.readable_diff
+}
+
 fn should_prompt_for_push_confirmation(
     report: &PushReport,
     options: &PushOptions,
@@ -7156,9 +7191,10 @@ mod tests {
         mounted_projection_preflight_error, notion_authorize_url, notion_oauth_broker_config,
         print_push_confirmation_preview, projection_mode_for_target,
         projection_usage_options_for_target, prompt_for_push_confirmation,
-        pull_direct_fallback_error, should_prompt_for_push_confirmation,
-        should_refresh_notion_url_search, spinner_config_for_command, spinner_enabled,
-        status as run_status_command, validate_virtual_projection_registration, write_log_report,
+        pull_direct_fallback_error, push_confirmation_preview_matches_displayed,
+        should_prompt_for_push_confirmation, should_refresh_notion_url_search,
+        spinner_config_for_command, spinner_enabled, status as run_status_command,
+        validate_virtual_projection_registration, write_log_report,
     };
 
     #[test]
@@ -7868,6 +7904,31 @@ mod tests {
         );
     }
 
+    #[test]
+    fn push_confirmation_preview_match_includes_readable_diff() {
+        let mut displayed = push_report("confirm_plan");
+        displayed.readable_diff = Some(locality_core::readable_diff::ReadableDiffOutput {
+            files: Vec::new(),
+            text: "diff --locality a/Roadmap.md b/Roadmap.md\n-Old\n+New\n".to_string(),
+        });
+        let mut refreshed = displayed.clone();
+        refreshed.action = "apply_not_implemented".to_string();
+        refreshed.pipeline_action = "proceed_to_apply".to_string();
+
+        assert!(push_confirmation_preview_matches_displayed(
+            &displayed, &refreshed
+        ));
+
+        refreshed.readable_diff = Some(locality_core::readable_diff::ReadableDiffOutput {
+            files: Vec::new(),
+            text: "diff --locality a/Roadmap.md b/Roadmap.md\n-Old\n+Different\n".to_string(),
+        });
+
+        assert!(!push_confirmation_preview_matches_displayed(
+            &displayed, &refreshed
+        ));
+    }
+
     #[test]
     fn spinner_is_only_enabled_for_human_terminal_output() {
         assert!(spinner_enabled(false, true));

crates/loc-cli/src/diff.rs

@@ -213,7 +213,7 @@ pub struct DiffReport {
     pub unsupported: Vec<String>,
     pub message: Option<String>,
     pub suggested_fix: Option<String>,
-    #[serde(skip_serializing_if = "Option::is_none")]
+    #[serde(skip_serializing)]
     pub readable_diff: Option<ReadableDiffOutput>,
     pub completed_stages: Vec<String>,
 }

crates/loc-cli/tests/diff.rs

@@ -310,6 +310,26 @@ fn diff_report_includes_readable_patch_for_existing_page_edit() {
     );
 }
 
+#[test]
+fn diff_json_output_omits_readable_patch() {
+    let fixture = DiffFixture::new();
+    let mut store = fixture.store();
+    let path = fixture.write_page("Roadmap.md", "# Roadmap\n\nChanged paragraph.\n");
+    store
+        .save_shadow(&fixture.mount_id, shadow("# Roadmap\n\nOld paragraph.\n"))
+        .expect("save shadow");
+
+    let report = run_diff(&store, &path).expect("diff report");
+    assert!(
+        report.readable_diff.is_some(),
+        "expected internal readable diff"
+    );
+
+    let json = serde_json::to_value(&report).expect("serialize report");
+
+    assert!(json.get("readable_diff").is_none(), "{json:#}");
+}
+
 #[test]
 fn diff_plans_local_image_media_byte_update_from_manifest() {
     let fixture = DiffFixture::new();
@@ -517,6 +537,12 @@ fn diff_report_includes_readable_patch_for_created_entity() {
         "{}",
         readable.text
     );
+    assert!(
+        readable.text.contains("+title: New task"),
+        "{}",
+        readable.text
+    );
+    assert!(readable.text.contains("+Status: Todo"), "{}", readable.text);
     assert!(readable.text.contains("+# Notes"), "{}", readable.text);
     assert!(
         readable.text.contains("+Created locally."),

crates/localityd/src/push.rs

@@ -1334,7 +1334,7 @@ fn readable_diff_for_existing_entity(
 
 fn readable_diff_for_created_entity(
     source_path: &Path,
-    body: &str,
+    local_text: &str,
     pipeline: &PushPipelineResult,
 ) -> Option<locality_core::readable_diff::ReadableDiffOutput> {
     let plan = pipeline.plan.as_ref()?;
@@ -1344,7 +1344,7 @@ fn readable_diff_for_created_entity(
     locality_core::readable_diff::readable_diff_for_file(
         locality_platform::logical_path_display(source_path),
         None,
-        Some(body),
+        Some(local_text),
     )
 }
 
@@ -1662,8 +1662,8 @@ where
         },
         schema_validation,
     );
-    let readable_diff =
-        readable_diff_for_created_entity(&relative_path, &parsed.document.body, &pipeline);
+    let local_text = render_canonical_markdown(&parsed.document);
+    let readable_diff = readable_diff_for_created_entity(&relative_path, &local_text, &pipeline);
     Ok(PreparedPush {
         absolute_path,
         mount,
@@ -1997,8 +1997,9 @@ where
         },
         schema_validation,
     );
+    let local_text = render_canonical_markdown(&parsed.document);
     let readable_diff =
-        readable_diff_for_created_entity(&pending.projected_path, &parsed.document.body, &pipeline);
+        readable_diff_for_created_entity(&pending.projected_path, &local_text, &pipeline);
     Ok(PreparedPush {
         absolute_path,
         mount,