andodeki
cf73ef4c1d
Every serious bug in this stack has had one shape: a valid, well-typed,
empty-or-default value where the file plainly declared content. xobjects
empty for every document; acroform() dropping every field behind an
indirect reference; DCTDecode returning its own compressed bytes; a JPEG
decoder that was a stub returning black. None errored, none panicked, and
the tests asserted Ok, which they got.
Coverage would not have caught any of them. Measured when each shipped:
page.rs 92.4%, form.rs 93.6%, content.rs 89.2%, xref.rs 95.2%. The buggy
lines ran; nobody checked what they produced.
So: a property test that walks the raw object graph of every corpus
fixture, counts what the file declares, and requires the API to deliver
it - fonts, xobjects, graphics states, colour spaces, form fields,
filters, MediaBox. It reimplements the resolution rule independently of
page.rs on purpose; a test that asks the code under test what to expect
agrees with the bug.
It failed the day it was written, on a shape the corpus had never
contained. Every fixture wrote /Resources inline, and all six extractors
read it with dict.get_dict("Resources") - which returns None for an
indirect reference and never consulted /Parent. A page with
"/Resources 5 0 R", the commonest shape in real PDFs, reported no fonts,
no xobjects, no graphics states and no colour spaces. Same for a page
inheriting resources from its /Pages node. Empty, not wrong, so nothing
failed.
Fixed by resolving /Resources once in PdfPage::from_obj through a helper
implementing the full inheritance rule (32000-1 Table 30), and passing
the resolved dictionary down. Indirect /MediaBox entries resolve too.
Six resources/ fixtures cover the shapes that were missing.
Mutation-checked: reverting inheritance kills 5 tests, the sub-dict
reference 3, indirect MediaBox 2, and removing the depth bound hangs.
One mutation survived - a visited-set guarding a /Parent cycle, which
the depth bound already handles - so it was deleted rather than left as
untested defence with a reassuring comment.
tools/test-pdf-coverage.sh enforces a floor instead of printing a number,
with per-file floors as well as a total: image.rs could fall from 33% to
5% and move the total by under a point. All three failure modes verified
to fail. It caught a bug in itself first - its ignore regex matched its
own work directory and reported a confident TOTAL 0.00%.
.gitattributes marks *.pdf binary. An xref entry must be exactly 20 bytes
(7.5.4), so with a one-digit generation field it ends in a space, and
git diff --check was reporting unfixable "trailing whitespace" on every
fixture in the corpus.
TEST_TARGET=pdf: 695 passed, 0 failed (was 680). Coverage 83.42%.
ADR 0017 records the four mutations so they can be repeated by hand.
221 lines
10 KiB
Markdown
221 lines
10 KiB
Markdown
# 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
|
||
|
||
- [x] 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
|
||
- [x] It re-implements the resolution rule independently of the code it
|
||
tests
|
||
- [x] Indirect `/Resources` delivers its fonts, asserted by value
|
||
- [x] Inherited `/Resources` delivers its fonts, asserted by value
|
||
- [x] Inheritance climbs past an intermediate node that overrides a
|
||
*different* key
|
||
- [x] Indirect `/Font` and `/XObject` sub-dictionaries are followed
|
||
- [x] Indirect `/MediaBox` entries resolve to numbers
|
||
- [x] A `/Parent` cycle terminates
|
||
- [x] Six `resources/` corpus fixtures, generated by `generate.py`
|
||
- [x] Every fix mutation-checked; the one surviving mutation resulted in a
|
||
code change, not a comment
|
||
- [x] Per-file and total coverage floors enforced, with all three failure
|
||
modes verified to fail
|
||
- [x] The floor script runs in CI
|
||
- [x] `TEST_TARGET=pdf ./tools/test-rust-clean.sh` green (695)
|
||
- [x] `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.
|