feat: add SXSD schema validation to Slides lint - #2103
Conversation
- validate SXSD attributes, values, and child structures - preserve server-filled readback fields - isolate SXSD failures by slide - keep valid slides eligible for layout checks
📝 WalkthroughWalkthroughExtends SXSD validation with length facets, strict numeric lexical checks, namespace handling, sanitized document validation, slide-scoped issue reporting, normalized layout parsing, improved hints, and expanded CLI, schema, geometry, and density tests. ChangesSXSD validation and lint integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@626cf3eb8749ca20e205f4954d4ff1a945aad247🧩 Skill updatenpx skills add larksuite/cli#feat/slides-sxsd-lint -y -g |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2103 +/- ##
==========================================
+ Coverage 75.16% 75.19% +0.02%
==========================================
Files 912 914 +2
Lines 96475 96789 +314
==========================================
+ Hits 72517 72778 +261
- Misses 18381 18413 +32
- Partials 5577 5598 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
skills/lark-slides/scripts/sxsd_validator.py (1)
152-157: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInline attribute type keys depend on
id()of live ET elements. Both sites mint/recompute__inline_attribute_{name}_{id(attribute)}, so the producer and consumer only agree while everyxs:attributeelement remains reachable; unreachable ones can be collected and theirid()reused, mapping an attribute to a different simple type. Replace the identity component with a deterministic one on both sides.
skills/lark-slides/scripts/sxsd_validator.py#L152-L157: mint the key from stable data (owner complexType/element name plus attribute name, or a monotonically increasing counter recorded on the model).skills/lark-slides/scripts/sxsd_validator.py#L213-L216: derive the lookup key with the exact same deterministic scheme instead ofid(attribute).🤖 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 `@skills/lark-slides/scripts/sxsd_validator.py` around lines 152 - 157, Replace the id(attribute)-based key generation in both the inline attribute type producer at lines 152-157 and its lookup at lines 213-216 with one shared deterministic scheme, such as owner type/element name plus attribute name. Ensure both sites derive exactly the same key and preserve correct mapping even after XML elements become unreachable.skills/lark-slides/scripts/xml_text_overlap_lint_test.py (1)
2793-2793: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFractional
fontSizewas edited out of the fixture without covering the new constraint.Changing
fontSize="12.4"to"12"implies the schema pass now rejects fractional font sizes, but nothing asserts that. Add a small case pinning the expected code (sxsd_invalid_scalar/sxsd_value_out_of_range) forfontSize="12.4", otherwise a future loosening or tightening of that facet goes unnoticed.🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py` at line 2793, Add a focused test in the fixture validation tests covering a fractional fontSize value of 12.4, and assert the expected schema error code (sxsd_invalid_scalar or sxsd_value_out_of_range). Keep the existing integer fontSize fixture unchanged and ensure the test fails if fractional font sizes are later accepted or rejected with an unexpected code.
🤖 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 `@skills/lark-slides/scripts/sxsd_validator.py`:
- Around line 99-104: Update the xs:union handling in the schema parser to
collect nested xs:simpleType children as union members, including their inline
pattern constraints, while retaining declared memberTypes. Resolve local schema
type names through the existing type-resolution path so both named and anonymous
union members are included in validation; ensure BulletSizeType no longer
produces an empty union_members tuple.
- Around line 502-512: Update xsd_pattern_matches to catch ValueError from
python_pattern_for_xsd and re.error from re.fullmatch, returning True so an
untranslatable or invalid schema pattern does not abort validate_sxsd. Apply
matching with a bounded or otherwise non-backtracking approach for non-hardcoded
patterns as well, preserving the existing optimized handling for [\w.-]+[.:]\S*.
In `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py`:
- Around line 3246-3256: Update test_rejects_non_xsd_whitespace_in_color_pattern
so the intentional non-breaking space in the color fixture is represented with
an explicit escape rather than a literal Unicode character. Preserve the test’s
malformed-color input and sxsd_pattern_mismatch assertion.
---
Nitpick comments:
In `@skills/lark-slides/scripts/sxsd_validator.py`:
- Around line 152-157: Replace the id(attribute)-based key generation in both
the inline attribute type producer at lines 152-157 and its lookup at lines
213-216 with one shared deterministic scheme, such as owner type/element name
plus attribute name. Ensure both sites derive exactly the same key and preserve
correct mapping even after XML elements become unreachable.
In `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py`:
- Line 2793: Add a focused test in the fixture validation tests covering a
fractional fontSize value of 12.4, and assert the expected schema error code
(sxsd_invalid_scalar or sxsd_value_out_of_range). Keep the existing integer
fontSize fixture unchanged and ensure the test fails if fractional font sizes
are later accepted or rejected with an unexpected code.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 237b452e-6f35-47cc-a163-663e1e5c22f5
📒 Files selected for processing (3)
skills/lark-slides/scripts/sxsd_validator.pyskills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
skills/lark-slides/scripts/xml_text_overlap_lint_test.py (1)
3436-3471: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHint assertion is coincidental, not a real check of the boilerplate text.
self.assertIn("unsupported", str(issues[0]["hint"]).lower())only passes because the fixture's own type is namedUnsupportedPatternType, which gets echoed into the hint viaf"Extend the SXSD pattern interpreter for {attr_rule.type_name}; ...". The actual boilerplate hint text produced byvalidate_element_attributesforsxsd_unsupported_patternnever contains the word "unsupported" itself — so this assertion wouldn't catch a regression in the hint wording, and would pass even if the hint became meaningless, as long as the schema fixture keeps that type name.♻️ Assert against wording that's independent of the fixture's type name
- self.assertIn("unsupported", str(issues[0]["hint"]).lower()) + self.assertIn("pattern interpreter", str(issues[0]["hint"]).lower())🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py` around lines 3436 - 3471, Update test_reports_unsupported_xsd_pattern_without_crashing so its hint assertion verifies stable boilerplate wording from the sxsd_unsupported_pattern diagnostic rather than the fixture-dependent type name. Keep the existing code and attribute assertions, and assert a meaningful expected phrase that validate_element_attributes always emits.
🤖 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.
Nitpick comments:
In `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py`:
- Around line 3436-3471: Update
test_reports_unsupported_xsd_pattern_without_crashing so its hint assertion
verifies stable boilerplate wording from the sxsd_unsupported_pattern diagnostic
rather than the fixture-dependent type name. Keep the existing code and
attribute assertions, and assert a meaningful expected phrase that
validate_element_attributes always emits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be7841be-d01e-48bf-8d22-c767d1a159c2
📒 Files selected for processing (2)
skills/lark-slides/scripts/sxsd_validator.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/lark-slides/scripts/xml_text_overlap_lint.py (1)
400-503: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftSuggestion-based suppression can silently drop real
sxsd_missing_required_attr/sxsd_unexpected_childerrors on unrelated sibling elements.
suggested_attr_candidates/missing_attrs_by_location/unsupported_tag_locationsare all keyed by(path, tag)(no sibling index), same assxsd_validator.py's own path scheme. Since virtually every slide has multiple<shape>(or other) siblings computing an identical path, a typo on one element can suppress a completely unrelated, genuinely-missing-attribute error on another element with the same tag. Example:<shape type="text" topLeftXX="80" topLeftY="80" width="300" height="60"/> <!-- typo --> <shape type="text" topLeftY="80" width="300" height="60"/> <!-- genuinely missing topLeftX -->Both compute path
slide/data/shape. Shape 1's typo suggeststopLeftX; the schema validator reportstopLeftXmissing on both shapes (identical(path, tag, attr)triple); the ambiguity-resolution logic treats this as "exactly one candidate resolves the ambiguity" and suppresses every matching schema issue — including shape 2's real, unrelated error.A correct fix needs per-element correlation (hard, since
_validate_sxsd_schema_constraintsruns the schema validator on a separately deep-copied tree). A cheap, safe mitigation is to only allow suppression when the(path, tag)location is unique in the document:🛡️ Guard suppression behind a uniqueness check
def validate_sxsd_document(xml: str, root: ET.Element) -> list[dict[str, Any]]: tag_attributes = load_sxsd_tag_attributes() supported_tags = set(tag_attributes) issues: list[dict[str, Any]] = [] suggested_attr_candidates: dict[tuple[str, str], list[set[str]]] = {} + location_occurrence_counts: dict[tuple[str, str], int] = {} def visit(element: ET.Element, ancestors: list[str], path: str) -> None: if should_skip_sxsd_subtree(element, ancestors): return tag_name = xml_local_name(element.tag) current_path = f"{path}/{tag_name}" if path else tag_name + location_occurrence_counts[(current_path, tag_name)] = ( + location_occurrence_counts.get((current_path, tag_name), 0) + 1 + ) parent_name = ancestors[-1] if ancestors else None ... suggested_attrs: set[tuple[str, str, str]] = set() for location, candidate_groups in suggested_attr_candidates.items(): + if location_occurrence_counts.get(location, 0) != 1: + continue # multiple elements share this tag/path; don't cross-suppress missing_attrs = missing_attrs_by_location.get(location, set()) ...The same non-uniqueness applies to the
sxsd_unexpected_childsuppression viaunsupported_tag_locations. Please also add a regression test with two same-tag siblings (one with a typo, one genuinely missing a different/unrelated required attribute) to lock this in.🤖 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 `@skills/lark-slides/scripts/xml_text_overlap_lint.py` around lines 400 - 503, Prevent suggestion-based suppression in validate_sxsd_document from applying to ambiguous repeated elements: track the number of elements for each (path, tag) location during traversal and only suppress suggested missing-attribute or unexpected-child schema issues when that location occurs exactly once. Apply the same uniqueness guard to suggested_attrs and unsupported_tag_locations, and add a regression test with same-tag siblings where one has a typo and another is genuinely missing a required attribute, preserving both errors.
🤖 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.
Outside diff comments:
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 400-503: Prevent suggestion-based suppression in
validate_sxsd_document from applying to ambiguous repeated elements: track the
number of elements for each (path, tag) location during traversal and only
suppress suggested missing-attribute or unexpected-child schema issues when that
location occurs exactly once. Apply the same uniqueness guard to suggested_attrs
and unsupported_tag_locations, and add a regression test with same-tag siblings
where one has a typo and another is genuinely missing a required attribute,
preserving both errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f5ce725-cb88-435f-ade0-37ad5e71fa13
📒 Files selected for processing (3)
skills/lark-slides/scripts/sxsd_validator.pyskills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
3 participants
Summary
Add self-contained SXSD schema validation to the Slides XML lint flow. The validator now reports structured and actionable syntax errors while preserving compatibility with XML returned by the Slides service.
Changes
<slide>fragments and valid Slides namespaces:http://www.larkoffice.com/sml/2.0/sml/2.0https://www.larkoffice.com/sml/2.0Test Plan
python3 -m unittest xml_text_overlap_lint_test.pyQ8NSsDHfFlL2ladrkmOc2YnEnJcslides +xml-get → xml_text_overlap_lint.pyerror_count=0,release_ready=trueRelated Issues
Summary by CodeRabbit
New Features
minLength/maxLengthfor simple types and stricterdouble/decimallexical checking.Bug Fixes
inf,-inf,nan) as SXSD invalid values.Tests