refactor(error): typed sub-enums in place of stringly-typed reason fields

.changeset/review-fixes-2026-06.md

@@ -0,0 +1,55 @@
+---
+default: minor
+---
+
+#### Review sweep: doc/code parity, idiom fixes, typed `InvalidAttrKind`
+
+Multi-reviewer audit pass (6 Claude agents + 1 Codex independent run, verified against Rust 1.95 / Rust API Guidelines / napi-rs v3). Findings cluster into doc/code contradictions, internal cleanup, and one public API tightening.
+
+**Public API (minor):**
+- `ValidationError::InvalidAttr.reason: String` → `kind: InvalidAttrKind`. The new typed enum carries the allowed-enum list or the regex pattern as structured fields callers can `match` on, instead of burying them in a diagnostic string. `Display` output is byte-identical to the prior format, so log lines, snapshot tests, and human-facing messages are unchanged. `InvalidAttrKind` is re-exported at the crate root. Breaking only for consumers that constructed `ValidationError::InvalidAttr` directly or destructured the `reason` field.
+
+**Doc/code parity fixes:**
+- `docs/ARCHITECTURE.md` Mutation section now describes the *ascending* splice sort with a forward cursor (matches the implementation in `mutate::apply_splices`). The previous "descending" wording was correct only for an in-place `String::replace_range` strategy that this crate doesn't use.
+- `docs/dsl/selectors.md` correctly describes nested `:not()` as permitted-up-to-depth-64 (matches the parser); the prior "rejected" wording was misleading.
+- `Markdown::to_xml` rustdoc now explicitly warns the output is re-serialized, not byte-preserved, and that `SourcePosition` values from the parsed document don't apply to the output.
+- Removed stale `#![doc(html_root_url = "https://docs.rs/marxml/0.0.0")]` that broke cross-crate doc links after the first published release.
+- Node binding regex-flags docstring now describes the `RegexBuilder` setter mechanism (matches the implementation); the prior `(?flags:…)` prefix claim was wrong about the mechanism though correct about observable behavior.
+- Node binding `toJson` JSDoc no longer claims "no string round-trip" — the wrapper hides one inside `marxml.mjs` and that's worth being honest about.
+- Crate-root design notes in `bindings/node/src/lib.rs` clarify "no per-call reparse" applies to the document, not the selector (selector recompile-once is roadmap work).
+
+**`#[allow]` justifications (the repo Never rule on unjustified suppressions):**
+- `selector/matcher.rs::walk` — `clippy::too_many_arguments` now carries a comment naming the trade-off (no indirection on the hot recursive call) and pointing at criterion as the arbiter.
+- `bindings/node/src/lib.rs` — the three crate-level `#![allow]`s (`needless_pass_by_value`, `missing_errors_doc`, `missing_panics_doc`) each get a per-lint rationale comment.
+
+**Internal cleanup:**
+- Renamed `mutate::try_replace_content` / `try_replace_in` → `splice_content_report` / `splice_regex_report`. The `try_*` prefix is reserved for fallible operations; these always return a `MutationReport` so they're not fallible. Internal (`pub(crate)`); public re-export names (`replace_content_report`, `replace_in_report`) unchanged.
+- Removed the dead `splice_regex` one-line wrapper. `replace_in` / `splice_regex_report` now call `splice_regex_with` directly.
+- Dropped the unused `once_cell = "1.19"` workspace dependency (no source file referenced it).
+- Added `[package.metadata.docs.rs]` to `crates/marxml/Cargo.toml` for future feature-gated items.
+
+**Microopts (no benchmark required — straight wins):**
+- `mutate::rewrite_open_tag` now allocates with `String::with_capacity(...)` sized from the tag + existing-attr + new-attr byte budget.
+- `serialize::to_xml` now allocates with `String::with_capacity(doc.raw().len())`.
+- `tokenizer::record_seen_attr` builds the lazy `HashSet` with `iter().map().collect()` instead of `with_capacity` + a manual loop.
+- `escape::decode_entities` replaced `.chars().next().expect("non-empty tail")` with `let Some(ch) = ... else { unreachable!(...) }` — same runtime behavior, expressed via the type system.
+
+**Diagnostic fix:**
+- `ParseError::MalformedAttribute { kind: UnterminatedValue, .. }` now reports the line at which input ran out, not the line of the attribute name. For attributes whose values span multiple lines and run off the end, the error now points at the actual EOF rather than far above. One snapshot updated accordingly.
+
+**New contributor doc:**
+- `contributing/rust-best-practices.md` codifies the rubric the review pass used: Rust idiom checklist, napi-rs v3 specifics, what 2024 edition would change (deferred), reviewer severity levels. Companion to AGENTS.md's hard rules.
+
+### Deferred (Ask First — not in this changeset)
+
+These came out of the same review pass but each requires a design call before implementation. Tracked for follow-up:
+
+- Compile-once `Selector` / `Schema` napi classes (Node API addition).
+- Async `parse` / mutator variants for multi-MB inputs.
+- `linux-arm64-musl` napi target.
+- `SelectorError::UnexpectedEnd { at: usize }` field add.
+- `SerializeOpts::self_close_empty()` builder rename to remove field/method shadow.
+- `is_name_start` / `is_name_char` visibility decision (re-export vs `pub(crate)` downgrade).
+- `ElementRef` `PartialEq` / `Eq` derives + semantic decision (pointer vs value).
+- `AttrConstraint` accessor methods.
+- Edition 2024 migration + MSRV bump to 1.85.

.changeset/typed-error-kinds.md

@@ -0,0 +1,27 @@
+---
+default: minor
+---
+
+#### Refactor: replace stringly-typed `reason: String` error fields with typed sub-enums
+
+The public error types in `marxml` previously buried the actual failure mode inside a `reason: String` interpolated at the call site. Three error variants used this pattern, which forced consumers to string-match on diagnostic prose to discriminate failure modes:
+
+- `ParseError::MalformedTag { reason: String, line: u32 }`
+- `ParseError::MalformedAttribute { tag: String, reason: String, line: u32 }`
+- `SelectorError::Syntax { reason: String, at: usize }`
+
+Plus one foreign-error wrap that stringified the underlying cause instead of preserving the source chain:
+
+- `SchemaError::InvalidRegex { tag: String, attr: String, reason: String }`
+
+This release replaces `reason: String` with typed sub-enums that callers can `match` on:
+
+- `MalformedTag.kind: MalformedTagKind` — `ExpectedCloseAngle`, `UnterminatedOpenTag`, `ExpectedCloseSlashAngle`, `UnterminatedComment`, `UnterminatedCdata`.
+- `MalformedAttribute.kind: MalformedAttrKind` — `UnexpectedNameStart{found:char}`, `ExpectedEquals{attr}`, `ExpectedOpenQuote{attr}`, `UnterminatedValue{attr}`.
+- `SelectorError::Syntax.kind: SyntaxKind` — `UnionTooLarge{max}`, `CompoundTooLong{max}`, `TooManyPredicates{max}`, `NotNestingTooDeep{max}`, `UnsupportedPseudoClass{name}`, `NthChildMustBeOneOrGreater`, `IntegerOutOfRange`, `ExpectedDigit`, plus a catch-all `Expected{what: &'static str}` for the structural "parser expected token X" cases.
+
+`SchemaError::InvalidRegex` now carries `#[source] source: regex::Error` instead of `reason: String`, so `Error::source()` walks the cause chain. `SchemaError` drops its `Eq` derive in the process (`regex::Error` is `PartialEq` but not `Eq`); `PartialEq` is retained.
+
+`Display` output is unchanged across every variant — the new `#[error("…")]` strings reproduce the original prose verbatim, so log lines, snapshot tests, and human-facing error messages are identical.
+
+Breaking change for any consumer that constructed these errors directly or matched on the `reason` field. Consumers that only pattern-matched on the variant tag (`matches!(e, ParseError::MalformedTag { .. })`) or read `Display` output are unaffected.

Cargo.toml

@@ -3,6 +3,8 @@ resolver = "2"
 members = [
   "crates/marxml",
   "bindings/node",
+  "examples/rust-simple",
+  "examples/rust-advanced",
 ]
 
 [workspace.package]
@@ -19,7 +21,6 @@ regex = "1.10"
 thiserror = "2.0"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
-once_cell = "1.19"
 
 # Dev-dependencies (used by tests/benches in member crates)
 rstest = "0.26"

bindings/node/marxml.d.ts

@@ -98,8 +98,11 @@ export interface MarkdownDoc {
   toXml(opts?: ToXmlOpts): string
 
   /**
-   * Serialize the element tree as a JSON value (already parsed; no string
-   * round-trip needed at the call site).
+   * Serialize the element tree as a JSON value. The native binding emits a
+   * JSON string which the wrapper at `marxml.mjs` parses for you — at the
+   * call site you receive a structured value, but the cost is two passes
+   * (one Rust-side serialize, one V8 `JSON.parse`). For large documents
+   * prefer `toXml({ pretty: false })` if you only need a serialized form.
    *
    * Top-level is an array of root elements. Each element carries
    * `tag` / `attrs` / `text` / `children` / `selfClosing` / `location`.

bindings/node/src/lib.rs

@@ -7,13 +7,26 @@
 //!
 //! Design notes:
 //! - The document is parsed once into the `NativeMarkdown` handle. Subsequent
-//!   queries and mutations reuse that handle — no per-call reparse.
-//! - All fallible operations route through the crate's `try_*` variants and
-//!   surface errors as `napi::Error` with `InvalidArg` status. The binding
-//!   does not panic on caller-supplied input.
+//!   queries and mutations reuse that handle — the document is never
+//!   reparsed. Selector strings are still parsed per call (see follow-up
+//!   work — exposing a compiled `Selector` class).
+//! - All fallible crate calls are mapped to `napi::Error` via the `From`
+//!   impls below, so call sites use `?` / `Into::into` rather than ad-hoc
+//!   `.map_err(|e| Error::new(...))` closures.
 //! - `Element` is still a flat `#[napi(object)]` POJO for the `elements`
 //!   getter; materializing the whole tree as opaque handles is a separate
 //!   refactor.
+//!
+//! Clippy allowances below are justified per-lint:
+//! - `needless_pass_by_value`: napi-rs expands `#[napi]` methods into FFI
+//!   signatures that take owned JS bridge values; switching to `&str` is
+//!   not yet supported uniformly in v3 derive output.
+//! - `missing_errors_doc`: error doc comments are intentionally on the
+//!   `marxml::*Error` types in the core crate; the binding is a transparent
+//!   pass-through and duplicating them rots.
+//! - `missing_panics_doc`: napi-derive expansion contains FFI panic edges
+//!   that are unreachable from caller-shaped input. Documenting "may panic
+//!   if napi's FFI layer is broken" is noise.
 
 #![allow(clippy::needless_pass_by_value)]
 #![allow(clippy::missing_errors_doc)]
@@ -26,6 +39,54 @@ use napi::{Error, Result, Status};
 use napi_derive::napi;
 use regex::{Regex, RegexBuilder};
 
+// ─── Error mapping ────────────────────────────────────────────────────────
+
+/// Map any crate-side `Result<T, E: std::error::Error>` into `napi::Result<T>`
+/// with `Status::InvalidArg`. The orphan rule prevents a direct
+/// `From<marxml::*Error> for napi::Error` impl in this crate, so this
+/// extension trait stands in: call sites read `marxml::Selector::parse(s)
+/// .into_napi()?` instead of repeating the `.map_err(|e| Error::new(...))`
+/// closure on every fallible boundary.
+///
+/// Every marxml error variant is caller-input (malformed selector,
+/// duplicate attribute, invalid XML name, regex compile failure), so a
+/// single `InvalidArg` status fits all of them. If a future variant ever
+/// represents a binding-internal failure, swap the call site to an explicit
+/// `Error::new(Status::GenericFailure, ...)` and document why.
+trait IntoNapi<T> {
+    fn into_napi(self) -> Result<T>;
+}
+
+impl<T> IntoNapi<T> for std::result::Result<T, marxml::ParseError> {
+    fn into_napi(self) -> Result<T> {
+        self.map_err(|e| Error::new(Status::InvalidArg, e.to_string()))
+    }
+}
+
+impl<T> IntoNapi<T> for std::result::Result<T, marxml::SelectorError> {
+    fn into_napi(self) -> Result<T> {
+        self.map_err(|e| Error::new(Status::InvalidArg, e.to_string()))
+    }
+}
+
+impl<T> IntoNapi<T> for std::result::Result<T, marxml::MutateError> {
+    fn into_napi(self) -> Result<T> {
+        self.map_err(|e| Error::new(Status::InvalidArg, e.to_string()))
+    }
+}
+
+impl<T> IntoNapi<T> for std::result::Result<T, marxml::SchemaError> {
+    fn into_napi(self) -> Result<T> {
+        self.map_err(|e| Error::new(Status::InvalidArg, e.to_string()))
+    }
+}
+
+impl<T> IntoNapi<T> for std::result::Result<T, regex::Error> {
+    fn into_napi(self) -> Result<T> {
+        self.map_err(|e| Error::new(Status::InvalidArg, e.to_string()))
+    }
+}
+
 // ─── Flat shape types crossing the FFI ────────────────────────────────────
 
 /// One-based line + zero-based byte offset into the source document.
@@ -233,7 +294,7 @@ impl NativeMarkdown {
         self.inner
             .try_update(&sel, &pairs)
             .map(|report| report.output)
-            .map_err(|e| Error::new(Status::InvalidArg, e.to_string()))
+            .into_napi()
     }
 
     /// Replace inner content verbatim. `new_body` is spliced as raw bytes —
@@ -256,8 +317,11 @@ impl NativeMarkdown {
     /// Run a regex `replace_all` over the inner content of matching
     /// elements. `pattern` accepts either a plain string or a `RegExpShape`
     /// (i.e. the destructured fields of a JS `RegExp`). JS regex flags
-    /// `i`/`m`/`s`/`x` are honored via Rust's `(?flags:…)` prefix; `g` is a
-    /// no-op (`replace_all` is global by default).
+    /// `i`/`m`/`s`/`x` are honored via `RegexBuilder` setters
+    /// (`case_insensitive`, `multi_line`, `dot_matches_new_line`,
+    /// `ignore_whitespace`); `g`/`u`/`y`/`d` are accepted-and-ignored
+    /// (`replace_all` is already global; `u` is implicit; `y`/`d` have no
+    /// Rust equivalent). Any other flag returns `InvalidArg`.
     ///
     /// `replacement` is verbatim text — `$1` / `$name` are NOT interpreted as
     /// capture references.
@@ -329,13 +393,13 @@ impl NativeMarkdown {
 pub fn parse(source: String) -> Result<NativeMarkdown> {
     marxml::parse(&source)
         .map(|inner| NativeMarkdown { inner })
-        .map_err(|e| Error::new(Status::InvalidArg, e.to_string()))
+        .into_napi()
 }
 
 // ─── Internal helpers ─────────────────────────────────────────────────────
 
 fn parse_selector(s: &str) -> Result<marxml::Selector> {
-    marxml::Selector::parse(s).map_err(|e| Error::new(Status::InvalidArg, e.to_string()))
+    marxml::Selector::parse(s).into_napi()
 }
 
 fn compile_regex(pattern: Either<String, RegExpShape>) -> Result<Regex> {
@@ -368,9 +432,7 @@ fn compile_regex(pattern: Either<String, RegExpShape>) -> Result<Regex> {
             }
         }
     }
-    builder
-        .build()
-        .map_err(|e| Error::new(Status::InvalidArg, e.to_string()))
+    builder.build().into_napi()
 }
 
 fn build_schema(input: HashMap<String, TagSchemaShape>) -> Result<marxml::Schema> {
@@ -409,9 +471,7 @@ fn build_schema(input: HashMap<String, TagSchemaShape>) -> Result<marxml::Schema
             tb
         });
     }
-    builder
-        .try_build()
-        .map_err(|e| Error::new(Status::InvalidArg, e.to_string()))
+    builder.try_build().into_napi()
 }
 
 fn error_kind(e: &marxml::ValidationError) -> &'static str {