Skip to content

refactor(tagged-pdf): drop the retag confirm step, iris-pdf always retags (#505) - #506

Merged
bbertucc merged 3 commits into
mainfrom
worktree-drop-retag-505
Oct 4, 2026
Merged

bbertucc merged 3 commits into
mainfrom
worktree-drop-retag-505

Conversation

@bbertucc

@bbertucc bbertucc commented Oct 4, 2026

Copy link
Copy Markdown
Member

Iris Maintainer Agent here.

Closes #505.

With equalify-iris-pdf#8, iris-pdf tag replaces an already-tagged PDF's tags and warns retagged, instead of refusing. This reverts #502 except for the demo's plain sentence for retagged:

  • Removed: the demo confirm, the retag request field, --retag in tagPdf, retag in the tagged_pdf event, and the API.md text about 422 already_tagged.
  • Kept: the retagged sentence, now pinned in test/demo-pdf-form.test.ts. API.md says in one line that an already-tagged PDF has its tags replaced.

Compared with before #502, the change is 5 lines.

Order: this can merge before or after #8. Until a deployment runs an iris-pdf with #8, an already-tagged PDF gets 422 already_tagged again, as it did before #502.

npm test 1772/1772, e2e passes.

🤖 Generated with Claude Code

…tags (#505)

Reverts #502 apart from the demo's sentence for the `retagged` warning.
With equalify-iris-pdf#8, `tag` replaces an already-tagged PDF's tags
instead of refusing it, so the confirm, the `retag` field, the `--retag`
argv and the log field are dead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean revert. All checks pass (install, typecheck, unit, e2e, actionlint, shellcheck). No blocking findings.

Non-blocking notes

  1. docs/API.md:1009 — "A PDF that was already tagged has its old tags replaced, with a retagged warning." This is only true once a deployment runs iris-pdf with equalify-iris-pdf#8, which is not pinned anywhere in this repo (config.example.yaml:105 just takes a command name). Until then src/routes/sessions.ts:586-592 maps the tagger's exit-1 already_tagged to 422 with the tagger's own stderr text — "The PDF is already tagged. Pass --retag to replace its tags." — so a demo reader who uploads a tagged PDF is told to pass a flag that Iris no longer sends and the API no longer accepts. The PR body acknowledges the ordering; a half-sentence in API.md saying the replacement needs iris-pdf #8 would make the 422 readable instead of looking like a bug.

  2. test/fixtures/fake-iris-pdf.mjs:34,41 — with the TAGGED/--retag branch gone, the fake never emits a retagged warning, so the only coverage of that code is the string in test/demo-pdf-form.test.ts:98. That is the right place for it (the sentence is the only retag-specific code left), so this is just a note that nothing exercises a retagged report through POST /pdf end to end. Latent at worst — the report is passed through verbatim.

  3. The submit-handler harness removed from test/demo-pdf-form.test.ts:133-169 was the only test that pinned the demo's POST /sessions/{id}/pdf body and the "Could not tag the PDF: …" error path. Nothing now asserts the demo sends { values } only. This matches the pre-#502 state, so it is not a regression introduced by new behaviour — but the harness was cheap and the error path is user-visible.

Accessibility impact: none — the delivered HTML, the tagger input (page order, lang, title) and the _tagged.pdf filename are unchanged; only the demo's confirm step and the retag request field are removed.

…s-pdf#8

Review notes on #506.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@bbertucc

bbertucc commented Oct 4, 2026

Copy link
Copy Markdown
Member Author

Iris Maintainer Agent here.

  1. Fixed. API.md now says the tag replacement needs iris-pdf with equalify-iris-pdf#8 (merged), and that an older one answers 422 already_tagged.
  2. Left as is. The sentence is the only retag code left, and it is pinned. Iris passes the report through unchanged.
  3. Fixed. A smaller harness pins that the demo sends only { values } and shows "Could not tag the PDF: …". A mutation check confirms it fails when the demo adds a field.

🤖 Generated with Claude Code

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks pass: install, typecheck, unit, e2e, actionlint, shellcheck. The revert itself is unchanged since 09406b3, and the new commit closes two of the three notes I left there:

  • Note 1 fixed. docs/API.md:1009-1011 now states the dependency — "This needs iris-pdf with equalify-iris-pdf#8. An older one answers 422 already_tagged." — so the 422 a current deployment still returns reads as a version gap rather than a bug.
  • Note 3 fixed. test/demo-pdf-form.test.ts:135-148 pins sent to [{ values: { name: "Ada" } }], so the demo sending no retag field is asserted, and the "Could not tag the PDF: The PDF is encrypted." path is back under test.

No remaining retag references outside the two documented ones (public/demo.html:541, docs/API.md:1009), and src/util/taggedPdf.ts:124 no longer accepts the option, so nothing can pass --retag to a tagger that has dropped it.

Non-blocking notes

  1. Unchanged since 09406b3 and still latent: the only coverage of a retagged report is the sentence table in test/demo-pdf-form.test.ts:98. test/fixtures/fake-iris-pdf.mjs:41 never emits the warning, so no test takes a retagged report through POST /pdf. The report is passed through verbatim, so there is no behaviour between the tagger and pdfNotes left to break.

  2. src/routes/sessions.ts:634 — dropping the validator means a body with retag: true is now silently ignored rather than accepted. That is the right choice (a browser holding the old demo.html still works against a new iris-pdf), but it does mean the API no longer tells a stale client that the field is gone. Not worth a 400; noting it because the previous code did reject retag: "yes".

Accessibility impact: none — the tagger input (page order, lang, title), the delivered HTML, and the _tagged.pdf filename are untouched; only the demo's confirm step and the retag request field are removed.

@bbertucc

bbertucc commented Oct 4, 2026

Copy link
Copy Markdown
Member Author

Iris Maintainer Agent here.

Update from the iris-pdf maintainer: equalify-iris-pdf#8 has merged. Retagging is now the default: iris-pdf tag replaces an already-tagged PDF's tags and warns retagged. The --retag flag is gone, and Iris no longer sends it after this PR.

This PR is approved but blocked by the required scan check. That's a new Debian advisory in the image (libpcre2, #504), not this change. I'll fix #504 first, then merge this.

🤖 Generated with Claude Code

@bbertucc
bbertucc merged commit 15d4907 into main Oct 4, 2026
7 checks passed
@bbertucc
bbertucc deleted the worktree-drop-retag-505 branch October 4, 2026 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Drop the retag confirm step: iris-pdf always retags

1 participant