feat(value): introduce Array storage abstraction

[anakrish]

Adds an opaque Array newtype paralleling Object, living under src/value/array/ with the same module structure as src/value/object/ (mod.rs / iter.rs / serde.rs).

Array wraps Vec<Value> today but exposes only a curated surface: constructors, indexed access, first / last, push / pop, insert / remove, truncate, sort / sort_by, dedup, extend_from_slice, reverse, iter / iter_mut, cursor (resumable), serde, conversions to Value, and a hand-written lexicographic Ord. The cursor type is re-exported behind the rvm feature so the follow-up IterationState::Array swap can land additively.

No crate-root Array re-export is added; the new type lives under regorus::value::Array, matching the storage-abstraction namespace strategy for Object and Set.

Value::Array is unchanged in this PR (still Rc<Vec<Value>>); the payload swap and call-site migration ship in the next PR.

Tests live in src/value/tests.rs alongside the Object tests. This also repairs pre-existing doctest examples exercised by cargo test --doc --all-features, so the full requested validation suite passes.

[anakrish]

@copilot Review this PR using all the review skills in this repo. Use a separate agent for each skill.

[Copilot]

Pull request overview

Introduces a new value::Array storage abstraction (parallel to Object) with opaque iterator/cursor types, serde support, and an explicit lexicographic Ord, plus associated unit tests and several doctest fixes across the crate.

Changes:

• Added src/value/array/ module implementing Array, iterator newtypes, (feature-gated) cursor, and serde impls.
• Added comprehensive Array tests to src/value/tests.rs.
• Repaired multiple doctest imports/paths and an example target-loading snippet to keep cargo test --doc --all-features working.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/value/tests.rs	Adds unit tests covering Array constructors, mutation APIs, iteration, cursor (rvm), ordering, conversions, and serde.
src/value/mod.rs	Wires in the new array module and re-exports Array.
src/value/array/mod.rs	Core Array type and API surface, including ordering, indexing, and cursor iteration.
src/value/array/iter.rs	Opaque iterator newtypes and IntoIterator impls for Array and references.
src/value/array/serde.rs	Serialize/Deserialize implementations with allocator limit checks during decode.
src/schema/validate.rs	Updates doctest imports to use crate-root re-exports.
src/schema.rs	Adds missing doctest imports and updates example use paths.
src/engine.rs	Fixes a doctest indexing path into the AST JSON.
src/compiled_policy.rs	Fixes doctest target-loading by reading JSON then calling from_json_str.
docs/value/array.md	Adds documentation describing the new Array abstraction and its design goals.

[Copilot]

@copilot Review this PR using all the review skills in this repo. Use a separate agent for each skill.

Here are the results from both review skills run in parallel:

---

code-review skill — 2 findings (0 critical, 1 high, 1 medium)

HIGH | High confidence

Location: src/value/array/mod.rs:117
Issue: Array::remove, Array::insert, and Index<usize> all panic on out-of-bounds input — violating regorus's no-panic rule.

pub fn remove(&mut self, index: usize) -> Value { self.inner.remove(index) }
pub fn insert(&mut self, index: usize, value: Value) { self.inner.insert(index, value); }
fn index(&self, index: usize) -> &Self::Output { ops::Index::index(self.inner.as_slice(), index) }

Any out-of-bounds call panics. If Array reaches a FFI path this poisons the whole process.
Fix: Make these fallible (Option/Result), matching Object::remove's convention. Avoid publicly exposing Index<usize>.

---

MEDIUM | Medium confidence

Location: src/value/array/mod.rs:68
Issue: as_slice(), get_mut(), iter_mut(), and Index hand out Rust references into contiguous memory, already committing to Vec-specific semantics before Value::Array migrates.

Any future lazy/FFI-backed backend would need to fully materialize to satisfy these contracts, defeating the stated "no call-site changes" abstraction goal.
Fix: Defer as_slice, mutable borrows, and Index until the backend contract is settled.

---

deep-review skill — 7 findings (0 critical, 2 high, 3 medium, 2 low)

HIGH | High confidence — Panic-on-invalid-index (confirmed)

Same as above, confirmed by multiple agents and adversarial verifier.

---

HIGH | Medium confidence — Array growth bypasses memory-limit checks

Location: src/value/array/mod.rs:46–48, 102–151, 222–239

push, extend_from_slice, and with_capacity allocate directly through Vec with no enforce_limit() / check_memory_limit_if_needed() calls. The crate-wide accumulation pattern requires these guards; the only limit-aware path in this PR is Deserialize.

pub fn push(&mut self, value: Value) { self.inner.push(value); }
pub fn extend_from_slice(&mut self, other: &[Value]) { self.inner.extend_from_slice(other); }

Fix: Either add checked growth helpers or document that Array is not limit-enforcing outside serde/evaluation code.

---

MEDIUM | High confidence — remove freezes a panic-based semver contract

Location: src/value/array/mod.rs:122

Object::remove returns Option<Value>; Array::remove returns Value directly. Since the type is already public, changing this later is a breaking change.
Fix: Decide the API contract now; prefer Option<Value> for consistency with Object.

---

MEDIUM | High confidence — Serde is lossy for non-JSON-native Value variants (confirmed)

Location: src/value/array/serde.rs:15–18, 30–43

Arrays containing Undefined, Set, or objects with non-string keys don't round-trip correctly through JSON serde:

• [Undefined] → ["<undefined>"] → [String("<undefined>")]
• [Set({1,2})] → [[1,2]] → [Array([1,2])]

This is inherited from existing Value serde but may warrant documentation or a rejected-variant guard.

---

MEDIUM | Medium confidence — Public surface leaks Vec backend

Location: src/value/array/mod.rs:82–90, 159–163, 213–219

as_slice() and into_value() → Value::Array(Rc::new(self.inner)) make the current Vec-backed representation directly observable; tests also assert Value::Array(crate::Rc::new(vec![...])) directly, baking in the backend.

---

LOW | High confidence — No test coverage for invalid-index contract

Location: src/value/tests.rs:591

array_mutators_and_accessors only exercises valid indexes. Add a #[should_panic] test (or a test for fallible return if the API is changed) to document the intended contract.

---

LOW | Medium confidence — Test edge-case and abstraction gaps

Location: src/value/tests.rs:574, 680

with_capacity is only tested with 2; conversion assertions compare against the concrete Rc<Vec<_>> representation rather than public semantics. Add a limit-boundary test and rewrite conversion assertions through Value equality or public accessors.

---

Combined summary

Both skills independently identified the same two highest-priority issues: panic-on-bad-index in public mutating APIs and premature commitment to Vec semantics that undermines the abstraction goal. The deep-review additionally flagged missing enforce_limit() calls on growth paths (a crate-wide convention) and Object-inconsistent remove return type. These are the four items I'd prioritize before the payload-swap follow-up PR lands.

[anakrish]

@copilot Review this PR using the skills in the repo. Use a separate agent for each skill.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

[Copilot]

@copilot Review this PR using the skills in the repo. Use a separate agent for each skill.

Completed. I ran separate agents for both repo review skills (code-review and deep-review) and consolidated the results.

Validated findings:

• src/value/array/mod.rs: Array::with_capacity can panic on oversized capacities (confirmed).
• docs/value/array.md: docs list cursor unconditionally, but cursor/next are #[cfg(feature = "rvm")] (confirmed).
• src/value/array/mod.rs: growth helpers (push/extend_from_slice/Extend) currently rely on caller-side limit enforcement (likely).
• src/value/array/mod.rs: FromIterator<Value> likely shares the same overflow-panic class as capacity construction.

Dropped during adversarial verification:

• Undefined serde roundtrip concern is pre-existing in Value serde, not introduced by this PR.
• dual export path (pub mod array + pub use array::Array) treated as intentional API choice.

[anakrish]

Addressed the outstanding Array foundation review items:

• Updated docs/value/array.md to accurately describe the public API, rvm-gated cursor/next, and Vec-like surfaces that future backends may materialize.
• Made Array::with_capacity checked via try_reserve and return Option for oversized/unreservable capacities.
• Added memory-limit checks to Array growth paths: push/extend_from_slice now return Result; Extend/FromIterator route through the checked path.
• Added capacity boundary/overflow coverage and removed tests that asserted Value::Array with Rc<Vec<_>> directly.

Commits:

• 84d9b53 docs(value/array): accurate API surface and feature gating
• 87b26c3 feat(value/array): checked with_capacity
• ebc5165 feat(value/array): enforce memory limit on growth paths
• 25ba655 test(value/array): cover overflow + remove Vec-backend assertions

Intentionally deferred: no Value::Array migration or removal of ergonomic Vec-like APIs in this foundation PR; the docs now call out that future backends may need to materialize those views.

[Copilot]

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.

[anakrish]

@copilot Review this PR using all the review skills in this repo. Launch a separate agent for each skill.

[Copilot]

@copilot Review this PR using all the review skills in this repo. Launch a separate agent for each skill.

Ran both review skills with separate agents (code-review and deep-review) and consolidated results.

Findings:

1. Critical — Array::Extend/FromIterator can abort the process on recoverable memory-limit errors (src/value/array/mod.rs:259-277).
2. High — Array::insert can grow without memory-limit enforcement (src/value/array/mod.rs:132-137).
3. Medium — push/extend_from_slice can return Err after mutation (partial writes) (src/value/array/mod.rs:112-115,180-183).
4. Medium — with_capacity reserves without memory-limit enforcement (src/value/array/mod.rs:52-55).
5. Medium — From<Vec<Value>> for Array bypasses cooperative limit checks (src/value/array/mod.rs:281-285).
6. Medium (API risk) — pub mod array broadens semver surface (src/value/mod.rs:14-23).

Also noted test gaps for the memory-limit/error-path cases above in src/value/tests.rs.

[anakrish]

Addressed the follow-up review pass (Copilot consolidated findings + line comment on pub mod array) plus our own strict re-review.

Memory-limit correctness

• Dropped the process-aborting Extend/FromIterator impls; they now match the Object convention (delegate to inner container, no per-element cooperative-limit check). Limit-aware bulk insertion now goes through explicit fallible APIs.
• Added Array::try_from_vec and Array::try_from_iter as limit-aware counterparts to From<Vec<Value>> / FromIterator.
• with_capacity now also calls check_memory_limit_if_needed after try_reserve so capacity reservations cannot silently cross the cap.
• insert now returns Result<()>; bounds and limit failures both flow through anyhow. The limit case rolls back the inserted element.
• push and extend_from_slice roll back partial writes on limit failure so the array never observes a half-mutated state.

API surface

• Made array module private (was pub mod). Only Array (always) and ArrayCursor (rvm feature) are re-exported from crate::value — matching the Object / ObjectCursor pattern, no longer locking the module layout into the public API.
• Renamed the rvm-only Cursor to ArrayCursor for symmetry with ObjectCursor and to disambiguate at the value-module level.

Docs + tests

• Updated docs/value/array.md to reflect the private module, ArrayCursor rename, and the limit-enforcement contract (which methods enforce vs bypass).
• Added try_from_vec / try_from_iter happy-path tests and an out-of-bounds insert error test. Existing insert call sites migrated to the Result API.

Commit: 93216b2 fix(value/array): tighten memory-limit & API-surface review findings

Gates green: fmt, clippy (default + all-features + wasm bindings), 627 lib tests with --all-features, 242 lib tests default, 107 doc tests.

[anakrish]

@copilot Review this PR using all the review skills in this repo. Launch a separate agent for each skill.

[Copilot]

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.

[Copilot]

@copilot Review this PR using all the review skills in this repo. Launch a separate agent for each skill.

Completed. I ran separate agents for both review skills (code-review and deep-review) and consolidated the outputs.

Findings:

• code-review: 1 finding

  • High — memory-limit rollback in Array mutators restores length but may retain grown capacity (src/value/array/mod.rs).

• deep-review: 7 findings

  • High — throttled check_memory_limit_if_needed() can allow over-limit Array mutations/serde to return success (src/value/array/mod.rs, src/value/array/serde.rs).
  • Medium — with_capacity reserves before limit check (src/value/array/mod.rs).
  • Medium — iter/iter_mut return concrete iterator types that are not re-exported/nameable (src/value/array/mod.rs, src/value/mod.rs).
  • Medium (likely) — rollback may leave elevated capacity.
  • Low — three test gaps around limit/throttle semantics and iterator API exposure.

CI signal check: workflow run 27162681343 reports no failed jobs/logs (failed_jobs: 0).