feat(asciihex): ASCIIHexDecode encode + decode + chain wiring (PR #01, stacked on #3) by rjzondervan · Pull Request #4 · ConductionNL/sapp

rjzondervan · 2026-05-27T09:54:57Z

Summary

Implements PDF 1.7 §7.4.2 ASCIIHexDecode as the first of four filter PRs that attach to the chain dispatcher from #3. The dispatcher arm is a strict case extension — no structural change to the dispatcher itself.

Stacked on #3 feat/filter-chain-dispatch. This PR's base will auto-retarget to work/text-replacement once #3 merges.

What landed

Helper (on `PDFObject`, `protected static`)	Purpose
`ASCIIHexDecode($_stream, $params): string\|false`	Strips PDF whitespace (§7.5.1), finds EOD `>`, pads odd trailing nibble with `0`, accepts both cases, fail-safe on illegal chars
`ASCIIHexEncode($_stream, $params): string`	Uppercase hex pairs + 80-col line wrap + EOD `>`; empty input emits just `>`

Both wired into apply_filter_chain_decode / apply_filter_chain_encode via case 'ASCIIHexDecode':.

Spec contract

Pinned by openspec/changes/feat-asciihex-decode/:

REQ-1 — ASCIIHexDecode SHALL decode per PDF 1.7 §7.4.2 (even-length, odd-length-pad, whitespace, lowercase, trailing-after-EOD)
REQ-2 — ASCIIHexEncode SHALL emit uppercase hex pairs with EOD (basic, empty, 80-col line wrap)
REQ-3 — Lossless round-trip on arbitrary binary input
REQ-4 — Illegal characters SHALL fail safely (p_error + raw input return; no exception)
REQ-5 — Chain dispatcher recognises /ASCIIHexDecode on both decode + encode (MODIFIED capability from feat(filter-chain): array-form /Filter dispatch in PDFObject (PR #05 — foundation) #3)

Test plan

php examples/poc-filter-roundtrip-asciihex.php — 5 REQs, 0 assertion failures
php examples/poc-replace-text.php — baseline still green (PoC FlateDecode path unchanged)
php examples/poc-filter-chain-roundtrip.php — dispatcher tests green after sentinel update
php -l clean on touched files
No new composer dependencies; PHP 7.4 compat preserved

Companion change to dispatcher's round-trip test

Dispatcher's "unknown filter" assertion (from #3) previously used /ASCIIHexDecode as the sentinel — now that ASCIIHexDecode is implemented, the sentinel needs to be a name that won't ever be a real filter. Updated to /SappTestUnknownFilter (5 LOC change in examples/poc-filter-chain-roundtrip.php). Future filter PRs (#2-#4) won't need to touch the sentinel.

Files changed (348 +, 23 −)

src/PDFObject.php — ASCIIHexDecode + ASCIIHexEncode helpers, dispatcher arms
examples/poc-filter-roundtrip-asciihex.php — new round-trip gate
examples/poc-filter-chain-roundtrip.php — sentinel update for forward-compat
docs/upstream-prs/01-asciihex-decode/{proposal,design,tasks}.md — implementation notes
openspec/changes/feat-asciihex-decode/tasks.md — 22/22 complete

Dependencies + ordering

Depends on: feat(filter-chain): array-form /Filter dispatch in PDFObject (PR #05 — foundation) #3 (filter-chain-dispatch — the dispatcher this attaches to)
Parallel-ish with: chore(openspec): scaffold OpenSpec workflow for the text-replacement upstream-PR series #2 RunLengthDecode, feat(filter-chain): array-form /Filter dispatch in PDFObject (PR #05 — foundation) #3 ASCII85Decode, feat(asciihex): ASCIIHexDecode encode + decode + chain wiring (PR #01, stacked on #3) #4 LZWDecode (any order; each adds one case arm; orthogonal codecs)
Unblocks: nothing on its own; combined with chore(openspec): scaffold OpenSpec workflow for the text-replacement upstream-PR series #2-feat(asciihex): ASCIIHexDecode encode + decode + chain wiring (PR #01, stacked on #3) #4 unblocks PR feat(ascii85): ASCII85Decode encode + decode + chain wiring (PR #03, stacked on #5) #6 (ToUnicode CMap) which needs the chain dispatcher proven across multiple filter types

Out of scope

ASCII85 / RunLength / LZW filters (PRs chore(openspec): scaffold OpenSpec workflow for the text-replacement upstream-PR series #2-feat(asciihex): ASCIIHexDecode encode + decode + chain wiring (PR #01, stacked on #3) #4)
/DecodeParms for ASCIIHexDecode — none defined per PDF 1.7 Table 5

…#1) Implements PDF 1.7 §7.4.2 ASCIIHexDecode on top of the chain dispatcher from PR #5. Both encode and decode go through the existing chain plumbing — adding ASCIIHexDecode is a strict `case` extension to apply_filter_chain_decode / apply_filter_chain_encode, no structural change to the dispatcher itself. New helpers on PDFObject (both `protected static`, PascalCase per upstream's filter-name convention): - ASCIIHexDecode($_stream, $params): string|false Strips PDF whitespace (§7.5.1), finds EOD `>`, discards anything after, pads odd-length input with `0` per §7.4.2 ¶3, accepts both cases of hex digits. Illegal characters → p_error + return raw input (D2 fail-safe, matches upstream convention). - ASCIIHexEncode($_stream, $params): string strtoupper(bin2hex($_stream)) + chunk_split at 80 columns + EOD `>`. Empty input emits just `>`. Dispatcher `case 'ASCIIHexDecode':` arms added to both encode + decode chain helpers. The dispatcher's existing round-trip test (from PR #5) updated to use `/SappTestUnknownFilter` as the unknown-filter sentinel so it stays valid as subsequent filter PRs (#2-#4) land their own case arms. Contract pinned by openspec/changes/feat-asciihex-decode/ (proposal + design D1-D5 + spec REQ-1 through REQ-5 with MODIFIED filter-chain-dispatch capability + tasks). Verification: - examples/poc-filter-roundtrip-asciihex.php (new) — 5 REQs covered: decode scenarios (even-length, odd-length-pad, whitespace, lowercase, trailing-after-EOD), encode scenarios (basic, empty input, 80-col wrap), lossless round-trip on 1024 random bytes, illegal-char fail-safe, chain integration (single-filter and [/ASCIIHexDecode /FlateDecode] two-filter outer-envelope). - examples/poc-replace-text.php — baseline still green. - examples/poc-filter-chain-roundtrip.php — dispatcher tests pass after the unknown-filter sentinel update. Implementation note for the eventual upstream PR (preserved in docs/upstream-prs/01-asciihex-decode/): hex2bin + chunk_split-based implementation keeps the code path under 40 LOC and dependency-free. PHP 7.4 minimum preserved. Closes openspec/changes/feat-asciihex-decode/tasks.md 22/22.

WilcoLouwerse

Strict-mode review (REQUEST_CHANGES)

ASCIIHexDecode/Encode implementation is mostly spec-correct, but contains a critical contract bug: the decoder's documented string|false return is never false on failure (it returns the raw input), so the dispatcher's === false short-circuit is dead code and a corrupted chain silently passes garbage downstream; several other gaps in tests, docs, and edge cases.

Findings: 🔴 3 blockers · 🟡 6 concerns · 🟢 9 minors. All findings have been posted as inline comments above. Per Strict-mode rules, any 🟡 or 🔴 holds the verdict; address each thread before requesting re-review.

🔴 Blockers

src/PDFObject.php:237 — ASCIIHexDecode never returns false on failure — dispatcher's error short-circuit is dead code
src/PDFObject.php:222 — PHPDoc @return string|false contradicts implementation (never returns false)
examples/poc-filter-roundtrip-asciihex.php:201 — Chain integration has no test for illegal-char failure propagation

(Inline comments include **Impact:** and **Suggested fix:** sections per Strict-mode body template.)

Adds two new commits on top of the existing PR #3 branch: 1. The PR #1 fix cherry-picked: PoC's PDFDoc::replaceTextInDocument rename to snake_case + add_object write-back + escape-contract docblock + streams_scanned semantics + Phase-2 exhaustive sweep in the verify gate + namespace/named-arg fixes in poc-make-fixture.php. Same content as the corresponding PR #1 fix; arrives here because PR #3 has a cherry-pick of the PoC and the PR #1 review's blockers apply to that copy too. 2. This commit: PR #3-specific fixes from Wilco's first-pass strict review. 🔴 PR #3 Blocker - PHP 7.4 named-args in poc-make-fixture.php — addressed by commit (1) above (the cherry-picked PR #1 fix replaces named args with positional and pins gzcompress level to 6). 🟡 PR #3 Concerns - get_stream(false) on chain failure now returns `false` (matching upstream's pre-refactor `return p_error(...)` semantics) rather than the raw $this->_stream. Callers using the `if ($decoded === false) continue;` idiom get the skip behaviour they did before the dispatcher refactor. spec.md REQ-5 scenario updated to match. - build_flate_params /Columns default fixed from 0 → 1 per PDF 1.7 §7.4.4.3 Table 8. Hidden today because Predictor=1 short-circuits; would have bitten the moment a PNG-predicted stream landed without explicit /Columns. - REQ-2 (encode-decode round-trip lossless) spec scenario tightened to "Predictor absent or = 1" — round-trip with a non-trivial PNG predictor is acknowledged as out of scope (no encode-side predictor support); future `feat-flate-predictor-encode` closes the gap. - design.md D1 wording fixed from "We add two `private` helpers" to "We add two `protected static` helpers", matching what shipped. Visibility choice justified (test-suite subclasses can stub for chain-ordering assertions; matches FlateDecode). - design.md D6 rewritten to reflect what the gate actually exercises (5 contract scenarios including the new REQ-3 smoke test) instead of the original "synthetic two-filter PDF" promise. Two-filter ordering is locked at spec layer but proved inductively by upstream-PRs #1-#4 each pairing the new codec with /FlateDecode. - examples/poc-filter-chain-roundtrip.php gained a REQ-3 block smoke-testing /DecodeParms positional routing — single-filter chain with explicit `/DecodeParms [<</Predictor 1 /Columns 4>>]` that round-trips cleanly. - docs/upstream-prs/05-filter-chaining/{proposal,design,tasks}.md no longer carry the duplicated 18-line "Implementation note" block. Each file now links to `openspec/changes/feat-filter-chain-dispatch/design.md` as the canonical artefact — single source of truth. - docs/upstream-prs/README.md now explicitly explains the two numbering schemes: directory `01..08` is the upstream-submission order; fork PR numbers are the local landing order. Foundation (`05`) lands first locally so dependent codecs (`01..04`) can attach. Cross-references use the `upstream-PR #NN` form. - replaceTextInDocument camelCase fixed — via the PR #1 fix cherry-pick (commit 1 above) which renames to snake_case replace_text_in_document. 🟢 Minors deferred - normalise_filter_chain dead defensive branch — the branch handles a `null` input that may or may not be reachable depending on PDFObject construction path; leaving as-is. Verification: both poc-replace-text.php (PR #1 gate) and poc-filter-chain-roundtrip.php (this PR's gate, now with the REQ-3 smoke test block) exit 0 after these two commits. NOTE on the stacked-PR / no-force-push approach: this PR's commit diff still references the OLD scaffold content from PR #2 (the pre-rewrite spec.md / proposal.md / etc.). PR #2's mechanical pass (REQ-NNN numbering + GIVEN clauses + Status/Purpose stub headers) is shipped on `chore/openspec-scaffold` and will arrive here naturally when this branch is rebased onto an updated `work/text-replacement` after PR #2 merges. The spec amendments in this commit will then merge with PR #2's rewrites — small expected conflicts in spec.md's REQ-5 + REQ-2 scenarios, resolvable inline.

# Conflicts: # examples/poc-filter-chain-roundtrip.php

Adds a merge commit bringing `feat/filter-chain-dispatch`'s fix commits (PoC snake_case rename + chain dispatcher get_stream-returns- false + Columns default fix + docs/upstream-prs deduplication) into this branch, and a follow-up commit with PR #4 specific fixes. 🔴 PR #4 Blockers - ASCIIHexDecode failure-path return changed from `$_stream` (raw input) to `false`. Matches the chain dispatcher's `=== false` short-circuit contract and upstream `p_error`'s default return. Critical correctness fix: prior to this, a malformed outer- ASCIIHex layer in a `[/ASCIIHexDecode /FlateDecode]` chain silently fed the unmodified illegal bytes to the inner FlateDecode arm — silent data corruption. - PHPDoc on ASCIIHexDecode updated: `@return string|false` is now accurate (the failure paths return `false` per the previous fix). Description clarified to surface the chain-dispatcher contract requirement. - Negative chain test added in `poc-filter-roundtrip-asciihex.php` asserting the dispatcher short-circuits on outer-ASCIIHex failure: `[/ASCIIHexDecode /FlateDecode]` chain with malformed hex bytes MUST return `false` from `get_stream(false)`. This locks the failure-propagation contract that the previous bug silently bypassed. 🟡 PR #4 Concerns - 80-col line wrap edge case in `ASCIIHexEncode`: when the hex representation is a multiple of 80, the encoder previously appended `>` directly after the final chunk, producing an 81- char line. Now inserts a newline before `>` so no line exceeds 80 chars. Locked by a new 40-byte (80-hex-char) test case. - `tasks.md` 1.2 corrected: shows the actual stacked-PR base (`feat/filter-chain-dispatch`) instead of `work/text-replacement`. - `tasks.md` 2.2 corrected: the failure-path returns `false` not raw input (matches the post-fix code). - `tasks.md` 4.1 corrected: case label is bare `'ASCIIHexDecode'` not `'/ASCIIHexDecode'` (the dispatcher strips the leading slash before the switch). - `docs/upstream-prs/01-asciihex-decode/{proposal,design,tasks}.md` no longer carry the duplicated 9-line implementation note. Each links to `openspec/changes/feat-asciihex-decode/design.md` as the canonical artefact (same pointer-only pattern as PR #3). 🟡 Concerns deferred to follow-ups - Numbering scheme reconciliation: covered by the `docs/upstream-prs/README.md` explainer in PR #3's fix commit (`upstream-PR #NN` vs fork PR # numbering), which arrives here via the merge. - Encode-dispatcher symmetry on infallible filters: ASCIIHexEncode documented as infallible; an explicit `=== false` symmetry would require a return-type contract change. Tabled. 🟢 Minors addressed in tests - Missing-EOD tolerance now has an assertion (`ASCIIHexDecode('48656C6C6F') === 'Hello'`). - Empty-input edge cases asserted: `'>'`, ` ' \n \t >'`. - Round-trip of empty input asserted: `decode(encode('')) === ''`. - Unnecessary `@` operator on `hex2bin` dropped (the alphabet validation above guarantees it can't legitimately fail). 🟢 Minors deferred - PHP case-insensitivity nit on `ASCIIHexDecode` collision — no action; PascalCase-on-filter-names is the established convention. - PHP 7.4 union return type in docs — the docs use PHPDoc-only notation already; the docs/upstream-prs note is reasonable. - ReflectionClass coupling — by design; documented. - `p_error` stdout assumption — acceptable; tests don't depend on the error channel routing. Verification: all 3 gates exit 0 - poc-replace-text.php (PR #1 gate) - poc-filter-chain-roundtrip.php (PR #3 gate) - poc-filter-roundtrip-asciihex.php (this PR's gate, with the new chain-failure-propagation test + 40-byte 80-col edge + the missing-EOD/empty-edge assertions)

… feat/asciihex-decode # Conflicts: # openspec/changes/feat-asciihex-decode/specs/asciihex-decode-filter/spec.md # openspec/changes/feat-asciihex-decode/tasks.md

WilcoLouwerse

APPROVE — focused fix commit 70d9a3a cleanly addresses every 🔴 blocker and the substantive 🟡/🟢 findings from the first-pass review. Verified against src/PDFObject.php at head + the expanded examples/poc-filter-roundtrip-asciihex.php (chain-failure propagation, 40-byte 80-col boundary, missing-EOD tolerance, empty round-trip, EOD-first edges).

🔴 3/3 resolved

false-return on illegal char → ASCIIHexDecode now returns false on alphabet failure AND on defensive hex2bin failure; chain dispatcher's === false short-circuit now actually fires.
PHPDoc contract → @return string|false accurate; doc body documents the chain-dispatcher rationale.
Negative chain test → [/ASCIIHexDecode /FlateDecode] malformed-outer test asserts get_stream(false) === false.

🟡 5/6 resolved, 1 deferred with rationale

Resolved: 80-col line wrap edge (modulo-80 guard + 40-byte regression test); task 1.2 stacked base; task 4.1 case label; doc dedup; numbering scheme (clearer upstream-PR #NN vs fork PR # notation + README explainer via merge).
Deferred with rationale: encode-dispatcher symmetry — ASCIIHexEncode documented as infallible; explicit === false symmetry would require a contract change. Acceptable.

🟢 6/9 resolved, 3 deferred with rationale, 1 silent (non-blocking)

Tests added: missing-EOD assertion, > and whitespace-EOD edges, empty round-trip.
Code: @hex2bin suppression dropped; misleading signature notation resolved by removing the duplicated implementation note from the docs/upstream-prs files.
Deferred with rationale: case-collision (PascalCase-on-filter-names is the convention), ReflectionClass coupling (was "no action required" from the start), p_error stdout assumption.
Silent: consecutive set_stream calls was a "consider a comment or refactor" nit — not blocking.

Resolved all 18 prior threads. No new findings. Cleared for merge once #3 lands.

feat(runlength): RunLengthDecode encode + decode + chain wiring (PR #2, stacked on #4)

feat(lzw): LZWDecode + FlateDecode predictor refactor (PR #4, stacked on #6)

@hex2bin

Blockers: - Only the first match per TJ operator was processed. processTjArray returned on the first hit; outer caller advanced past the entire TJ. Multi-needle TJs and same-needle-twice-in-one-TJ under-counted replacements_per_needle. Rewrote with an outer while-loop that re-resolves fragment texts after each splice and continues until no remaining needle matches. New REQ-006 in spec.md anchors the contract with two-needle and same-needle- twice scenarios. - Missing isset check on $stats['replacements_per_needle'][$needle]++ in processTjArray. Lazy-init kept as a belt-and-braces guard (the entry point already pre-keys via array_fill_keys, but the guard survives future entry-point refactors). - Odd-length hex silently dropped a char (@hex2bin($hex) ?: ''). PDF 1.7 §7.3.4.3 mandates implicit trailing-zero padding. Now pads via `if (strlen($hex) & 1) $hex .= '0'`. New REQ-005 scenario covers this. - Non-spec leniency: numeric tokenizer accepted [-+\d.eE]. PDF 1.7 §7.3.3 forbids exponent notation in Numeric Objects. Tightened to [-+.0-9] so a producer's `e` glyph after a hex CID can't be swallowed as part of a number. New REQ-005 scenario covers this. - Phantom-ticked tasks 5.4 and 5.5 (no fixtures). Reverted to unchecked with explicit deferral notes. Concerns: - Dead $matchedOnce assignment removed via the rewrite. - Comments not treated as whitespace inside TJ arrays. Now skipped per PDF 1.7 §7.2.4. REQ-005 scenario. - tj_arrays_modified diagnostic key now pre-initialised at top of replace_text_in_document (contract no longer fragile). - preg_match-per-byte for whitespace replaced with strpos against the 6-byte PDF whitespace set (~50× faster on large TJ arrays). - Shape inheritance: placeholder now always emitted as literal regardless of first matched fragment's shape (matches D2 documented contract). - `@hex2bin` defensive `@` removed — the odd-length check above makes the warning unreachable; PHP errors for other malformed shapes now surface to callers explicitly. - end() / reset() pointer-advancing avoided in the new render path (group-based emission uses numeric indexing). - Triplicate Implementation Note blocks in docs/upstream-prs/07-tj- flattening/{proposal,design,tasks}.md replaced with canonical pointers (same pattern as PRs #1/#2/#3/#4/#5/#6). - Tf set inside q/Q leaks documented as a known gap (q/Q stack not maintained); deferred to a follow-up. Verification: all 7 gates green (poc-replace-text, poc-tounicode-cmap, poc-tj-flattening, plus the 4 filter PoCs). Also fixed in this commit: poc-tj-flattening.php was calling $doc->replaceTextInDocument() (camelCase) which doesn't exist after PR #1's snake_case rename. Updated to replace_text_in_document.

feat(asciihex): /ASCIIHexDecode filter (SAPP PR #1)

feat(lzw): /LZWDecode filter + applyPngPredictor refactor (SAPP PR #4)

WilcoLouwerse mentioned this pull request May 28, 2026

feat(filter-chain): array-form /Filter dispatch in PDFObject (PR #05 — foundation) #3

Merged

4 tasks