nigig-org/REVIEWS/adr/0018-pdf-destinations.md

ADR 0018: destinations — the module at 0% coverage was dead, and every internal link with it

• Status: Accepted
• Date: 2026-08-16
• Review item: the open item left by ADR 0017, "destinations.rs remains at 0% coverage and has no floor; it needs tests before it needs a floor"
• Supersedes: nothing
• Related: ADR 0017 (declared-versus-delivered; this is the same class of defect, found by following its open item), ADR 0012 (refusing dangerous actions rather than performing them), ADR 0004 / ADR 0006 (deep-resolve destroying the references that identify things)

Context

ADR 0017 recorded destinations.rs at 0% line coverage. The obvious reading is "an untested module". The real one is worse.

Nothing called it. The module was pub used from lib.rs and referenced from nowhere else in the workspace:

$ grep -rn 'destinations::|GoToAction|DestinationKind' --include='*.rs' .
./pdf-document/src/lib.rs:23:pub use destinations::{Destination, GoToAction};

0% coverage was not a gap in the tests. It was the symptom of dead code, and the thing it was supposed to do was not being done by anything else.

Internal links did nothing

PdfAnnotation::from_dict read a link's target as:

dest: dict.get_name("Dest").map(|s| s.to_string()),

A name /Dest, and nothing else. Not /Dest [4 0 R /Fit] (an explicit array), and not /A << /S /GoTo /D ... >> — which is how internal links are written in practically every real document.

The corpus already contained one, in annotations/links.pdf, and had since Phase 6:

11: << /Type /Annot /Subtype /Link /Rect [72 660 272 680]
      /P 3 0 R /A << /S /GoTo /D [4 0 R /Fit] >> >>

Probing it before any change:

Link { uri: Some("https://example.org/docs"), dest: None }  -> OpenUri(...)
Link { uri: None, dest: None }                              -> action=None

action=None. Clicking that link did nothing. No error, no warning — the viewer received no action and correctly performed none. A link to nowhere and a link the reader cannot parse are indistinguishable from the outside, which is exactly the failure mode ADR 0017 is about, one layer up.

The viewer was already wired for this: PdfAction::GoToPage exists in interaction.rs, is matched in test_host.rs, and was never constructed by anything. A complete delivery path with nothing at the source end.

Why no test caught it

The corpus had one internal link and no test asserted where it went. annotation_roundtrip.rs and corpus.rs checked that link annotations parsed and that their rectangles were right. Both passed. The assertion that was missing is the one that names a page number.

Decision

Make the module real, and call it

destinations.rs now parses all three forms a destination is legally written in (§12.3.2): an explicit array, a name, a byte string, and the /D-wrapped dictionary a name tree leaf uses. Destination::from_object accepts any of them.

XYZ carries Option<f64> per component, because null is meaningful and only meaningful there: it means "leave this unchanged". Collapsing null to 0.0 is not a harmless default — it scrolls to the origin, and for zoom it requests 0% magnification. A zoom of literal 0 means the same as null (Table 151) and is normalised to None, so a viewer cannot render at zero.

An unrecognised fit degrades to /Fit rather than failing: losing the framing is recoverable, losing the page is not.

Resolve names where the catalogue is in reach

PdfDocument gained lookup_named_destination, resolve_destination and named_destinations. Both spellings are searched, because both are current: the /Names /Dests name tree (§7.9.6, walked through /Kids) and the pre-1.2 /Root /Dests dictionary. Supporting only the modern one loses every link in an older file.

Lookup uses a deliberate resolve_shallow, not resolve. Deep-resolving a destination array replaces [4 0 R /Fit] with the page dictionary and destroys the only thing identifying the target — the same defect that once emptied every AcroForm (ADR 0006) and every annotation reference (ADR 0004). Mutation M8 below confirms this is load-bearing, not superstition.

Destinations are resolved in page_annotations, so annotations reach the viewer already carrying a page index. An unresolvable one is left unresolved, never defaulted to page 0: silently landing on the first page is the worst outcome, because it looks like the link worked.

Refuse by verb

GoToAction::from_dict requires /S to be GoTo. The previous version ignored /S entirely and read /D from whatever it was given, so a /GoToR (another file), a /Launch (a program) or a /JavaScript carrying a /D was reported as an in-document page jump. Same policy as ADR 0012: refuse by name, do not silently perform.

Verification

Every claim checked by running it.

Before: action=None for the corpus's only internal link. After: action=Some(GoToPage { page_index: 1 }).

Mutation testing — seven mutations, all killed:

#	Mutation	Result
M1	/S verb check removed	1 fails — /Launch became GoToPage { page_index: 1 }
M2	null in XYZ read as 0.0	1 fails
M3	zoom 0 not normalised	1 fails
M4	name tree /Kids not descended	2 fail
M5	legacy /Root /Dests fallback removed	1 fails
M6	unresolvable name defaults to page 0	1 fails
M7	/Dest on the annotation ignored	4 fail
M8	resolve_shallow → deep resolve	5 fail

M1 was a false negative on the first attempt, and that is worth recording. The first run reported it surviving. It had not survived — the Python replacement string omitted an interleaved comment, so the patch never applied and I measured the unmutated build. Re-run with an assert old in s guard, it failed as it should. A mutation harness that does not verify the mutation was applied reports "test is weak" when the truth is "the test never ran against changed code", and the conclusion — deleting a real security check as untested — would have been actively harmful. Every mutation here now asserts its own application.

Coverage: destinations.rs 0% → 98.65%; total 83.42% → 83.86%. Floors added for destinations.rs (94) and annotations.rs (70), and the new floor verified to fail when raised above measurement.

Suite: TEST_TARGET=pdf ./tools/test-rust-clean.sh → 724 passed, 0 failed (was 695; +29).

Merge criteria

• destinations.rs is called by production code, not only exported
• Explicit array, name, string and /D-wrapped forms all parse
• All eight fit kinds parse to distinct values, asserted individually
• null in XYZ reads as "unchanged", not 0.0
• Zoom 0 normalised to "unchanged"; coordinate 0 kept as 0.0
• Named destinations resolved through a nested /Names /Dests tree
• Legacy /Root /Dests dictionary resolved
• /GoToR, /Launch, /JavaScript refused by verb even when carrying a /D
• An unresolvable destination never becomes page 0
• A cyclic name tree terminates
• Corpus-wide invariant: no link resolves past the end of its document
• Five destinations/ fixtures generated by generate.py
• Seven mutations, each verified to apply and each killed
• Coverage floor added for destinations.rs, verified to fail when breached
• TEST_TARGET=pdf ./tools/test-rust-clean.sh green (724)
• cargo fmt --check and clippy -D warnings clean

Consequences

Positive. Internal navigation works: table-of-contents links, cross-references and outline targets now reach the viewer as page indices. The GoToPage path that existed end-to-end but was never fed is now fed. A dead module is either alive or gone.

Negative. AnnotationType::Link changed shape — dest: Option<String> became destination: Option<Destination> — and AnnotationAction gained GoToDestination. Both are public. The old field could not express an explicit destination, so keeping it would have meant keeping the bug.

AnnotationAction also lost its Eq derive, because a destination carries f64 coordinates. Deriving Eq would have forced those coordinates to be dropped or rounded, which is the wrong trade for a comparison nothing needs.

Risk. resolve_destination is called for every link annotation on every page_annotations, adding a name-tree walk per named link. The tree is traversed linearly rather than binary-searched: correctness first, and these trees are small. If a document with thousands of named links ever shows up in a profile, the ordered /Limits make a binary search straightforward.

Out of scope, deliberately: /GoToR remote navigation (parsed and refused, not followed — acting on it means opening another file, which is a host decision under rule 5), outline /Outlines trees, /OpenAction on the catalogue, and structure-destination /SD preference. Each is a separate feature with its own risk, not a detail of this one.