nigig-org/REVIEWS/adr/0017-pdf-declared-versus-delivered.md

ADR 0017: declared versus delivered — the invariant behind four silent bugs, and a coverage floor

• Status: Accepted
• Date: 2026-08-16
• Review item: the coverage/mutation tranche proposed after Phase 3, in answer to the standing question "why do we have bugs — are we writing code with no complete 100% test coverage?"
• Supersedes: nothing
• Related: ADR 0009 (gs mis-parsed), ADR 0011 (BMC never parsed), ADR 0015 (filters), ADR 0016 (the stub JPEG decoder). Each of those fixed one instance of the pattern this ADR generalises.

Context

Every serious bug found in this stack so far has had the same shape, and it is not the shape people write tests for.

Bug	What it returned	Why nothing failed
PdfPage::xobjects empty for every document	{}	a page with no xobjects also returns {}
acroform() dropped fields behind an indirect /AcroForm	a form with 0 fields	a document with no fields also has 0
DCTDecode returned its compressed input	plausible bytes	a caller cannot tell decoded from not
the JPEG decoder was a stub	a correctly sized black image	black is a legal image

None of these returned an error. None panicked. All four returned a valid, well-typed, empty-or-default value — and the calling test asserted Ok, which it got. This is why the coverage question has a counter-intuitive answer, and the measurements say so plainly.

Coverage would not have caught them

Measured with llvm-cov at the time each bug was found:

File	Coverage when the bug shipped
page.rs	92.4%
form.rs	93.6%
content.rs	89.2%
xref.rs	95.2%

The buggy lines were executed by the test suite. They were executed and their results were not checked, or were checked against a fixture that avoided the broken shape. 100% line coverage would have changed nothing: coverage measures whether a line ran, and the defect was in what the line produced. So the answer to "are we writing code with no complete coverage" is: coverage was already high in exactly the files that broke.

What was missing was an invariant — something that knows what the answer should be without being told case by case.

Decision

Three things, in order of value.

1. A declared-versus-delivered property test

pdf-document/tests/declared_resources.rs walks the raw object graph of every corpus fixture, counts what the file declares, and requires the API to deliver the same thing. It tests no single function; it asserts a relationship that must hold for every document, including documents nobody has written a fixture for yet.

It re-implements the spec rule (indirect references, /Parent inheritance) independently of page.rs. This matters: a test that computes its expectation by calling the code under test agrees with the bug. The two implementations disagreeing is the signal.

Covered invariants: declared fonts, xobjects, graphics states and colour spaces are all delivered; every declared /AcroForm field is delivered; every declared filter either decodes to something different from its input or fails with an error naming the filter; a declared /MediaBox is never silently replaced by the 612×792 default; resolving every object of every fixture terminates.

Each of those would have caught at least one of the four bugs above, and the filter one would have caught two.

2. Two real bugs it found immediately

Written first, it failed on the day it was written — on a shape that had never had a fixture.

Every fixture in the corpus wrote /Resources inline on the page. All six resource extractors read it with dict.get_dict("Resources"), which returns None for an indirect reference, and none of them consulted /Parent. So:

• a page with /Resources 5 0 R — the commonest shape in real PDFs — reported no fonts, no xobjects, no graphics states, no colour spaces;
• a page inheriting /Resources from its /Pages node reported the same.

Empty, not wrong. The page rendered without its resources and every existing test still passed. Confirmed by probe before any fix:

indirect_resources.pdf:  fonts=[] content_len=49
inherited_resources.pdf: fonts=[] content_len=49

and after:

indirect_resources.pdf:  fonts=["F1"] content_len=49
inherited_resources.pdf: fonts=["F1"] content_len=49

Fixed by resolving /Resources once, in PdfPage::from_obj, through a helper implementing the full inheritance rule (PDF 32000-1 Table 30, which makes /MediaBox, /CropBox, /Rotate and /Resources inheritable), and passing the resolved dictionary to the extractors. Six duplicated lookups became one. /MediaBox entries that are themselves indirect are now resolved too, and six resources/ fixtures were added for the shapes the corpus had never contained.

3. A coverage floor that is enforced, not printed

tools/test-pdf-coverage.sh runs the engine suites under -C instrument-coverage in a throwaway toolchain and fails the build below a floor. It carries per-file floors as well as a whole-stack one, because a single total hides the exact regression it is supposed to catch: image.rs could fall from 33% to 5% and move the total by under a point.

Current measurement: 83.42% total, floor 80%. Per-file floors are set a few points under measurement on the files that have actually harboured bugs (image.rs, jpeg.rs, font.rs, cmap.rs, content_writer.rs, graphics_state.rs, filter.rs, page.rs, document.rs).

A floor naming a file that is not measured is a failure, not a pass: otherwise renaming a file silently deletes its protection.

Verification

Every claim here was checked by running it, not by reading the code.

The property tests fail when the fix is reverted. Four mutations:

Mutation	Result
/Resources inheritance → direct lookup	5 tests fail
indirect /Font, /XObject sub-dict not followed	3 tests fail
indirect /MediaBox entries not resolved	2 tests fail
/Parent depth bound removed	hangs (killed at 240s)

One mutation survived, and that changed the code. A visited-set guard against a /Parent cycle was written alongside the depth bound. Removing it failed nothing — the depth counter already terminates a cycle and both paths return None. Two mechanisms where one suffices means one is never exercised, so the visited-set was deleted rather than left as untested defence with a reassuring comment. This is the same lesson as the LZW reserved-slot mutation in ADR 0015, where a surviving mutation revealed two independent code paths rather than a weak test.

The coverage gate fails when it should. Three negative tests:

Scenario	Result
total floor raised to 99%	fails, names the total
image.rs floor raised to 90%	fails, names the file
floor on a non-existent file	fails, explains the rename risk

The gate also caught a bug in itself: the first version's ignore regex matched its own work directory (pdf-coverage.*), excluded every source file, and reported a confident TOTAL 0.00% — a coverage tool silently measuring nothing, which is precisely the failure mode this ADR is about.

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

Merge criteria

• A property test asserts declared-versus-delivered for fonts, xobjects, graphics states, colour spaces, form fields, filters and /MediaBox, over the whole corpus rather than named fixtures
• It re-implements the resolution rule independently of the code it tests
• Indirect /Resources delivers its fonts, asserted by value
• Inherited /Resources delivers its fonts, asserted by value
• Inheritance climbs past an intermediate node that overrides a different key
• Indirect /Font and /XObject sub-dictionaries are followed
• Indirect /MediaBox entries resolve to numbers
• A /Parent cycle terminates
• Six resources/ corpus fixtures, generated by generate.py
• Every fix mutation-checked; the one surviving mutation resulted in a code change, not a comment
• Per-file and total coverage floors enforced, with all three failure modes verified to fail
• The floor script runs in CI
• TEST_TARGET=pdf ./tools/test-rust-clean.sh green (695)
• cargo fmt --check and clippy -D warnings clean

Consequences

Positive. The class of bug that has produced every serious defect in this stack now has a standing assertion against it, applied to every fixture including future ones. A new extractor that returns nothing fails on the day it is written. Two real bugs affecting the commonest resource shape in real PDFs are fixed.

Negative. The property tests re-implement spec rules, so a spec rule now lives in two places and both must change together. This is intentional — the duplication is the test — but it is a maintenance cost, and a reviewer must resist the temptation to "de-duplicate" it by having the test call page.rs. Doing so would silently disable it.

Cost. The coverage run installs its own toolchain and takes about 40 seconds locally; in CI it is a separate step and will be slower.

Risk. Floors invite gaming: a test that executes lines without asserting anything raises the number. The floor is therefore documented as a regression alarm, not a target. The exemption list (malformed/, encrypted/) is by directory rather than by filename, so exempting a fixture is a visible edit and cannot be done one awkward file at a time.

Not done, deliberately: cargo-mutants as a CI job. Mutation testing was used here by hand and earned its place — it changed the code twice — but a full run over these crates is far too slow to gate a push, and a sampled run reports a different number every time. The four mutations above are recorded in this ADR so they can be repeated by hand when the resource path changes. destinations.rs remains at 0% coverage and has no floor; it needs tests before it needs a floor.