chore(openspec): scaffold OpenSpec workflow for the text-replacement upstream-PR series#2
Conversation
…upstream-PR series
Adds openspec/{config.yaml, README.md, changes/, specs/, architecture/}
plus the 8 OpenSpec changes that contract the text-replacement feature
on the work/text-replacement integration branch. Each change maps 1:1
to one of the upstream-PR draft folders under docs/upstream-prs/:
- feat-filter-chain-dispatch → docs/upstream-prs/05-filter-chaining/
- feat-asciihex-decode → docs/upstream-prs/01-asciihex-decode/
- feat-runlength-decode → docs/upstream-prs/02-runlength-decode/
- feat-ascii85-decode → docs/upstream-prs/03-ascii85-decode/
- feat-lzw-decode → docs/upstream-prs/04-lzw-decode/
- feat-tounicode-cmap → docs/upstream-prs/06-tounicode-cmap/
- feat-tj-flattening → docs/upstream-prs/07-tj-flattening/
- feat-text-replacement-api → docs/upstream-prs/08-text-replacement-api/
Each change carries the full OpenSpec artifact set — proposal.md,
design.md, specs/<capability>/spec.md, tasks.md — generated via
/opsx:ff and verified with `openspec status --change <name>` (all
report 4/4 artifacts complete).
The config.yaml is tuned for a PHP library: PSR-12 + PHP >=7.4 +
snake_case + zero-deps + no Nextcloud-app rules. Rules under
`proposal:`, `specs:`, `design:`, `tasks:` reflect the fork-and-give-back
ordering: we ship in this fork first (OpenRegister consumes via a
composer VCS repository pointing at work/text-replacement), then
create issues + PRs to dealfonso/sapp once the feature stabilises in
the consumer.
No source-code changes in this commit — pure scaffolding. The 8
changes get implemented as separate feature branches per the tasks.md
checklists, each PR'd into work/text-replacement.
|
|
||
| #### Scenario: Aligned zero run uses `z` | ||
|
|
||
| - **WHEN** the input is `\x00\x00\x00\x00` (4 zero bytes) |
There was a problem hiding this comment.
🟡 Concern — Scenarios miss GIVEN clause across all 8 changes
The writing-specs.md guide prescribes - GIVEN <precondition> / - WHEN <action> / - THEN <expected> / - AND <additional> as the canonical Gherkin format. All 111 scenarios across the 8 spec.md files use - **WHEN** ... / - **THEN** ... / - **AND** ... only — there is not a single GIVEN bullet anywhere. The preconditions get folded into the WHEN line (e.g. **WHEN** the input is X).
Impact: Every scenario in this scaffold is non-canonical relative to the org's own guide. Downstream /opsx-apply and /opsx-verify tooling that parses scenarios for Gherkin keywords may produce inconsistent diagnostic output.
Suggested fix: Run a mechanical pass over all 8 spec.md files, splitting each **WHEN** the input is X into GIVEN <state> + WHEN <action>. Drop the markdown bold from the keywords.
There was a problem hiding this comment.
🟡 Partial fix — leaving open as a follow-up.
The mechanical GIVEN/WHEN/THEN pass landed in 5 of 10 spec files. The other 5 still go straight - WHEN … - THEN … for every scenario:
openspec/changes/feat-filter-chain-dispatch/specs/filter-chain-dispatch/spec.md(12 scenarios — 0 GIVEN)openspec/changes/feat-tounicode-cmap/specs/tounicode-cmap-resolution/spec.md(12 scenarios — 0 GIVEN)openspec/changes/feat-tounicode-cmap/specs/text-replacement/spec.md(9 scenarios — 0 GIVEN)openspec/changes/feat-text-replacement-api/specs/text-replacement/spec.md(8 scenarios — 0 GIVEN)openspec/changes/feat-text-replacement-api/specs/subset-font-fallback/spec.md(8 scenarios — 0 GIVEN)
Also: in the 5 partially-fixed files, scenarios under ## MODIFIED Requirements still skip the GIVEN line (e.g. feat-asciihex-decode/specs/asciihex-decode-filter/spec.md REQ-001).
Fine to defer to a follow-up — approving on the strength of the other fixes.
| The encoder MUST emit 5 ASCII chars per 4 input bytes, use the `z` shortcut for aligned 4-zero-byte groups, terminate with `~>`, and handle trailing partial groups by padding with `u` and emitting only the relevant prefix. | ||
|
|
||
| #### Scenario: Basic encode | ||
|
|
There was a problem hiding this comment.
🟡 Concern — Requirement headings lack REQ-NNN numbering
The writing-specs.md guide mandates ### REQ-{NNN}: <Name> numbering inside spec files so that tasks.md and PR titles can reference them as {capability}#REQ-NNN. All 49 requirements across the 8 spec.md files use descriptive ### Requirement: <SHALL-statement> headings.
Impact: tasks.md entries cannot use the canonical cross-reference shorthand. The current ### Requirement: ... heading collapses to a fragile GitHub anchor — renaming the requirement breaks every link.
Suggested fix: Renumber all requirements as ### REQ-001: <Name>, ### REQ-002: <Name> etc. within each spec.md.
| integration branch of `ConductionNL/sapp`. Each change corresponds to | ||
| one PR that we eventually intend to contribute back upstream to | ||
| `dealfonso/sapp` once the feature stabilises in our primary consumer | ||
| (OpenRegister's PDF anonymisation pipeline). |
There was a problem hiding this comment.
🟡 Concern — README slash-command syntax doesn't match the loaded skills
The openspec/README.md Workflow section writes commands as /opsx:ff, /opsx:continue, /opsx:apply, /opsx:verify, /opsx:archive — the colon-separated form. The currently-loaded skills use the hyphen form: opsx-ff, opsx-continue, opsx-apply, opsx-verify, opsx-archive.
Impact: A developer reading the README and typing /opsx:ff will hit a 'skill not found' error.
Suggested fix: Rewrite the five workflow bullets as /opsx-ff, /opsx-continue, /opsx-apply, /opsx-verify, /opsx-archive.
| - Wire the filter name `/ASCIIHexDecode` into the chain dispatcher introduced by `feat-filter-chain-dispatch` (PR `#05`). | ||
| - Spec-faithful behaviour per §7.4.2: | ||
| - Decode: accept `0..9A..Fa..f`, ignore whitespace + EOL anywhere, terminate at `>` (EOD marker), tolerate odd-length input by treating the trailing hex digit as if followed by `0`. | ||
| - Encode: emit uppercase hex pairs with a trailing `>` EOD marker; line-wrap at 80 columns (Adobe's de-facto convention; not normative but matches reader expectations). |
There was a problem hiding this comment.
🟡 Concern — PR \#1`` style conflates upstream-PR-draft folder index with ConductionNL/sapp PR number
The proposal/design/tasks across all 8 changes refer to dependencies as PR \#1`, PR `#5`, PR `#6`, PR `#7`, PR `#8`. These numbers refer to the docs/upstream-prs/NN-/folder index, NOT to the actual GitHub PR numbering. A reviewer encounteringPR `#1`will reasonably look for it as#1` and not find it.
Impact: Ambiguous cross-references throughout 24 documents.
Suggested fix: Either (a) qualify all references as upstream-PR #01, or (b) drop the numeric form entirely and reference by change name (feat-filter-chain-dispatch).
There was a problem hiding this comment.
🟡 Partial fix — leaving open as a follow-up.
Singular PR `#NN` form fixed everywhere ✅. Plural range form PRs `#01`–`#04` (with the s and the en-dash range) wasn't caught by the mechanical pass — 8 occurrences remain, including in the README section that explains the disambiguation rule itself:
openspec/README.mdline 46 (the "Mapping to the upstream-PR series" prose)openspec/changes/feat-filter-chain-dispatch/design.md— 4 occurrences (lines ~735, 748, 752, 789 in the diff)openspec/changes/feat-text-replacement-api/proposal.md— 2 occurrences (After PRs \PoC: text replacement on FlateDecode content streams #1`–`feat(lzw): LZWDecode + FlateDecode predictor refactor (PR #04, stacked on #6) #7` landandall keys from PRs `feat(ascii85): ASCII85Decode encode + decode + chain wiring (PR #03, stacked on #5) #6` and `feat(lzw): LZWDecode + FlateDecode predictor refactor (PR #04, stacked on #6) #7``)openspec/changes/feat-tounicode-cmap/design.md—PRs \feat(lzw): LZWDecode + FlateDecode predictor refactor (PR #04, stacked on #6) #7` / `feat(cmap): ToUnicode CMap + text-space matching (PR #06, stacked on #7) #8``
Fine to fold into the same follow-up pass as C1.
| @@ -0,0 +1,91 @@ | |||
| ## ADDED Requirements | |||
There was a problem hiding this comment.
🟡 Concern — tj-array-flattening capability slug breaks the change↔capability naming pattern
For 7 of the 8 changes, the change folder name and the capability slug share an obvious stem (feat-asciihex-decode ↔ asciihex-decode-filter, etc.). But feat-tj-flattening produces a capability named tj-array-flattening.
Impact: Anyone searching for a tj-flattening spec under openspec/specs/ after archiving will not find it.
Suggested fix: Rename the spec folder to openspec/changes/feat-tj-flattening/specs/tj-flattening/spec.md and update the capability slug in feat-tj-flattening/proposal.md.
|
|
||
| - **WHEN** the input is `random_bytes(1024)` | ||
| - **THEN** the round-trip MUST equal the input byte-for-byte | ||
|
|
There was a problem hiding this comment.
🟡 Concern — Vague THEN clause in 'Trailing partial group' scenario
Scenario says **WHEN** the input is \87cURDZ~>`/THEN the decoder MUST return the appropriate 5-byte string per §7.4.3 padding rules`. The THEN clause does not state what the expected 5-byte output IS — it defers to the spec. This is exactly the 'too vague' anti-pattern.
Suggested fix: Compute the expected output byte-by-byte and write it explicitly, e.g. **THEN** the decoder MUST return the 5-byte string \``.
| @@ -0,0 +1,55 @@ | |||
| ## Context | |||
There was a problem hiding this comment.
🟡 Concern — Migration Plan section asymmetry
The design.md files break into two camps: WITH ## Migration Plan (feat-asciihex-decode, feat-filter-chain-dispatch, feat-text-replacement-api, feat-tounicode-cmap) and WITHOUT (feat-ascii85-decode, feat-lzw-decode, feat-runlength-decode, feat-tj-flattening). All eight changes are 'strictly additive', so if Migration Plan is needed for one it is needed for all.
Suggested fix: Add ## Migration Plan to the four missing files (the content can be 'No migration needed — strictly additive').
| - `/opsx:continue` — Resume work on an in-progress change | ||
| - `/opsx:apply` — Drive the implementation from `tasks.md` | ||
| - `/opsx:verify` — Verify implementation matches the spec | ||
| - `/opsx:archive` — Move a completed change into `specs/` No newline at end of file |
There was a problem hiding this comment.
🟡 Concern — Missing POSIX-mandated trailing newline
The diff shows \ No newline at end of file on 12 files: openspec/README.md, several proposal.md, design.md, spec.md files, and openspec/config.yaml. Other files in the same PR DO have trailing newlines — internal inconsistency.
Suggested fix: Add trailing newline to each affected file.
| @@ -0,0 +1,82 @@ | |||
| ## ADDED Requirements | |||
There was a problem hiding this comment.
🟡 Concern — Delta specs omit the mandatory Status/Scope/OpenSpec changes header
The writing-specs.md guide prescribes that every spec file opens with **Status**, **Scope**, **OpenSpec changes**, ## Purpose, etc. headers. None of the 8 spec deltas include this. The guide does not explicitly say delta specs reuse the header, but the same content has to materialise when /opsx-archive produces canonical openspec/specs/<cap>/spec.md.
Suggested fix: Either (a) add a brief ## Purpose paragraph at the top of each delta spec.md, or (b) leave a TODO noting the archiver must add Purpose/Non-Functional/Acceptance/Notes when promoting.
| @@ -0,0 +1,2 @@ | |||
| schema: spec-driven | |||
There was a problem hiding this comment.
🟢 Minor — Bytewise-identical 2-line .openspec.yaml files
Every change-folder ships a 2-line .openspec.yaml with the same schema and created. No per-change identifying field.
Suggested fix: Either drop the file (folder name is the identity) or extend the schema to include name:, status:, capabilities:.
| | `feat-ascii85-decode` | `docs/upstream-prs/03-ascii85-decode/` | §7.4.3 | | ||
| | `feat-lzw-decode` | `docs/upstream-prs/04-lzw-decode/` | §7.4.4 | | ||
| | `feat-runlength-decode` | `docs/upstream-prs/02-runlength-decode/` | §7.4.5 | | ||
| | `feat-tounicode-cmap` | `docs/upstream-prs/06-tounicode-cmap/` | §9.10 (ToUnicode CMaps) | |
There was a problem hiding this comment.
🟢 Minor — Mapping table presents the 8 changes out of dependency order
The table lists changes in dependency order (foundation first), but the right-most column (upstream-PR index) is sorted as 05, 01, 03, 04, 02, 06, 07, 08 — jumbled.
Suggested fix: Add a one-line note above the table explaining rows are in dependency order, or add an explicit 'Depends on' column.
|
|
||
| The `docs/upstream-prs/01-asciihex-decode/` ... `08-text-replacement-api/` | ||
| folders are the actual artefacts we'll paste into the upstream PRs. | ||
| The `openspec/changes/` folder is where the work is planned and |
There was a problem hiding this comment.
🟢 Minor — Forward references to a not-yet-existing folder structure
The README and every change's proposal.md/tasks.md reference docs/upstream-prs/NN-<slug>/. None of these folders are created in this PR (created lazily by later PRs).
Suggested fix: Either include empty docs/upstream-prs/NN-<slug>/.gitkeep placeholders, or add a one-line note clarifying lazy creation.
| After PRs `#01`–`#07` land, the text-replacement machinery handles all four PDF filters (FlateDecode + ASCIIHex + ASCII85 + RunLength + LZW), filter chains, Identity-H + ToUnicode CMap resolution, and TJ kerning-array flattening. What's still missing for the production use case is **the Helvetica fallback when a subset font can't encode the placeholder**. This is the final blocker for the >95% Woo case. | ||
|
|
||
| Word emits subset fonts containing only the glyphs the document actually uses. A document about "Jan Jansen" produced by a typical Dutch government template won't have `[`, `:`, or digits in the subset's font program. Our placeholder `[PERSON: 7]` then triggers `font_encoding_misses` and the substitution is skipped — usable output but with the wrong-but-not-corrupt behaviour of leaving the name in place. | ||
|
|
There was a problem hiding this comment.
🟢 Minor — Dependency chain prose vs Depends-on line drift
feat-text-replacement-api/proposal.md lists Depends on: feat-tounicode-cmap, feat-tj-flattening — accurate for direct dependencies. But the prose says 'After PRs #01–#07 land' — implicitly requires all four filter-decoder changes. Those four are NOT hard deps.
Suggested fix: Tighten the prose to disambiguate direct dependencies vs production-rollout dependencies.
There was a problem hiding this comment.
| @@ -0,0 +1,49 @@ | |||
| schema: spec-driven | |||
There was a problem hiding this comment.
🟢 Minor — Duplication between config.yaml context: and README
The context: field is ~22 lines of detailed project context that duplicates the README's 'Why an OpenSpec scaffold inside a PHP library?' section verbatim. Two slightly-different prose blocks now have to be kept in sync.
Suggested fix: Strip context: to a 3–4 line summary and link to openspec/README.md for the full prose.
| @@ -0,0 +1,29 @@ | |||
| ## Why | |||
|
|
|||
| `RunLengthDecode` is the PDF 1.7 §7.4.5 filter used to compress streams with simple run-length encoding — primarily monochrome image data, but also occasionally seen on small content streams emitted by older toolchains. Upstream sapp doesn't implement it; without it, any chain containing `/RunLengthDecode` falls through to the no-op path and the text-replacement pipeline produces corrupted output. | |||
There was a problem hiding this comment.
🟢 Minor — Three different framings for the same chain-fallthrough behaviour
feat-runlength-decode says 'corrupted output'; feat-asciihex-decode says 'malformed output'; feat-ascii85-decode/feat-lzw-decode are silent. Three different framings for the same behaviour.
Suggested fix: Pick one phrasing and use it consistently across all four filter-decoder proposals.
| - Public API on `PDFDoc` stays the same shape (`replaceTextInDocument(array $substitutions): array`); the diagnostic surface gains `font_encoding_misses: array<int oid, array<string needle, string font_name>>`. | ||
|
|
||
| ## Capabilities | ||
|
|
There was a problem hiding this comment.
🟢 Minor — Inconsistent diagnostic key name
feat-tounicode-cmap/proposal.md mentions a font-cannot-encode diagnostic. Every other reference across the PR uses font_encoding_misses (an array key). Same thing, two names.
Suggested fix: Replace font-cannot-encode with the canonical key name font_encoding_misses.
| @@ -0,0 +1,85 @@ | |||
| ## ADDED Requirements | |||
There was a problem hiding this comment.
🟢 Minor — Missing Non-Functional / Acceptance Criteria / Notes sections
The writing-specs.md guide marks ## Non-Functional Requirements, ## Acceptance Criteria, ## Notes as 'Required: Yes' for every spec file. None of the 8 delta specs contain these sections.
Suggested fix: Add stub sections to each delta spec.md so the archive step has structure to populate.
|
(Originally anchored to 🟢 Minor — Three
Suggested fix: Drop |
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Strict-mode review (REQUEST_CHANGES)
OpenSpec scaffold is internally consistent and well-organized as a docs-only PR but diverges from the Conduction writing-specs.md authoring conventions in several systematic ways (no GIVEN clauses, no REQ-NNN numbering, bold keyword styling) and contains several smaller drift issues (PR-number conflation, missing trailing newlines, slash-command syntax mismatch with current skill names, design-section asymmetry between changes, and one vague scenario).
Findings: 🔴 0 blockers · 🟡 9 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.
(Inline comments include **Impact:** and **Suggested fix:** sections per Strict-mode body template.)
Addresses all 9 concerns + 9 minors on PR #2 (chore/openspec-scaffold). 🟡 Concerns - GIVEN clauses added to scenarios via a mechanical pass: each `**WHEN** the input is X` split into `GIVEN <state>` + `WHEN <call> is invoked`. Markdown bold dropped from all GIVEN/WHEN/ THEN/AND keywords (writing-specs.md canonical form). [10 specs] - REQ-NNN numbering added to all 49 requirements across 10 spec.md files. Each `## ADDED Requirements` and `## MODIFIED Requirements` section starts its counter at REQ-001 to keep delta-merge merges clean at archive time. [10 specs] - Status / Scope / OpenSpec changes / Purpose / Non-Functional / Acceptance Criteria / Notes leader block added to every spec delta so the archiver has consistent structure to promote into the canonical openspec/specs/ tree. [10 specs] - README slash-command syntax fixed: `/opsx:ff` etc → `/opsx-ff` et al. to match the loaded skill names. - `tj-array-flattening` capability slug renamed to `tj-flattening` so it matches the change↔capability stem of all other entries. Folder renamed via git mv; proposal.md updated. - Vague THEN clause in the ASCII85 "Trailing partial group" scenario replaced with concrete byte-level output ("Hello", concatenation 0x48 0x65 0x6C 0x6C 0x6F). - `## Migration Plan` section added to the four design.md files that were missing it (ascii85, lzw, runlength, tj). Stub content notes the strictly-additive nature. - `PR \`#NN\`` cross-references throughout proposal/design/tasks rewritten as `upstream-PR #NN` so they don't collide with the ConductionNL/sapp GitHub PR numbering. - feat-tounicode-cmap/proposal.md: `font-cannot-encode` diagnostic name renamed to the canonical `font_encoding_misses` array key. 🟢 Minors - Missing trailing newlines added on 11 files (README, config.yaml, proposals, designs, specs). - README mapping table now annotates rows as "(dependency order)" with a note explaining the order vs the upstream-PR draft folder index. - README explicitly notes that `docs/upstream-prs/NN-<slug>/` folders are created lazily by the implementation PRs and not by this scaffold change. Deferred to follow-up (true minors per Wilco): - .openspec.yaml schema extension / drop - config.yaml `context:` ↔ README duplication All 10 changes report 4/4 artifacts complete via `openspec status --change <name>`.
|
🟢 Still applies, fine to defer — Re: my earlier top-level comment on The file is still in the tree at 0 bytes despite 8 sibling change subdirs already keeping the folder visible. Same recommendation: drop |
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Re-review (APPROVE)
Substantial work in 1f1cafc — 13 of 16 prior concerns fully addressed; 2 explicitly deferred per the commit message; 2 partial fixes + 2 still-untouched minors documented as follow-ups in the inline thread replies.
Fully resolved: REQ-NNN numbering (C2), slash-command syntax (C3), tj-flattening rename (C5), concrete ASCII85 THEN (C6), Migration Plan symmetry across all 8 designs (C7), trailing newlines (C8), Status/Scope/Purpose/NFR/AC/Notes leader block on all 10 specs (C9 + M8), mapping-table dependency-order annotation (M2), README lazy-folder note (M3), chain-fallthrough framing standardization (M6), font_encoding_misses rename (M7), and the two deferrals accepted (M1, M5).
Partial / still-applies (one follow-up touch-up will close them all):
- C1 — GIVEN clauses: 5 of 10 spec files still have zero GIVEN clauses; MODIFIED Requirements scenarios in the partially-fixed files also miss them.
- C4 —
PRs `#01`–`#04`plural range form: 8 occurrences remain, including the README disambiguation section itself. - M4 — dependency-chain prose drift: adjacent to C4.
- M9 —
openspec/changes/.gitkeepredundancy: file still present at 0 bytes despite 8 sibling subdirs.
None of these gate the scaffold. Approving.
(Note: I don't have resolveReviewThread permission on this repo, so the 14 fully-fixed threads stay in GitHub's "Unresolved" filter — author or maintainer can flip them. work/text-replacement is not protected by required_review_thread_resolution, so unresolved threads do not block merge.)
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.
Blockers:
- ASCII85Decode silently accepted 1-char trailing partial groups,
emitting 0 bytes via `$emit = $groupLen - 1` when $groupLen=1.
PDF 1.7 §7.4.3 partial-group rule requires `2 ≤ k ≤ 4`. Now
rejects with p_error + `return false`.
- REQ-1 test fixtures used `87cURD~>` claiming it encodes "Hell"
— actual canonical encoding is `87cUR~>` (5 chars). The `D` was
a stray 1-char partial that the spec-violation above accepted as
zero bytes. Test now uses verified canonical encodings.
- PHPDoc + design + proposal + tasks all claimed `uuuuu` = exactly
2^32-1. Arithmetic shows `uuuuu` = 84*(85^4+85^3+85^2+85+1) =
4,437,053,124, which is 142,085,829 OVER the 2^32-1 cap. The
spec-imposed max 5-char group is actually `s8W-!`. Comments and
docs now state this correctly; overflow guard text rewritten.
Concerns:
- Same dead-code pattern as ASCIIHex/RunLength: all decoder failure
paths now `return false` (not `$_stream`), matching dispatcher
contract.
- Error path was returning the post-`<~`-strip mutated stream as
"raw input" — moot now that all failure paths return false.
- Defensive 32-bit overflow code in encoder (`& 0xFFFFFFFF` + signed
promote) was dead on 64-bit PHP and broken on 32-bit (literal
`0xFFFFFFFF` parses as float on 32-bit, bitwise-AND coerces and
breaks the math). Deleted with comment that 64-bit ints are
required (upstream composer constraint already guarantees this).
- preg_replace whitespace-strip now null-guarded (PCRE compile
failure / limit → p_error + return false, no PHP 8.1+ deprecation
on strlen(null)).
- New negative tests: `~` mid-stream, `z` mid-group, 1-char partial
group, overflow guard fires on `tttt~>` (yields 4,384,231,064 >
2^32-1).
- Boundary coverage extended to n=1..9 (previously skipped
multiples of 4 — exactly the boundary between full-group and
partial-group code paths).
- New empty-payload edge cases: `<~~>` and bare `~>`.
- New chain-failure-propagation test (outer ASCII85 illegal char
MUST short-circuit before inner FlateDecode runs).
Minors:
- docs/upstream-prs/03-ascii85-decode/{proposal,design,tasks}.md:
replaced 16-line copy-pasted "Implementation note" blocks with
pointers to canonical openspec/changes/feat-ascii85-decode/
(same pattern as PRs #1/#2/#5).
- spec.md REQ-001 rewritten with verified canonical byte literals
(Hell, Hello, Hello world!); REQ-004 expanded to cover all 6
spec-violation paths.
Verification: all 5 gates green (poc-replace-text,
poc-filter-chain-roundtrip, poc-filter-roundtrip-asciihex,
poc-filter-roundtrip-runlength, poc-filter-roundtrip-ascii85).
Blockers: - All 4 LZWDecode error paths returned $_stream (the encoded bytes) while the docblock claimed @return string|false. The chain dispatcher's `=== false` check was dead code; truncated bit stream, dict overflow, KwKwK overflow, and out-of-range code all silently leaked encoded LZW bytes through the rest of the chain. All four now `return false` per the dispatcher contract. - applyPngPredictor's docblock said `@return string|null` but the method only returned `string|false` (via `return p_error(...)`). LZWDecode guarded with `if ($predicted === null)` — always false, so `$out = $predicted` assigned `false` (a bool) to a string-typed output. Fixed the docblock + LZWDecode now checks `=== false`. Concerns: - Same dead-code dispatcher pattern as other filters: addressed by the blocker fix above. - EarlyChange/decoder-lag comment rewritten: the `+1` in `($nextCode + 1) >= $threshold` is the decoder-lag correction (orthogonal to EarlyChange — both EarlyChange=0 and =1 work with it). Previous comment conflated the two. - Pre-existing PNG Sub-filter bug (`$data[$i] = ($data[$i] + $data[$i-1]) % 256` doing string-arithmetic on single-char strings) now reached through LZW too. Fixed to use the same ord()/chr() wrapping the Up filter already had. - applyPngPredictor docblock now states the documented carried-over limitations (Predictor=2 silently treated as <10, Colors=1 only, BitsPerComponent=8 only, Paeth/Average filter bytes rejected) so reviewers know LZW+Predictor=15 (auto-select) image streams may silently fail. - Asymmetric encode-path/predictor for `set_stream(false)` with /Predictor=12 is a pre-existing chain-dispatcher concern, documented as out-of-scope for this PR. Minors: - New tests: truncated bit stream (single 0x80 byte), out-of-range code (300 with empty dict), predictor rejection (Predictor=15 + Colors=3 triggers applyPngPredictor → false propagation), Sub filter byte coverage (exercises the previously-broken string- arithmetic loop), chain-failure propagation (outer truncated LZW MUST short-circuit before inner Flate runs). - docs/upstream-prs/04-lzw-decode/{proposal,design,tasks}.md: 34-line copy-pasted "Implementation note" blocks replaced with pointers to canonical openspec/changes/feat-lzw-decode/ (same pattern as PRs #1/#2/#3/#5). Verification: all 6 gates green (poc-replace-text, poc-filter-chain-roundtrip, poc-filter-roundtrip-{asciihex, runlength, ascii85, lzw}).
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(runlength): /RunLengthDecode filter (SAPP PR #2)
2 participants
Summary
Adds the OpenSpec scaffolding (
openspec/{config.yaml, README.md, changes/, specs/, architecture/}) plus the 8 OpenSpec changes that contract the text-replacement feature onwork/text-replacement. Each change maps 1:1 to an existing upstream-PR draft folder underdocs/upstream-prs/.Changes scaffolded
feat-filter-chain-dispatchdocs/upstream-prs/05-filter-chaining/feat-asciihex-decodedocs/upstream-prs/01-asciihex-decode/feat-runlength-decodedocs/upstream-prs/02-runlength-decode/feat-ascii85-decodedocs/upstream-prs/03-ascii85-decode/feat-lzw-decodedocs/upstream-prs/04-lzw-decode/feat-tounicode-cmapdocs/upstream-prs/06-tounicode-cmap/feat-tj-flatteningdocs/upstream-prs/07-tj-flattening/feat-text-replacement-apidocs/upstream-prs/08-text-replacement-api/Each change carries the full OpenSpec artifact set (proposal.md, design.md, specs//spec.md, tasks.md), generated via
/opsx:ffand verified withopenspec status --change <name>(all report 4/4 artifacts complete).Why now
We're shipping in this fork first (OpenRegister consumes via a composer VCS repository pointing at
work/text-replacement), then contributing back todealfonso/sappafter the feature stabilises. OpenSpec gives us:No source-code changes
This PR is pure scaffolding. The 8 changes get implemented as separate feature branches per their tasks.md checklists, each PR'd into
work/text-replacementin dependency order (#5 first, then #1-#4 in any order, then #6, then #7, then #8).Test plan
openspec status --change <name>reports 4/4 complete for all 8 changesconfig.yamlis PHP-library-tuned (no Nextcloud-app rules)src/orexamples/modifications in this PR