feat: add aster-forge-xml crate to replace xmltree#3
Merged
Conversation
- Complete implementation of XML tree library using quick-xml - Feature parity with xmltree crate - Includes parser, serializer, and tree operations - Security checks: depth limit, size limit, DTD disabled - Full test coverage Closes AsterCommunity#2"
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough新增 Changesaster_forge_xml XML 核心链路
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
aster_forge_xml 性能基准报告
测试说明基准测试覆盖以下场景：
结果解析性能序列化性能Round-trip详细数据
性能特性分析1. 解析器时间复杂度
2. 序列化性能
3. 深度嵌套处理
4. 内存使用
与 xmltree（xml-rs）对比预估
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
docs/crates/aster_forge_xml.md (1)
168-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win给这个安全检查示意图代码块加个语言标识，markdownlint 已经点名了。
📝 建议修复
-``` +```text 输入字节流🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/crates/aster_forge_xml.md` at line 168, 为安全检查示意图所在的 Markdown 代码块添加 text 语言标识，将无语言标识的围栏改为 ```text，并保持代码块内容不变。Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aster_forge_xml/src/lib.rs`:
- Around line 256-279: Remove the unsafe pointer-based
descendants_mut/collect_mut_ptr implementation because it returns overlapping
mutable references to ancestors and descendants. Replace the API with a
non-aliasing design, preferably a mutable visitor such as visit_mut that invokes
the callback during traversal, or return paths/indices for later disjoint
access; update callers to use the new interface and preserve traversal coverage.
- Around line 8-14: 将 lib.rs 中的 serializer 模块声明从公开改为私有，保持与 parser 的模块封装方式一致；继续通过
pub use serializer::SerializeOptions 导出必要的配置类型，不要改变 Element::write、Display
或其他现有序列化入口。
In `@crates/aster_forge_xml/src/parser.rs`:
- Around line 126-201: Update parse() after the root element is built in the
Start and Empty branches to consume events through Eof before returning. Skip
comments, processing instructions, and whitespace-only text; return
Error::InvalidXml("extra content after root node".into()) for any additional
element or non-whitespace content, while preserving the existing root parsing
and error behavior.
- Around line 128-149: 在 build_tree 的 Event::Start 分支递归构造元素前，以及 parse() 的顶层
Event::Start 分支中，补充与 Event::Empty 分支一致的 check_element_count
调用；确保每个非自闭合元素（包括根元素）在创建前都计入 max_elements，并保留现有超限处理行为。
---
Nitpick comments:
In `@docs/crates/aster_forge_xml.md`:
- Line 168: 为安全检查示意图所在的 Markdown 代码块添加 text 语言标识，将无语言标识的围栏改为 ```text，并保持代码块内容不变。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c1703cf-5611-457c-87c9-3626c6a2f9cf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcrates/aster_forge_xml/Cargo.tomlcrates/aster_forge_xml/src/error.rscrates/aster_forge_xml/src/lib.rscrates/aster_forge_xml/src/parser.rscrates/aster_forge_xml/src/serializer.rsdocs/crates/aster_forge_xml.md
…rser Replace the lightweight `aster_forge_utils::xml` validation shim with a full-featured, safety-bounded XML library in `aster_forge_xml`. - Add `BorrowedDocument` / `XmlDocument` arena parser that keeps all plain values as zero-copy source slices and allocates only for decoded entities; supports `write_original` for byte-exact round-trips - Add `XmlStreamReader` for namespace-resolved event streaming with `skip_current`, `capture_current`, and `read_text_current` helpers; enforces all safety limits incrementally without buffering the full input - Add `XmlStreamWriter` with `XmlWriteOptions` for bounded, validated XML generation; rejects invalid names, duplicate attributes, forbidden namespace bindings, and unsafe character values at write time - Expand `XmlSafetyPolicy` with granular limits: `max_input_bytes`, `max_elements`, `max_attributes_per_element`, `max_text_bytes`, and `max_events` in addition to existing `max_depth` and `reject_doctype` - Redesign `Error` / `XmlSafetyError` hierarchy: `XmlSafetyError` is now `Copy + Eq`, carries `InvalidEncoding`, `OutputTooLarge`, `TooManyElements`, `TooManyAttributes`, and `TextTooLarge` variants, and is wrapped by `Error::Safety`; `Error::Io` carries the original `std::io::Error` rather than a string - Remove `aster_forge_utils::xml` module and `quick-xml` dependency from `aster_forge_utils`; callers migrate to `aster_forge_xml` directly - Add integration test suites: `xml`, `document`, `stream`, `writer`, `roxmltree_compat`, `xmltree_compat`, and a proptest property suite with a saved regression seed - Add libfuzzer fuzz targets `parse_stream` and `writer_roundtrip` - Add Criterion benchmarks for parse, write, and multistatus generation comparing `forge_arena`, `roxmltree`, `xmltree`, and `quick-xml` - Register `aster_forge_xml` in docs sidebar, index, new-project guide, and README utilities table; remove the xml section from `aster_forge_utils` docs
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (10)
crates/aster_forge_xml/benches/support/mod.rs (2)
150-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win两份一模一样的策略构造，抽个 helper。
L151-157 和 L199-205 完全重复；将来改一处忘另一处，两个 bench 就不再可比。另外这里放宽的
max_elements/max_events/max_text_bytes与xml_bench.rs中 DOM 路径使用的XmlSafetyPolicy::untrusted()默认值不同，跨路径对比时请注意这层不对称。♻️ 建议抽取
+fn benchmark_policy(input: &[u8]) -> XmlSafetyPolicy { + XmlSafetyPolicy { + max_input_bytes: input.len().max(1), + max_elements: 1_000_000, + max_text_bytes: input.len().max(1), + max_events: 10_000_000, + ..XmlSafetyPolicy::untrusted() + } +} + pub(crate) fn walk_forge_stream(input: &[u8]) -> usize { - let policy = XmlSafetyPolicy { - max_input_bytes: input.len().max(1), - max_elements: 1_000_000, - max_text_bytes: input.len().max(1), - max_events: 10_000_000, - ..XmlSafetyPolicy::untrusted() - }; + let policy = benchmark_policy(input); let mut reader = XmlStreamReader::new(input, policy).expect("benchmark policy is valid");
validate_forge_stream同样替换。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_xml/benches/support/mod.rs` around lines 150 - 215, Extract the duplicated XmlSafetyPolicy construction from walk_forge_stream and validate_forge_stream into a shared helper, then have both functions obtain their reader policy through that helper. Preserve the existing input-dependent limits and relaxed max_elements/max_text_bytes/max_events values so both benchmark paths remain comparable.
115-122: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
unescape在新版 quick-xml 上是多余开销，会给对照组抹黑。quick-xml 已把
BytesText::unescape换成decode，Text 事件不再包含转义片段，实体统一由Event::GeneralRef上报。这里decode()之后再unescape()属于纯多余扫描，还会把 fixture 里"/<的处理成本重复计入 quick-xml 对照路径——你在 PR 里贴的对比表就是靠这些数字支撑的。♻️ 去掉多余的 unescape
Event::Text(text) => { - let decoded = text.decode().expect("benchmark text is valid"); - checksum = checksum.wrapping_add( - unescape(&decoded) - .expect("benchmark text entities are valid") - .len(), - ); + checksum = checksum + .wrapping_add(text.decode().expect("benchmark text is valid").len()); }顺带移除顶部的
use quick_xml::escape::unescape;。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_xml/benches/support/mod.rs` around lines 115 - 122, 在基准测试的 Event::Text 分支中移除对 unescape 的调用，使 checksum 直接使用 text.decode() 的结果长度；同时删除顶部对 quick_xml::escape::unescape 的导入，保留现有有效性断言和其他事件处理逻辑不变。crates/aster_forge_xml/benches/xml_memory.rs (3)
295-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win子进程失败时把 stderr 带上，不然只能靠占卜调试。
♻️ 建议改法
- assert!(output.status.success(), "memory benchmark child failed"); + assert!( + output.status.success(), + "memory benchmark child failed: mode={mode} fixture={fixture_name}\n{}", + String::from_utf8_lossy(&output.stderr) + );两处
assert!都建议这么改。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_xml/benches/xml_memory.rs` around lines 295 - 315, Update both child-process success assertions in the benchmark loops to include the captured output.stderr when the command fails, while preserving the existing success check and stdout printing behavior.
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
i64_to_u64只在 unix cfg 下用到，Windows 上就是个未使用导入。你自己的跑分报告写的是 Windows 10 —— 那条路走
#[cfg(not(any(target_os = "linux", target_os = "macos")))]分支，L10 的导入没人用。CI 只要带-D warnings，非 unix 目标立刻炸。给导入加上同样的 cfg 或把它挪进函数体内。♻️ 建议改法
-use aster_forge_utils::numbers::i64_to_u64; use aster_forge_xml::{BorrowedDocument, ValidatedXml, XmlSafetyPolicy, validate_xml_input};#[cfg(any(target_os = "linux", target_os = "macos"))] fn peak_rss_bytes() -> u64 { use aster_forge_utils::numbers::i64_to_u64; // ... }Also applies to: 238-263
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_xml/benches/xml_memory.rs` at line 10, Update the i64_to_u64 import used by peak_rss_bytes to be available only on Linux and macOS, either by applying the matching cfg attribute or moving the import inside that cfg-gated function; keep the non-Unix implementation free of unused imports.
110-212: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value15 个 arm、15 次
print_result，复制粘贴的味道太冲。除了
forge_arena_original/xmltree_write需要重置测量窗口，其余 arm 的结构完全一致。建议让 match 只负责"跑负载并返回 snapshot"，print_result在函数末尾调用一次；漏写一次打印的机会也就没了。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_xml/benches/xml_memory.rs` around lines 110 - 212, 重构 run_child，使 match 各分支仅执行对应负载并返回测量结果快照，由函数末尾统一调用一次 print_result。保留 forge_arena_original 和 xmltree_write 在写出阶段重置测量窗口的特殊行为，并确保所有分支仍传递相同的 mode、fixture_name 和 input.len() 信息。crates/aster_forge_xml/fuzz/Cargo.toml (1)
10-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win给 fuzz crate 加一个独立编译任务。
libfuzzer-sys0.4.13 存在；但这个[workspace] members = ["."]的 fuzz crate 不会随父 workspace 一起构建，API 一变就会默默掉队。CI 加一句cargo build --manifest-path crates/aster_forge_xml/fuzz/Cargo.toml或cargo fuzz build。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_xml/fuzz/Cargo.toml` around lines 10 - 29, 为 fuzz crate 增加独立的 CI 编译任务，确保包含 parse_stream 和 writer_roundtrip 的清单能被实际构建。使用 crates/aster_forge_xml/fuzz/Cargo.toml 对应的 cargo build --manifest-path，或等价的 cargo fuzz build，并将其纳入现有 CI 流程。crates/aster_forge_xml/src/error.rs (1)
65-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Safety的 Display 转发 +source()同时返回自己，错误链会把同一句话说两遍。L68
error.fmt(f)已经把XmlSafetyError的文本原样吐出来了，L79 又把它作为source()暴露。任何按惯例遍历错误链输出的地方（anyhow、tracing的error.source()链、{:#}）都会打印成XML nesting depth exceeds the configured limit: XML nesting depth exceeds the configured limit。错误链的约定是 source 提供新增信息，转发型 Display 就不该再挂 source。
Io那条有XML I/O error:前缀，重复度低一些，但同样是重复内容，看你要不要一起收。♻️ 建议改动
impl std::error::Error for Error { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { - Self::Safety(error) => Some(error), Self::Io(error) => Some(error), - Self::InvalidXml(_) | Self::InvalidData(_) => None, + Self::Safety(_) | Self::InvalidXml(_) | Self::InvalidData(_) => None, } } }如果坚持要保留
Safety的 source（比如下游想downcast_ref::<XmlSafetyError>()），那就把 L68 改成带前缀的写法，别让两层文本一模一样。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_xml/src/error.rs` around lines 65 - 84, Update the Error implementations for the Safety variant so its Display output and source chain do not produce identical text: either stop exposing Safety through source(), or add a distinct contextual prefix in fmt::Display while preserving downcasting. Apply the same consistency decision to Io if needed, keeping InvalidXml and InvalidData behavior unchanged.crates/aster_forge_xml/src/document.rs (3)
992-1018: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
resolve_namespace+builder_value是 L263-279 / L246-254 的逐行复制，只为了Vec和Box<[]>的差别。两对函数的算法完全一致，唯一区别是 builder 阶段访问
Vec<NamespaceScope>/Vec<Box<str>>，document 阶段访问Box<[NamespaceScope]>/Box<[Box<str>]>——而Vec<T>和Box<[T]>都Deref<Target = [T]>，这个"差别"根本不需要两份代码。把 arena 四件套抽成一个私有结构体（持有
&[ArenaNode]/&[NamespaceScope]/&[NamespaceBinding]/&[Box<str>]的视图），让 builder 和XmlDocument都借它做查询，四个函数变两个。现在两份一致，改动便宜；等哪天只改了一边，build_element的前缀校验和ElementRef::namespace()就会对同一棵树给出不同答案。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_xml/src/document.rs` around lines 992 - 1018, Extract the duplicated namespace lookup and value-resolution logic from resolve_namespace and builder_value into one private arena-view structure holding slices of namespace scopes, bindings, and owned values. Make both the builder and XmlDocument pass their Vec or boxed-slice storage through this shared structure, relying on slice deref compatibility, and replace the four existing helper implementations with the shared two methods while preserving current lookup order and XML namespace handling.
246-254: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
stored_index把"内部状态坏了"翻译成"索引越界 panic"，而同一个函数里 UTF-8 失败又静默返回空串。同一份不变量，两种完全相反的失败模式。
stored_index的注释坦白了设计意图："make malformed internal state fail indexing"——也就是转换失败时返回usize::MAX，让后续的self.owned_values[usize::MAX]直接炸。而 L253 的from_utf8(...).unwrap_or("")遇到同一类不变量破坏却选择静默吐空串，上层拿到的是"这个元素没名字"而不是任何错误。另外 L251
let end = start + stored_index(value.length, ...)是裸加法：length一旦是usize::MAX，debug 下 overflow panic，release 下 wrap 成小数字，接着&source[start..end]因start > endpanic。三条路径三种炸法。这些
ValueRef目前全由本 crate 的 builder 构造、字段私有、外部无法伪造，所以当前不可达——我不是在说它今天会炸。我是在说 crate 顶上写着deny(clippy::panic)（lib.rsL12），转手就把 panic 当成内部错误处理机制，而且还只用了一半。要么统一成"内部不变量违反 → 返回Error/空值并且有 debug_assert"，要么统一成"直接 panic 且注释说明这是不可达"，别一半一半。resolve_namespace（L268、L270）和AttributeRef::data（L573）也是同一套裸索引。Also applies to: 1094-1098
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_xml/src/document.rs` around lines 246 - 254, 统一 ValueRef 内部状态异常的处理方式，重点更新 Document::value，并同步检查 resolve_namespace 与 AttributeRef::data 的裸索引和范围计算。避免 stored_index、UTF-8 转换及 start + length 分别产生 panic、静默空串和溢出；在现有返回类型约束下采用一致的安全失败结果，并用 debug_assert 标记不可达的不变量违规，确保无效范围不会继续切片或索引。
658-658: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
document.rs的闭标签校验依赖quick-xml默认配置，不如直接写在代码里。
check_end_names默认是开着的，但DocumentBuilder在只配置trim_text(false)的情况下就让Event::End(_) => self.end_element(event_end)直接丢了end_name；要修改开关或升级 quick-xml 语义时，这里跟parser.rs自带qualified_name比对的行为就只剩一个入口。要么显式设置reader.config_mut().check_end_names = true，要么像parser.rs那样自己比对闭标签名，别靠库默认值维持这条结构校验。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_xml/src/document.rs` at line 658, 在 DocumentBuilder 的 quick-xml reader 配置处显式启用闭标签名称校验，避免依赖默认配置；确保 Event::End 经过 end_element(event_end) 时仍保留结构校验行为，并与 parser.rs 的 qualified_name 比对逻辑保持一致。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aster_forge_xml/benches/support/mod.rs`:
- Around line 301-350: 统一处理 xmltree 对照 benchmark：优先将
crates/aster_forge_xml/benches/support/mod.rs#L301-L350 的
write_xmltree_multistatus、dav_element、xmltree_text_element 及依赖迁移到不属于默认 workspace
的独立 bench crate，并同步移除 manifest 与 lockfile 中的 xmltree；同时处理
crates/aster_forge_xml/benches/xml_bench.rs#L46-L66 的
xmltree_owned、validate_plus_xmltree，以及
crates/aster_forge_xml/benches/xml_memory.rs#L172-L187 的 xmltree_write、相关模式和
mode 列表。若不迁移，则必须在文档中明确记录仅 benchmark 保留 xmltree 的例外。
In `@crates/aster_forge_xml/benches/xml_bench.rs`:
- Around line 23-26: 统一 xml_bench.rs 与 xml_memory.rs 对 multistatus_10000 的
fixture 选择，避免一个 benchmark 过滤而另一个仍使用它；在 fixtures() 或两者共享的筛选逻辑处明确说明排除原因（默认
untrusted 上限或跑分耗时），并尽量让被排除的约 2.4 MB 字符串不再生成。
In `@crates/aster_forge_xml/benches/xml_memory.rs`:
- Around line 281-294: 统一 CPU 与内存 benchmark 对 multistatus_10000 的
XmlSafetyPolicy 口径：先核对默认 max_input_bytes/max_elements 及 XmlStreamWriter::new
的输出上限；若默认上限不足，则在 crates/aster_forge_xml/benches/xml_memory.rs#L281-L294
的相关模式传入放宽策略、在 crates/aster_forge_xml/benches/support/mod.rs#L217-L221 为
XmlStreamWriter 显式使用该策略，并在 crates/aster_forge_xml/benches/xml_bench.rs#L23-L26
保留过滤并补充原因注释；若上限足够，则移除该过滤并确保内存 benchmark 可稳定运行。
In `@crates/aster_forge_xml/Cargo.toml`:
- Around line 15-20: Remove the xmltree dependency from the [dev-dependencies]
section of the crate manifest, and eliminate any remaining xmltree references
from tests, benchmarks, and related configuration so Cargo.lock no longer
retains the package. Also inspect roxmltree usage and remove it if it is only
used for compatibility comparisons, preserving only dependencies required by
active tests or benchmarks.
In `@crates/aster_forge_xml/src/document.rs`:
- Around line 784-814: 在处理 start.attributes() 的循环中，使用现有的 validate_qualified_name
校验每个属性名，包括 xmlns 和 xmlns:* 声明；在根据 name 判断命名空间绑定之前完成校验，使非法前缀（如 1x）返回 Malformed，并与
parser.rs 的第一遍校验保持一致。
- Around line 445-447: Update the iteration logic in DescendantElements::next
and the corresponding traversal path around the second reported occurrence to
avoid collecting child_elements into a temporary Vec. Record the existing stack
length, extend self.stack directly with the child iterator, then reverse only
the newly appended slice in place so child order is preserved without an
allocation per visited node.
- Around line 903-935: The text_node function accumulates text_bytes after
trimming the value when trim_whitespace is true, but validate_xml_input counts
bytes before trimming. This causes inconsistent safety limit enforcement between
the two validation paths. Move the text_bytes accumulation (the checked_add call
and max_text_bytes comparison) to occur before the trim_whitespace conditional
block so that the byte limit is applied to the original input length rather than
the post-trim length, ensuring both code paths enforce the safety limit
consistently on raw input.
In `@crates/aster_forge_xml/src/parser.rs`:
- Around line 143-148: 统一 quick_xml 错误到 XmlSafetyError 的映射逻辑：将 document.rs 中的
map_quick_xml_error 提取到 crate 内共享位置，并让 validate_xml_input 的 read_event
错误路径复用该函数，而不是一律转换为 Error::InvalidXml。保留 safety_error 对已映射错误的处理，使非法编码输入在验证和 DOM
解析两个公共入口都返回 InvalidEncoding。
- Around line 400-463: Consolidate the duplicated XML safety helpers into one
new private module: in crates/aster_forge_xml/src/parser.rs lines 400-463,
remove the local implementations and import utf8, validate_qualified_name,
valid_name, validate_namespace_binding, and both namespace URI constants; in
crates/aster_forge_xml/src/document.rs lines 1021-1088, remove the duplicate
implementations and import the same shared symbols, moving document.rs’s
split_qualified_name into that module so validate_qualified_name reuses it. Also
move map_quick_xml_error into the shared module and use it at parser.rs lines
241-243. The four already-divergent behaviors require separate fixes and are not
addressed by this consolidation.
In `@crates/aster_forge_xml/src/stream.rs`:
- Around line 549-554: 更新 stream.rs 中处理 XmlStreamEvent::Comment 的分支，将
BytesText::new 替换为 BytesText::from_escaped，确保已解码的注释内容不会被二次转义，并保持原始 XML 注释内容不变。
In `@crates/aster_forge_xml/src/writer.rs`:
- Around line 433-451: Update attributes_share_expanded_name so duplicate
namespace declaration attributes are treated as conflicting: identical
declarations such as xmlns:a and xmlns:a, including duplicate default xmlns,
must return true regardless of their values. Preserve the existing expanded-name
comparison for non-declaration attributes. Extend the duplicate-attribute tests
in writer.rs to cover both prefixed and default namespace declarations.
- Around line 244-271: Update validated_subtree to prevent raw unprefixed
elements from being reinterpreted under the caller’s non-empty default
namespace. Before write_raw, detect whether the embedded subtree contains
unprefixed elements and whether the current writer scope has a non-empty default
namespace, then reject the operation with the appropriate invalid-data error (or
otherwise explicitly reset the namespace with xmlns=""); preserve the existing
depth and attribute validation.
In `@crates/aster_forge_xml/tests/writer.rs`:
- Line 45: 将 writer 测试中通过 xmltree::Element::parse 进行的输出校验移入
tests/xmltree_compat.rs 的 round-trip/parse contract 测试；更新 writer 测试以移除该校验及对
xmltree 的依赖，确保 xmltree 仅保留在兼容性测试中。
In `@docs/crates/aster_forge_xml.md`:
- Line 217: 将 benchmark 记录中的日期从 `2026-07-25` 更新为实际采样日期；若这些结果确实采集于 2026 年 7 月 24
日，则改为 `2026-07-24`，并同步 PR 说明中的报告日期，保持其余 workload-specific benchmark 数据不变。
---
Nitpick comments:
In `@crates/aster_forge_xml/benches/support/mod.rs`:
- Around line 150-215: Extract the duplicated XmlSafetyPolicy construction from
walk_forge_stream and validate_forge_stream into a shared helper, then have both
functions obtain their reader policy through that helper. Preserve the existing
input-dependent limits and relaxed max_elements/max_text_bytes/max_events values
so both benchmark paths remain comparable.
- Around line 115-122: 在基准测试的 Event::Text 分支中移除对 unescape 的调用，使 checksum 直接使用
text.decode() 的结果长度；同时删除顶部对 quick_xml::escape::unescape
的导入，保留现有有效性断言和其他事件处理逻辑不变。
In `@crates/aster_forge_xml/benches/xml_memory.rs`:
- Around line 295-315: Update both child-process success assertions in the
benchmark loops to include the captured output.stderr when the command fails,
while preserving the existing success check and stdout printing behavior.
- Line 10: Update the i64_to_u64 import used by peak_rss_bytes to be available
only on Linux and macOS, either by applying the matching cfg attribute or moving
the import inside that cfg-gated function; keep the non-Unix implementation free
of unused imports.
- Around line 110-212: 重构 run_child，使 match 各分支仅执行对应负载并返回测量结果快照，由函数末尾统一调用一次
print_result。保留 forge_arena_original 和 xmltree_write
在写出阶段重置测量窗口的特殊行为，并确保所有分支仍传递相同的 mode、fixture_name 和 input.len() 信息。
In `@crates/aster_forge_xml/fuzz/Cargo.toml`:
- Around line 10-29: 为 fuzz crate 增加独立的 CI 编译任务，确保包含 parse_stream 和
writer_roundtrip 的清单能被实际构建。使用 crates/aster_forge_xml/fuzz/Cargo.toml 对应的 cargo
build --manifest-path，或等价的 cargo fuzz build，并将其纳入现有 CI 流程。
In `@crates/aster_forge_xml/src/document.rs`:
- Around line 992-1018: Extract the duplicated namespace lookup and
value-resolution logic from resolve_namespace and builder_value into one private
arena-view structure holding slices of namespace scopes, bindings, and owned
values. Make both the builder and XmlDocument pass their Vec or boxed-slice
storage through this shared structure, relying on slice deref compatibility, and
replace the four existing helper implementations with the shared two methods
while preserving current lookup order and XML namespace handling.
- Around line 246-254: 统一 ValueRef 内部状态异常的处理方式，重点更新 Document::value，并同步检查
resolve_namespace 与 AttributeRef::data 的裸索引和范围计算。避免 stored_index、UTF-8 转换及 start
+ length 分别产生 panic、静默空串和溢出；在现有返回类型约束下采用一致的安全失败结果，并用 debug_assert
标记不可达的不变量违规，确保无效范围不会继续切片或索引。
- Line 658: 在 DocumentBuilder 的 quick-xml reader 配置处显式启用闭标签名称校验，避免依赖默认配置；确保
Event::End 经过 end_element(event_end) 时仍保留结构校验行为，并与 parser.rs 的 qualified_name
比对逻辑保持一致。
In `@crates/aster_forge_xml/src/error.rs`:
- Around line 65-84: Update the Error implementations for the Safety variant so
its Display output and source chain do not produce identical text: either stop
exposing Safety through source(), or add a distinct contextual prefix in
fmt::Display while preserving downcasting. Apply the same consistency decision
to Io if needed, keeping InvalidXml and InvalidData behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 43366832-dfa8-42ae-a4b9-1533a173f1fb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
README.mdcrates/aster_forge_utils/Cargo.tomlcrates/aster_forge_utils/src/lib.rscrates/aster_forge_utils/src/xml.rscrates/aster_forge_xml/Cargo.tomlcrates/aster_forge_xml/benches/support/mod.rscrates/aster_forge_xml/benches/xml_bench.rscrates/aster_forge_xml/benches/xml_memory.rscrates/aster_forge_xml/fuzz/.gitignorecrates/aster_forge_xml/fuzz/Cargo.tomlcrates/aster_forge_xml/fuzz/fuzz_targets/parse_stream.rscrates/aster_forge_xml/fuzz/fuzz_targets/writer_roundtrip.rscrates/aster_forge_xml/src/document.rscrates/aster_forge_xml/src/error.rscrates/aster_forge_xml/src/lib.rscrates/aster_forge_xml/src/parser.rscrates/aster_forge_xml/src/stream.rscrates/aster_forge_xml/src/writer.rscrates/aster_forge_xml/tests/document.rscrates/aster_forge_xml/tests/property.proptest-regressionscrates/aster_forge_xml/tests/property.rscrates/aster_forge_xml/tests/roxmltree_compat.rscrates/aster_forge_xml/tests/stream.rscrates/aster_forge_xml/tests/writer.rscrates/aster_forge_xml/tests/xml.rscrates/aster_forge_xml/tests/xmltree_compat.rsdocs/.vitepress/config.tsdocs/crates/aster_forge_utils.mddocs/crates/aster_forge_xml.mddocs/guide/new-project-integration.mddocs/index.md
💤 Files with no reviewable changes (3)
- crates/aster_forge_utils/Cargo.toml
- crates/aster_forge_utils/src/lib.rs
- crates/aster_forge_utils/src/xml.rs
feat(aster_forge_xml): extract syntax validation module and improve parser robustness **Core improvements:** - Extract shared XML syntax validation functions into new `syntax` module (utf8, validate_qualified_name, split_qualified_name, validate_namespace_binding, map_quick_xml_error) - Add `ArenaView` abstraction for safe internal value and namespace resolution with bounds checking - Enable `check_end_names` in quick-xml reader configuration for stricter tag matching - Validate qualified names during attribute parsing in arena builder - Count text bytes before trimming to enforce limits on original input size **Memory benchmark refinements:** - Split benchmark fixtures into `cpu_fixtures()` and `memory_fixtures()` to avoid inflating multi-sample benchmark setup with large 10k-response documents - Improve child process error reporting with stderr output on failure - Factor out common `stream_policy()` helper for benchmark reader configuration **Robustness and correctness:** - Add bounds checking to `AttributeRef::data()` and namespace resolution paths - Fix `DescendantElements` iterator to avoid intermediate allocation - Preserve comment escaping in stream capture using `BytesText::from_escaped` - Reject validated subtrees with unprefixed elements when writer has default namespace - Fix duplicate attribute detection to handle xmlns declarations correctly - Add error display test coverage and improve `Error::Safety` formatting **Testing:** - Add test for descendant iteration document order - Add test for arena value bounds checking - Add test for invalid encoding classification consistency - Add test for trim whitespace byte limit enforcement - Add test for validated subtree namespace inheritance
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/rust.yml:
- Line 9: Update the Rust workflow’s paths configuration to include the fuzz
crate’s source directory and harness files, including
crates/aster_forge_xml/fuzz/fuzz_targets/*.rs, alongside its Cargo.toml entry so
harness changes trigger CI.
- Around line 53-54: Update the “Build XML fuzz harness” workflow step to build
with the nightly Rust toolchain, installing or selecting nightly as needed, and
invoke the existing Cargo manifest build through cargo +nightly so libFuzzer
sanitizer flags are supported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 644c51c3-fbd5-4975-8e36-fe470989556c
📒 Files selected for processing (15)
.github/workflows/rust.ymlcrates/aster_forge_xml/benches/support/mod.rscrates/aster_forge_xml/benches/xml_bench.rscrates/aster_forge_xml/benches/xml_memory.rscrates/aster_forge_xml/src/document.rscrates/aster_forge_xml/src/error.rscrates/aster_forge_xml/src/lib.rscrates/aster_forge_xml/src/parser.rscrates/aster_forge_xml/src/stream.rscrates/aster_forge_xml/src/syntax.rscrates/aster_forge_xml/src/writer.rscrates/aster_forge_xml/tests/document.rscrates/aster_forge_xml/tests/stream.rscrates/aster_forge_xml/tests/writer.rscrates/aster_forge_xml/tests/xml.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/aster_forge_xml/benches/xml_bench.rs
- crates/aster_forge_xml/benches/xml_memory.rs
- crates/aster_forge_xml/src/error.rs
- crates/aster_forge_xml/tests/stream.rs
- crates/aster_forge_xml/tests/writer.rs
- crates/aster_forge_xml/tests/xml.rs
- crates/aster_forge_xml/src/parser.rs
- crates/aster_forge_xml/src/stream.rs
- crates/aster_forge_xml/src/document.rs
- crates/aster_forge_xml/src/writer.rs
| - '**/*.rs' | ||
| - 'Cargo.toml' | ||
| - 'Cargo.lock' | ||
| - 'crates/aster_forge_xml/fuzz/Cargo.toml' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -C3 'paths:|aster_forge_xml|fuzz_targets' .github/workflows/rust.ymlRepository: AsterCommunity/AsterForge
Length of output: 988
别把 fuzz harness 源码变更漏网。
当前 paths 只覆盖 crates/aster_forge_xml/fuzz/Cargo.toml；像 crates/aster_forge_xml/fuzz/fuzz_targets/*.rs 这样的 harness 源码修改不会触发 CI。把 fuzz crate 的源文件/目录一起纳入 paths。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/rust.yml at line 9, Update the Rust workflow’s paths
configuration to include the fuzz crate’s source directory and harness files,
including crates/aster_forge_xml/fuzz/fuzz_targets/*.rs, alongside its
Cargo.toml entry so harness changes trigger CI.
- Install nightly Rust toolchain with minimal profile before fuzzing - Use `cargo +nightly` to build XML fuzz harness explicitly on nightly - Ensures fuzz targets compile with required nightly features
AptS-1547
added
documentation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概述
实现新的
aster-forge-xmlcrate，用高性能的quick-xml替代性能较差的xmltree。变更内容
crates/aster-forge-xml/目录相关 Issue
Closes AsterCommunity/AsterDrive#433
测试
cargo test全部通过Summary by CodeRabbit
aster_forge_xml：基于安全策略的 XML 校验、命名空间感知的流式读取与受限流式写入。