fix(shell-core): Step 2 codex R2+R3 — empty-pages validate gap

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.
This commit is contained in:
Kayshen-X 2026-05-10 11:05:53 +08:00
parent aa966d0937
commit fb2d3b1a8d

View file

@ -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