Repository navigation
Ignore signed \'hh hex escapes in RTF outline import - #1351
h-a-n-a-s-h-i merged 1 commit into
Conversation
Found while testing crates/format/src/rtf.rs:226 (rtf_to_outline) Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Validate RTF hex escapes crates/format/src/rtf.rs |
The parser requires two ASCII hexadecimal digits for an escape. A regression test checks that a signed escape is ignored, yielding ab\n. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Possibly related PRs
- storytold/deckcraft#1260: Adds the RTF outline reader that this change updates.
Suggested reviewers: nikitkara
Merge Risk: 
9c985
-
The change is narrowly scoped and does not establish a merge-blocking risk.
Architecture risk: 
9c985
-
The change affects 1 system.
Changed systems:
cratesArchitecture concerns
No architecture-level concerns identified.Review details
-
Systems and components
- observed — crates (service) was modified; 1 changed file maps to changed impact.
Before / after behavior
- observed — Modified behavior in crates/format/src/rtf.rs: Hex escapes now require two ASCII hexadecimal digits before parsing; malformed escapes containing signs or other non-hex characters are ignored.
- observed — Modified behavior in crates/format/src/rtf.rs: Added a test asserting that a signed value in a
\'hhescape is ignored, yieldingab\n.
Reliability and maintainability
- inferred — Risk-relevant change factors for crates: blast_radius_1; direct_dependents_1
-
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: ignoring signed \'hh hex escapes during RTF outline import. Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
Landed on main — thank you for the contribution! If you haven't already, come join us on Discord: https://discord.gg/artcraft |
Found while testing
crates/format/src/rtf.rs:226 (rtf_to_outline)(no existing issue): RTF outline import accepts a '+' sign in 'hh hex escapes and inserts a control character.Cause
The RTF reader decoded the two characters after
\'withu8::from_str_radix(h, 16), which accepts a leading+. So\'+5became byte 0x05 and a control character was added to the slide text.Fix
Before parsing, the reader now checks that both characters after
\'are ASCII hex digits. Any other pair is ignored like other malformed escapes.Verified
rtf::tests::hex_escape_rejects_sign: fails onmainwithout the fix and passes with it (cargo test -p deckcraft-format --lib hex_escape_rejects_sign).cargo xtask cipasses.🤖 Generated with AI by ApolloOffice, see the Co-Authored-By trailers for the models
Summary by CodeRabbit
\'+5, are ignored rather than interpreted as signed numbers. Valid hexadecimal escapes continue to be supported, helping prevent malformed input from producing unintended character values.