feat(tj-flat): TJ kerning-array flattening with 4-shape splicer (PR #07, stacked on #8)#9
Conversation
… PR #7) Adds TJ operator support to the text-space matcher from PR #6. Needles spanning TJ fragment boundaries now match — covers the Word-emitted character-level-kerning shape that's typical for Dutch government Woo documents. New PDFDoc helpers (~250 LOC): - processTjArray: main handler when the walker hits TJ. Locates the [...] operand, parses entries, matches needles, returns the splice tuple (preChunk, replacement, opEndOffset). - parseTjArrayContent: walks inner bytes producing entries of two kinds — 'text' (bytes + 'literal'|'hex' shape) and 'kern' (original numeric token preserved verbatim). - renderTjArray / renderTjFragment: emit entries back to PDF syntax preserving the original shapes. Splice algorithm per D2 — four output shapes by match position: Full TJ matched → (placeholder) Tj Prefix matched → (placeholder) Tj [<remaining>] TJ Suffix matched → [<leading>] TJ (placeholder) Tj Middle matched → [<leading>] TJ (placeholder) Tj [<trailing>] TJ Kerning entries OUTSIDE the match span are preserved (visual appearance of surrounding text unchanged); kerning entries INSIDE the match span are discarded (the micro-kerning was character- specific and doesn't apply to the synthetic placeholder). Trailing kerning in the leading array (and leading kerning in the trailing array) are trimmed to avoid degenerate [... -5] shapes that PDF readers would tolerate but look ugly. CID-boundary alignment: matches whose start or end lands in a fragment's interior (e.g. one fragment resolves to a 'fi' ligature and the needle ends mid-ligature) are skipped and recorded as cid_split_mismatch — same diagnostic key as the CID-interior case in PR #6. Diagnostic surface gained: - tj_arrays_modified: int (counter for matched TJ operators, separate from streams_modified) Contract pinned by openspec/changes/feat-tj-flattening/ (proposal + design D1-D5 + spec REQ-1 through REQ-5 with MODIFIED text- replacement capability + tasks). Verification: - examples/poc-tj-flattening.php (new) — synthesises minimal PDFs with character-level TJ kerning and verifies all 4 splice shapes end-to-end. 5 REQs covered, 0 assertion failures. - All 7 prior gates remain green. The stack (PoC + dispatcher + 4 filters + CMap + TJ) is now production-ready except for the Helvetica fallback (PR #8) that recovers from subset-font font_encoding_misses. Closes openspec/changes/feat-tj-flattening/tasks.md 26/26.
| @@ -875,24 +875,39 @@ private function replaceInContentStream($stream, array $substitutions, array $pa | |||
| // through the active font and run substitutions in text space. | |||
There was a problem hiding this comment.
(Anchored to nearest diff line src/PDFDoc.php:875; finding concerns src/PDFDoc.php:681.)
🟡 Concern — Operator spacing inconsistency
$replacement .= $this->renderTjArray($leadingEntries) . ' TJ '; uses single-space delimiter. The Tj path elsewhere may emit \n. Mixing single-space-glued operators in the middle of an otherwise newline-formatted stream produces oddly-formatted PDF source.
Suggested fix: Verify with a real Word fixture and compare line-break density.
| foreach ($failures as $msg) { | ||
| echo " - $msg\n"; | ||
| } | ||
| exit(1); |
There was a problem hiding this comment.
(Anchored to nearest diff line examples/poc-tj-flattening.php:249; finding concerns examples/poc-tj-flattening.php:367.)
🟡 Concern — D2 contract under-tested
Middle-match test checks substr_count($result['decoded'], 'TJ') >= 2 but doesn't verify leading/trailing TJ arrays contain original -2 kerns, nor that no -2 leaked into placeholder region.
Suggested fix: Add assertion that decoded output contains -2 between leading characters AND does NOT contain -2 immediately before/after the placeholder Tj.
| foreach ($failures as $msg) { | ||
| echo " - $msg\n"; | ||
| } | ||
| exit(1); |
There was a problem hiding this comment.
(Anchored to nearest diff line examples/poc-tj-flattening.php:249; finding concerns examples/poc-tj-flattening.php:378.)
🟡 Concern — Doesn't verify tj_arrays_modified == 0
tasks.md says 'verify ... still exits 0 with tj_arrays_modified == 0'. PoC merely shells out to poc-replace-text.php and checks $exit !== 0. Doesn't look at the new key.
Suggested fix: Read child PoC's diagnostic surface, or have child fail-loud when key is non-zero.
| /* ---------------------------------------------------------------- * | ||
| * REQ-3 — Middle match: leading TJ + placeholder Tj + trailing TJ | ||
| * ---------------------------------------------------------------- */ | ||
| { |
There was a problem hiding this comment.
🟡 Concern — Hex-shape path unexercised
buildTjFixture emits only (literal) fragments. The <hex> branch in parseTjArrayContent plus renderTjFragment hex branch never exercised. Real Word output frequently uses hex strings for Identity-H CIDs.
Suggested fix: Add a second buildTjFixtureHex covering hex fragments.
| @@ -875,24 +875,39 @@ private function replaceInContentStream($stream, array $substitutions, array $pa | |||
| // through the active font and run substitutions in text space. | |||
There was a problem hiding this comment.
(Anchored to nearest diff line src/PDFDoc.php:875; finding concerns src/PDFDoc.php:577.)
🟢 **Minor — Docblock could be clearer
Return null on unexpected byte causes outer caller to copy bytes from $pos to $next + 2. The local comment hints 'bail to fall-through' but doesn't make recovery semantics clear.
Suggested fix: Add explicit note in the docblock.
| foreach ($failures as $msg) { | ||
| echo " - $msg\n"; | ||
| } | ||
| exit(1); |
There was a problem hiding this comment.
(Anchored to nearest diff line examples/poc-tj-flattening.php:249; finding concerns examples/poc-tj-flattening.php:375.)
🟢 Minor — Cross-platform portability
On Windows, PHP_BINARY = C:\\Program Files\\php\\php.exe, escapeshellcmd strips meta chars but won't quote the path.
Suggested fix: Use escapeshellarg for portability, or document POSIX-only constraint.
| foreach ($failures as $msg) { | ||
| echo " - $msg\n"; | ||
| } | ||
| exit(1); |
There was a problem hiding this comment.
(Anchored to nearest diff line examples/poc-tj-flattening.php:249; finding concerns examples/poc-tj-flattening.php:263.)
🟢 Minor — Heuristic stream selection
Test loops over every object, finds first one with /Filter == /FlateDecode containing BT. Fixture only has one such stream. Future fixture with images that compress with BT would mis-target.
Suggested fix: Use deterministic selector (page's /Contents ref).
| @@ -875,24 +875,39 @@ private function replaceInContentStream($stream, array $substitutions, array $pa | |||
| // through the active font and run substitutions in text space. | |||
There was a problem hiding this comment.
(Anchored to nearest diff line src/PDFDoc.php:875; finding concerns src/PDFDoc.php:584.)
🟢 Minor — No sanity cap on backward scan
If producer emits a ] somewhere weird with no preceding [, the loop scans all the way to byte 0. On a 10MB stream this is O(n).
Suggested fix: Cap at 4096 bytes — TJ arrays aren't megabytes long.
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Strict-mode review (REQUEST_CHANGES)
Solid splice implementation but has critical multi-match-in-same-TJ bug, missing REQ-4 CID-alignment test coverage despite tasks marked done, PHP notice risk on undeclared stats keys, dead-code variable, broken hex-string handling for odd-length / empty hex; documentation is triplicate-duplicated across three docs files.
Findings: 🔴 5 blockers · 🟡 14 concerns · 🟢 6 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/PDFDoc.php:875 — Multi-match in a single TJ array silently lost (only first needle/occurrence processed)
- examples/poc-tj-flattening.php:249 — REQ-4 (CID-boundary alignment) marked done in tasks.md but has no assertion in the test gate
- src/PDFDoc.php:875 — Undefined-index notice / silent zero on
replacements_per_needle[$needle]++ - src/PDFDoc.php:875 — Hex string with odd-length nibbles silently produces wrong bytes (PDF 1.7 §7.3.4.3 violation)
- src/PDFDoc.php:875 — Numeric tokenizer accepts scientific notation (
1e5) — non-PDF-spec leniency
(Inline comments include **Impact:** and **Suggested fix:** sections per Strict-mode body template.)
…j-flattening # Conflicts: # openspec/changes/feat-tj-flattening/tasks.md
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.
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Re-review (Strict) — 3 new findings + 8 carry-overs
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Re-review (Strict) — REQUEST_CHANGES
Fix commit 43f6dbd substantively addressed 17 of 25 prior findings (all 5 🔴 blockers + 9 of 14 🟡 + 3 of 6 🟢). I resolved those threads. Verdict held by 1 new 🔴 + 2 new 🟡 + 4 still-open prior 🟡.
🔴 New blocker (1)
- spec.md REQ-005 / REQ-006 added without verification scenarios — same anti-pattern as the prior 🔴 phantom-REQ-4 finding; spec's own acceptance criteria (line 24) require every REQ to be exercised in the gate.
🟡 New concerns (2)
- Task 5.3 phantom-tick — claims regression PoC verifies
tj_arrays_modified == 0but only checks the child's exit code. - tasks.md missing entries for REQ-005 / REQ-006 — spec acceptance criteria (line 26) require each REQ to be cited.
🟡 Still-open carry-overs (4)
- Operator spacing inconsistency
- D2 middle-match assertions under-tested
- Regression PoC doesn't verify
tj_arrays_modified == 0(same root cause as new task-5.3 phantom-tick) buildTjFixturehex-shape branch unexercised
🟢 Still-open minors (4)
- Docblock recovery semantics unclear
escapeshellcmd(PHP_BINARY)not portable on Windows- Heuristic content-stream selection in PoC
- No sanity cap on backward
]-scan
✅ Resolved this round (17 threads — all confirmed against head SHA)
| Prior finding | How addressed |
|---|---|
| 🔴 Multi-match TJ | Outer while-loop at src/PDFDoc.php:1166-1280; spec REQ-006 added |
| 🔴 Phantom REQ-4 (task 5.5) | Reverted to [ ] with deferral note for PR #10 |
| 🔴 Missing isset check | Explicit guard at lines 1270-1273 |
| 🔴 Odd-length hex silently dropped | Pad-with-zero at lines 1409-1411; explicit false check |
| 🔴 Numeric tokenizer scientific notation | Restricted to -+.0-9 (lines 1418, 1428) |
🟡 Dead $matchedOnce |
Removed by rewrite |
| 🟡 Comments not as whitespace | Now skipped per PDF 1.7 §7.2.4 (lines 1377-1382) |
🟡 tj_arrays_modified contract fragile |
Pre-initialised at line 825 |
🟡 preg_match per-byte |
Replaced with strpos against 6-byte PDF whitespace set |
| 🟡 Shape inheritance asymmetric | Placeholder now always 'literal' (line 1252) |
| 🟡 Triplicate Implementation Note | Canonical pointers in all 3 docs files |
| 🟡 Numbering scheme drift | "Stacked on (#6)" line removed in triplicate sweep |
| 🟡 Phantom-ticked task 5.4 | Reverted to [ ] with deferral note |
🟡 Defensive @ on hex2bin |
@ removed; explicit false check |
| 🟡 Tf set inside q/Q leaks | Acknowledged as deferred gap in fix-commit message |
🟢 end()/reset() pointer-advancing |
New group-based render uses numeric indexing |
| 🟢 Edge-case completeness note | Was note-only; no fix needed |
Please address the new findings + the still-open 🟡 carry-overs before requesting another review. Strict-mode holds open for any 🟡.
Blocker:
- spec.md REQ-005 (parser tightening) and REQ-006 (multi-match in
same TJ) were added in the first-pass fix without scenarios under
examples/ — violating the spec's "Every requirement below MUST be
exercised by at least one scenario in the change's verification
gate" clause.
PoC now adds:
REQ-005 — parseTjArrayContent invoked via Reflection (the
contract is purely about token-level parsing, no full PDF needed):
* Exponent notation `1e2` MUST cause parse-bail (return null)
* Odd-length hex `<41B>` MUST decode to bytes 0x41 0xB0
* `(A) % comment\n -2 (B)` MUST emit [text:A,kern:-2,text:B]
* VT (0x0B) MUST cause parse-bail (not in §7.2.3 whitespace set)
* All 6 spec-defined WS bytes between fragments MUST be skipped
REQ-006 — full PDF round-trip via runReplace():
* Two different needles in one TJ → both placeholders land,
both per-needle counters hit 1, tj_arrays_modified == 1
* Same needle twice in one TJ → 2 placeholders, counter = 2
Concerns:
- Task 5.3 phantom-tick (PoC shelled out to poc-replace-text.php
and only checked $exit !== 0, never reading tj_arrays_modified)
now actually enforced: poc-replace-text.php emits a
`tj_arrays_modified: <N>` line on stdout, and the TJ PoC parses
it from the child's captured output. Fails loud if the value is
non-zero OR the key is missing entirely (diagnostic-surface drift).
- tasks.md previously had no entries tracking the shipped REQ-005
parser-tightening or the REQ-006 multi-match loop (the strict-
review fix code in parseTjArrayContent + processTjArray's outer
while-loop). Added 2.6 (parser tightening) and 3.6 (multi-match
loop) with explicit references to the scenarios that exercise
them. Added 5.6 (REQ-005 scenarios) and 5.7 (REQ-006 scenarios).
Verification: PoC now runs 8 named scenarios (was 5); result line
updated to "all 6 REQs covered". All other PoC gates still green
including the child poc-replace-text.php which gained the new
diagnostic line.
| // Emit the TJ-arrays counter so downstream verification gates can | ||
| // assert "Tj-only fixture leaves tj_arrays_modified == 0" without | ||
| // reaching into the doc surface twice. | ||
| echo " tj_arrays_modified: " . ($stats['tj_arrays_modified'] ?? 0) . "\n\n"; |
There was a problem hiding this comment.
🟢 Minor — ?? 0 fallback defeats the missing-key drift-detection branch
The ca1c2f5f commit message states: "Fails loud if the value is non-zero OR the key is missing entirely (diagnostic-surface drift)." This emit uses ($stats['tj_arrays_modified'] ?? 0), so the line is always present in the child PoC's stdout. The parent gate's missing-key branch (examples/poc-tj-flattening.php:383) therefore can't fire: if replace_text_in_document ever drops the pre-init at src/PDFDoc.php:825, the child still emits tj_arrays_modified: 0, the parent's [0-9]+ regex matches, and the drift goes undetected.
The Tj-only zero-check still works (this is the primary regression you want to catch), so this is non-blocking. Worth tightening as a follow-up so the secondary contract guard actually fires:
echo " tj_arrays_modified: " . (array_key_exists('tj_arrays_modified', $stats) ? $stats['tj_arrays_modified'] : 'MISSING') . "\n\n";Then the parent's [0-9]+ regex falls through to the missing-key branch when the implementation forgets the pre-init.
| * ---------------------------------------------------------------- */ | ||
| { | ||
| // Concatenated text: "Jan Jansen meets Karel Karelsen here" | ||
| $pdf = buildTjFixture('', ''); // ignored; we build our own below |
There was a problem hiding this comment.
🟢 Minor — Dead buildTjFixture('', '') call
This line's result is immediately overwritten on line 310. The // ignored; we build our own below comment suggests the author intended a different construction path here, but the line below just reuses buildTjFixture with non-empty args — there's no separate fixture builder. Drop this line (and both surrounding comments, since the next line's args are self-explanatory).
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
🟢 APPROVE — All three findings from my second-pass strict review are addressed in ca1c2f5f.
Resolved
- 🔴 REQ-005 and REQ-006 added without scenarios → 6 new scenarios in
examples/poc-tj-flattening.php: 4 parser-level viaparseTjArrayContentreflection (exponent-bail on1e2, odd-length hex<41B>→\x41\xB0,%-comments treated as whitespace, VT (0x0B) bails + all 6 spec WS bytes skipped) and 2 full-PDF round-trips (two-needles-one-TJ + same-needle-twice). - 🟡 Task 5.3 phantom-tick →
examples/poc-replace-text.php:85now emitstj_arrays_modified: N; parent gate atexamples/poc-tj-flattening.php:373parses the child's stdout and fails on non-zero or missing. - 🟡 tasks.md missing REQ-005/006 entries → tasks 2.6 (parser tightening), 3.6 (multi-match loop), 5.6 (REQ-005 scenarios), 5.7 (REQ-006 scenarios) added with explicit REQ references.
Follow-ups (non-blocking, posted inline)
- 🟢
?? 0fallback defeats the missing-key drift-detection branch — the primary Tj-only zero-check still fires, only the secondary contract guard is unreachable. - 🟢 Dead
buildTjFixture('', '')call at the top of the REQ-6 two-needles scenario.
Still open from the first-pass review (4 🟡 + 4 🟢): author deferrals — not raising the bar in this re-review.
feat(tj): TJ-array flattening + multi-match in same TJ (SAPP PR #7)
2 participants
Summary
Adds TJ operator support to the text-space matcher from #8. Needles spanning TJ fragment boundaries now match — covers the Word-emitted character-level-kerning pattern (
[(J) -2 (a) -1 (n) ...] TJ) that's typical for Dutch government Woo documents.Stacked on #8 feat/tounicode-cmap.
Splice algorithm (D2)
Four output shapes selected by match position:
(placeholder) Tj(placeholder) Tj [<remaining>] TJ[<leading>] TJ (placeholder) Tj[<leading>] TJ (placeholder) Tj [<trailing>] TJKerning OUTSIDE the match span is preserved exactly (visual appearance of surrounding text unchanged). Kerning INSIDE the match is discarded — micro-kerning was character-specific to the original text and doesn't apply to the synthetic placeholder. Trailing/leading kerning in the surrounding arrays is trimmed to avoid degenerate
[... -5]shapes.What landed (695 +, 60 −)
PDFDoc)processTjArray[...]operand, parses entries, matches needles, returns splice tupleparseTjArrayContenttext(bytes + shape) andkern(numeric token verbatim) entriesrenderTjArray/renderTjFragmentSpec contract
Pinned by
openspec/changes/feat-tj-flattening/:cid_split_mismatchtj_arrays_modified: intTest plan
php examples/poc-tj-flattening.php— synthesises minimal PDFs with character-level TJ kerning; verifies all 4 splice shapes end-to-endphp -lclean on touched filesDependencies + ordering
Out of scope