Three defects found by auditing the ADR merge criteria against the code
rather than against memory.
1. The scheduled fuzz job ran five hardcoded targets. Four had been
added since and were never fuzzed: parse_revision_chain, decrypt,
eval_function and parse_colorspace. eval_function is the sharpest of
those - it executes PostScript taken verbatim from an untrusted file.
The list now comes from `cargo fuzz list` and the job fails rather
than passing vacuously if it comes back empty.
`cargo fuzz list` reads the manifest, so a target file added without
its [[bin]] entry would still be skipped silently. The engine job,
which runs on every push, now checks the two agree. Both directions
of that guard were exercised before committing.
2. ADR 0004 rule 3 promises an annotation whose appearance cannot be
generated "keeps its original /AP and is reported as skipped". The
keeping worked - to_dict clones the source dictionary - but nothing
reported it: SaveReport only tracked skipped appearances for form
fields. A caller who moved a stamp was never told its artwork still
showed the old position. Adds
SaveReport::annotation_appearances_skipped and
appearance_is_generated, which enumerates the out-of-scope types
explicitly so a new AnnotationType fails to compile until classified.
3. SetContents and SetFlags had no round-trip test. Both were
implemented and unit-tested against the in-memory model, but neither
was ever reparsed from written bytes - the assertion ADR 0004 calls
central.
New fixture annotations/stamp.pdf carries real /AP artwork for a Stamp
(undrawable: must be preserved and reported) beside a Square (drawable:
must not be reported), so the reporting cannot pass by reporting
everything. The stamp test was mutation-checked: it fails when the
reporting line is removed.
ADR 0003 and 0004 merge criteria are now ticked. 0003's were genuine
paperwork - every box traced to an existing named test. 0004's were not,
and its ADR now records what was missing rather than implying it always
worked.
TEST_TARGET=pdf 447 -> 451 passing. rustfmt and clippy -D warnings clean.
Phase 8 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md, designed in
REVIEWS/adr/0003-pdf-incremental-save.md.
The review treats Phase 8 as ten independent projects, each needing its own
design doc and merge criteria. Incremental save is taken first because it is
the only one whose prerequisites are already met, it is listed as a
prerequisite by two others (annotation editing and full AcroForm support),
and it closes a real credibility gap: DocumentFormEditor has been able to
edit form fields since Phase 3, and there was no way to save the result.
A grep for a public save API across all four crates returned nothing.
Design decision: append a revision, never rewrite. The original bytes are
copied verbatim and changed objects are appended with a new xref chained
through /Prev. A full rewrite would be easier and wrong: it would silently
discard everything this parser does not yet model (structure trees, optional
content, embedded files), and it would invalidate any signature, foreclosing
a feature listed later in the same phase. ADR 0003 records this in full.
Two latent bugs surfaced while building it, both pre-existing:
- find_xref_start searched with windows(10) for the 9-byte keyword
"startxref", so it never matched. Every parse silently fell through to a
forward scan for the first "xref" in the file. On a single-revision
document that happens to be correct; on an incrementally saved one it is
the *oldest* revision, so a saved edit read back as its pre-edit value.
This had no visible effect before because nothing produced multi-revision
files.
- XRefTable::parse read one section and ignored /Prev entirely, so a
multi-revision document lost every object the earlier revisions defined.
It now walks the chain newest-first, keeping the first definition of each
object, with a visited set against /Prev loops and bounds checks on the
offsets, which come from the file and cannot be trusted. A bad link ends
the chain instead of indexing out of bounds.
The xref unit fixture claimed startxref 408 in a 191-byte file and only ever
passed because of the windows(10) defect; it is corrected rather than
adjusted to keep passing.
Implementation:
- pdf-cos/src/incremental.rs: IncrementalUpdate builds one revision.
Recomputes stream /Length so a caller cannot write an inconsistent one,
emits xref subsections for contiguous runs, sizes /Size over the whole
chain, and is byte-reproducible for a given set of edits.
- pdf-document/src/save.rs: turns dirty AcroForm fields into a revision,
writing the new /V and a regenerated appearance stream referenced from
/AP, keyed by state name for checkboxes and radios.
Refusals rather than partial saves: an encrypted document returns
SaveError::Encrypted, because writing plaintext objects into it would
corrupt the file; a source with no startxref or no /Root is refused; and a
save with no pending edits returns the input unchanged rather than growing
the file and churning its timestamp.
Tests: 10 acceptance tests in tests/save_roundtrip.rs covering the ADR merge
criteria. The central one reparses from the written bytes rather than
reusing in-memory state, so it tests the file rather than the writer against
itself. Also asserts three chained revisions still reparse with the newest
value winning, that pages and annotations survive a save, and that saving
never panics on the malformed corpus.
Known limitations, recorded in the ADR rather than glossed: cross-reference
streams and object streams are not written, and superseded objects are not
compacted.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (307 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (351 tests, 6 ignored)
Both rustfmt and clippy -D warnings clean.
