From fb2d3b1a8d0484aa7cc9b7f548ad1e898ffe0d9e Mon Sep 17 00:00:00 2001
From: Kayshen-X <kayshen_xu@163.com>
Date: Sun, 10 May 2026 11:05:53 +0800
Subject: [PATCH] =?UTF-8?q?fix(shell-core):=20Step=202=20codex=20R2+R3=20?=
 =?UTF-8?q?=E2=80=94=20empty-pages=20validate=20gap?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two iterative tightenings on Document::validate after the R1
fixes landed in 3d291ec8.

# R2 CONCERN-1: empty-pages document silently passed validate

`Document::validate` previously gated the active_page_index
range check on `!pages.is_empty()`, so a `Document { pages:
vec![], active_page_index: 99, ... }` returned `Ok(())` —
inconsistent with the implicit "every Document has at least one
page" invariant that Document::empty() and Document::sample()
both establish.

Fix:
- `validate()` now treats `pages.is_empty()` as the FIRST
  violation it returns. Empty pages is itself an invariant
  violation — `Document::empty()` is the constructor for the
  default single-page shape.
- `active_page_index` range check now fires unconditionally.

New test `document_validate_catches_empty_pages` covers two
sub-cases:
- `pages: vec![], active_page_index: 0` → Err("pages is empty")
- `pages: vec![], active_page_index: 99` → Err (empty check
  fires first, range check short-circuited)

# R3 CONCERN: empty-vs-range ordering not asserted

The R2 second sub-case only asserted `.is_err()` without
proving WHICH violation fired first. Strengthened to:
- assert error contains "pages is empty"
- assert error does NOT contain "active_page_index"
Both asserts carry failure messages so a future regression
points at the cause.

Test count: 34 lib + 21 widgets_static + 6 jian + 4
render_backend = 65 shell-core tests passing.

R4 GO from codex.
---
 crates/openpencil-shell-core/src/document.rs | 55 +++++++++++++++++++-
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/crates/openpencil-shell-core/src/document.rs b/crates/openpencil-shell-core/src/document.rs
index b6c05cd98..849f8e912 100644
--- a/crates/openpencil-shell-core/src/document.rs
+++ b/crates/openpencil-shell-core/src/document.rs
@@ -325,13 +325,26 @@ impl Document {
 
     /// Run light invariant checks on the document. Returns `Err`
     /// with a human-readable message on the first violation:
+    /// - `pages` is empty (a document must have at least one
+    ///   page; use `Document::empty()` to construct a default
+    ///   single-page document)
     /// - duplicate node id anywhere in any page
-    /// - `active_page_index` out of range (when there are pages)
+    /// - `active_page_index` out of range
+    ///
+    /// Codex Step 2 R2 CONCERN-1: the prior version skipped the
+    /// `active_page_index` check when `pages.is_empty()`, leaving
+    /// a (Document { pages: vec![], active_page_index: 99, … })
+    /// silently valid. Empty pages is itself an invariant
+    /// violation; this version rejects it explicitly so the
+    /// active_page_index check applies unconditionally.
     pub fn validate(&self) -> Result<(), String> {
+        if self.pages.is_empty() {
+            return Err("Document::pages is empty (use Document::empty() for the default single-page shape)".to_string());
+        }
         if let Some(dup) = self.find_duplicate_id() {
             return Err(format!("duplicate NodeId: {:?}", dup));
         }
-        if !self.pages.is_empty() && self.active_page_index >= self.pages.len() {
+        if self.active_page_index >= self.pages.len() {
             return Err(format!(
                 "active_page_index {} out of range (pages.len()={})",
                 self.active_page_index,
@@ -480,6 +493,44 @@ mod tests {
         assert!(result.unwrap_err().contains("active_page_index"));
     }
 
+    #[test]
+    fn document_validate_catches_empty_pages() {
+        // Codex Step 2 R2 CONCERN-1: an empty-pages document with
+        // any active_page_index used to silently pass validate().
+        // Now empty pages is itself a violation.
+        let doc = Document {
+            pages: Vec::new(),
+            active_page_index: 0,
+            selected: NodeId::NONE,
+        };
+        let result = doc.validate();
+        assert!(result.is_err());
+        assert!(
+            result.unwrap_err().contains("pages is empty"),
+            "validate should mention empty pages"
+        );
+
+        // Also covers the previously-uncaught case: empty pages
+        // + bogus active_page_index → still rejected, AND the
+        // empty-check fires FIRST (not the range check). Codex
+        // Step 2 R3 CONCERN: prior version only asserted
+        // `is_err()`, leaving ordering ambiguous.
+        let doc2 = Document {
+            pages: Vec::new(),
+            active_page_index: 99,
+            selected: NodeId::NONE,
+        };
+        let err2 = doc2.validate().unwrap_err();
+        assert!(
+            err2.contains("pages is empty"),
+            "empty-pages check must fire before active_page_index range check; got: {err2}"
+        );
+        assert!(
+            !err2.contains("active_page_index"),
+            "empty-pages check must short-circuit; got both: {err2}"
+        );
+    }
+
     #[test]
     fn document_selected_node_scopes_to_active_page() {
         // Codex Step 2 R1 CONCERN-1: a selection on page 1 must