fix: close UFO round-trip gaps from the preservation-contract re-review#130
Open
kostyafarber wants to merge 3 commits into
Open
fix: close UFO round-trip gaps from the preservation-contract re-review#130kostyafarber wants to merge 3 commits into
kostyafarber wants to merge 3 commits into
Conversation
Atomic UFO and designspace writes operated on the symlink path itself, so the rename-exchange replaced the link with a real directory or file and orphaned the linked location. Existing targets are now canonicalized before staging, so the write lands at the resolved path and the link survives.
…keys Two UFO round-trip gaps: glyph-level <image> placement elements and <note> were dropped when the writer regenerated glyphs (orphaning the preserved image bytes), and any non-spec fontinfo.plist key failed the whole load via norad's deny_unknown_fields. The glif image reference and note now ride in the layer lib passthrough as opaque records under com.shift-editor.preserved.* keys; the writer pops them back out into the written glif, so they survive UFO, .shift, and working-store round-trips without being modeled. fontinfo.plist is partitioned into norad-known keys and an unknown remainder before norad sees it: unknown keys join the fontinfo remainder passthrough (loading through a symlinked shadow directory when needed) and are merged back into the staged fontinfo.plist after norad writes it.
Source names that could never be written as UFO layer names (control characters, the reserved public.default) are now rejected when the source is created, and a fontinfo remainder that norad cannot serialize fails at package open / store resume with a clear UnsavableFontState error. The save-time checks remain as backstops.
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
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.
Fixes four confirmed gaps found by the adversarial re-review of #119 (UFO preservation contract) and #117 (file I/O safety).
1. Glyph
<image>references and notes were destroyed on round-tripThe contract carried image bytes (
images/) but the writer regenerated every glif without its<image>placement element or<note>, orphaning the preserved bytes. The glif image reference (fileName, six transform numbers, optional color) and note now ride in the layer lib passthrough as opaque records undercom.shift-editor.preserved.*keys; the writer pops them back out into the written glif. Nothing is modeled — it is a round-trip blob (promotion to a real reference-image feature is deliberately out of scope, see #113), and it survives UFO,.shift, and working-store round-trips through the existing lib plumbing with zero schema changes.2. One unknown fontinfo key bricked the whole UFO load
norad deserializes fontinfo with
deny_unknown_fields, so any non-spec key infontinfo.plistfailedFont::loadoutright.fontinfo.plistis now pre-parsed alone (no double-parse of the rest of the UFO) and partitioned into norad-known keys and an unknown remainder via per-key probes. When unknown keys exist, norad loads through a temp shadow directory that symlinks every UFO entry and substitutes the sanitized fontinfo; the unknown keys join the fontinfo remainder passthrough and are merged back into the stagedfontinfo.plistafter norad writes it. A norad-known key with an unparseable value stays a hard error, matching norad's own verdict.3. Saving through a symlinked UFO destroyed the symlink
The staging + rename-exchange path operated on the link inode, replacing the symlink with a real directory. Existing targets are now canonicalized before staging/exchange in both
UfoWriter::write_atomicandwrite_file_atomic(covering designspace XML and every companion UFO in multi-UFO saves), so the write lands at the resolved path and the link survives.4. Save-only failures moved to creation time
public.default) are rejected when the source is created (CoreError::InvalidSourceName), not when save hits norad'sNamevalidation..shiftpackage or working store) now fails atopen/resumewithWorkspaceError::UnsavableFontStatenaming the offending key, instead of only failing the next save. The save-time checks remain as backstops.Tests
preserves_glyph_image_reference_note_and_unknown_fontinfo_keys— fixture UFO with an<image>element (color + non-identity transform), a glyph note, and unknown fontinfo keys (string + nested dict): loads, writes the element/note/keys back verbatim, leaks nocom.shift-editor.preserved.*keys into the written glif lib, reloads identically.save_through_symlink_updates_target_and_keeps_linkandwriting_through_symlink_updates_target_and_keeps_link(#[cfg(unix)]).create_source_rejects_names_unwritable_as_ufo_layers,tampered_package_fontinfo_remainder_fails_at_open,tampered_store_fontinfo_remainder_fails_at_resume.cargo test --workspacegreen (all crates),cargo fmtclean,cargo clippy --workspace --all-targetszero warnings. Out of scope per review triage: point identifiers, objectLibs, staging-dir sweep, EINVAL rename fallback.🤖 Generated with Claude Code