🐛 Validate anchors and aliases on the composer paths too#194
Conversation
The fast decoder rejects three malformed anchor/alias constructs; the round-trip composer (which also backs `OPT_ROUND_TRIP` and `OPT_INCLUDES`) did not, so the same document was a hard error on the fast and annotated paths but silently accepted on the composer paths: - an unknown or forward `*alias` (`b: *ghost`) resolved to a silent `None` instead of erroring, so a typo in an alias name became data loss; - an alias carrying its own anchor or tag (`&b *a`, `!!str *a`) was accepted; - a node with two anchors (`&a\n&b value`) was accepted. The composer now enforces all three, matching the fast decoder and PyYAML. It tracks the anchors defined so far in the current document (recording each only after its children compose, so a self-reference is still an unknown reference) and clears that set at every document boundary, so an anchor cannot leak across documents either. Valid aliases are unaffected; all load paths now agree.
frenck
added
the
bugfix
|
Warning Review limit reached
Next review available in: 55 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: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #194 +/- ##
==========================================
+ Coverage 92.44% 92.57% +0.12%
==========================================
Files 40 40
Lines 11872 11981 +109
==========================================
+ Hits 10975 11091 +116
+ Misses 897 890 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR closes a cross-path consistency gap in anchor/alias validation. The fast decoder (loads/dumps default path) already rejects three malformed constructs — an unknown/forward *alias, an alias carrying its own &anchor/!tag, and a node with two anchors — but the round-trip composer (backing OPT_ROUND_TRIP, OPT_INCLUDES, the annotated multi-document path, and schema validation) silently accepted them, resolving an undefined alias to None. The composer now enforces the same rules by tracking anchors seen so far in the current document (anchors_seen), recording each anchor only after its children compose (so a self-reference is still unknown) and clearing the set at every document boundary. This is an intentional breaking change: documents that were already invalid now error consistently across all load paths, matching the fast decoder and PyYAML.
Changes:
- Add
anchors_seentoComposer, reject unknown/forward aliases, aliases with an anchor/tag, and duplicate anchors, clearing anchor scope at each document start. - Update the
cyclic_anchor_terminates_via_depth_capunit test to build the cyclic AST by hand (compose now rejects it) and assert the composer refuses the document form. - Replace the obsolete "delete unrelated key with preexisting dangling alias" test with cross-load-path tests for unknown aliases and malformed anchor/tag cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/roundtrip/composer.rs |
Adds anchors_seen tracking and the three validation checks, mirroring the fast decoder; clears anchor scope at document boundaries. |
src/roundtrip/anchors.rs |
Rewrites the cyclic-anchor test to construct the cycle directly (since composing it now errors) and asserts the composer rejects the document form. |
tests/core/test_anchors.py |
Removes the now-impossible dangling-alias delete test; adds cross-path tests for unknown aliases and malformed anchor/tag constructs. |
I verified parity with the fast decoder (src/decode/mod.rs): the two-anchor, alias-with-anchor/tag, unknown-alias, and cross-document clearing logic all match, HashSet is imported, mapping keys and values both flow through compose_node (so key anchors are recorded), the empty-anchored-node case still records its anchor, error messages surface correctly on all paths (including includes via a located YAMLRocksParseError), and the reworked Rust test compiles against the current Span/ScalarStyle. No issues found.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Breaking change
Documents that were already invalid now raise on the round-trip and includes paths instead of being silently accepted (an undefined
*aliasused to resolve toNonethere; a malformed anchor/tag was accepted). No valid document changes behavior. This aligns the composer paths with the fast/annotated paths and PyYAML, so it removes an inconsistency rather than adding one.Proposed change
The fast decoder rejects three malformed anchor/alias constructs; the round-trip composer (which backs
OPT_ROUND_TRIPandOPT_INCLUDES) did not, so the same document was a hard error on the fast and annotated paths but silently accepted on the composer paths:*alias(b: *ghost) resolved to a silentNoneinstead of erroring, so a typo in an alias name became data loss;&b *a,!!str *a) was accepted;&a\n&b value) was accepted.The composer now enforces all three, matching the fast decoder and PyYAML. It tracks the anchors defined so far in the current document (recording each only after its children compose, so a self-reference is still an unknown reference) and clears that set at every document boundary, so an anchor cannot leak across documents either. Valid aliases are unaffected; all load paths now agree.
Found by a differential cross-path consistency sweep.
Type of change
Additional information
Checklist
uv run pytestpasses locally. A pull request cannot be merged unless CI is green.uv run ruff check .anduv run ruff format --check .pass.cargo fmt --checkandcargo clippy --all-targets -- -D warningspass.If the change is user-facing:
docs/is added or updated, anddocs/verify_examples.pystill passes.