Skip to content

fix(1107): apply_redactions_destructive indirect reference to an indirect reference to an array - #1108

Open
ultrasaurus wants to merge 1 commit into
yfedoseev:mainfrom
ultrasaurus:get_page_content_bytes
Open

fix(1107): apply_redactions_destructive indirect reference to an indirect reference to an array#1108
ultrasaurus wants to merge 1 commit into
yfedoseev:mainfrom
ultrasaurus:get_page_content_bytes

Conversation

@ultrasaurus

@ultrasaurus ultrasaurus commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

apply_redactions_destructive fails on pages whose /Contents is an indirect reference to an array

Linked issue

Closes #1107

What and why

I should be able to call apply_redactions_destructive on any spec-compliant PDF

Type of change

  • Bug fix
  • New feature (has an accepted issue: #___)
  • Performance
  • Refactor / internal
  • Docs / CI / chore
  • Breaking change

Tests

  • I added a test that fails before this change and passes after
    (revert-checked). For bug fixes the reproducer is a minimal synthetic
    PDF built in code
    — no third-party/reporter PDF is committed.
  • Test named by defect class, not an issue/PR number; no
    contributor/company names in code or fixtures.
  • cargo test passes, and the affected feature tiers:
    rendering / fips,icc / ml / binding (list which): ____
  • cargo fmt --check and cargo clippy -- -D warnings are clean.

Regression on real PDFs

Corpus I tested: 32 PDFs total (including the PDF where I found this failure)

  • 20 IRS tax forms
  • 9 personal-library documents
  • 2 exported from Google Docs (public domain text w/ page headers, footers)
  • 1 large real-world scan — warandpeace030164mbp.pdf, 725 pages, from archive.org
  • Ran corpus_sig and diffed my branch against main
  • Diffed my branch against the latest release (v0.3.77)
  • [n/a] Judged with a structural metric (word-Jaccard + spacing outliers), not
    char-Levenshtein.

Diff summary vs main:

zero diff, no regressions

Diff summary vs latest release:

zero diff, no regressions

AI assistance disclosure

  • AI assistance: none, or assisted — tool: Claud, extent:draft code/test, wrote probe, ran corpus testing
  • I understand and can explain every line; the description and my review
    replies are written by me, not generated. This PR is not fully or
    predominantly AI-generated.

Checklist

  • One logical change (no bundled refactor/perf/correctness).
  • Commits follow Conventional Commits and are DCO signed-off (git commit -s).
  • [n/a] Docs/CHANGELOG.md updated if user-facing; reporters credited in the
    CHANGELOG (not in code).
  • [n/a] Public-API changes considered for semver (semver-checks will run).

@ultrasaurus
ultrasaurus requested a review from yfedoseev as a code owner August 21, 2026 17:11
@ultrasaurus
ultrasaurus marked this pull request as draft August 21, 2026 17:11
@ultrasaurus
ultrasaurus force-pushed the get_page_content_bytes branch from 3d9b512 to 201a33b Compare August 22, 2026 02:14
@ultrasaurus ultrasaurus changed the title bug(1107): failing test: apply_redactions_destructive indirect reference to an indirect reference to an array fix(1107): failing test: apply_redactions_destructive indirect reference to an indirect reference to an array Aug 22, 2026
@ultrasaurus
ultrasaurus force-pushed the get_page_content_bytes branch from 201a33b to d30f024 Compare August 22, 2026 02:46
@ultrasaurus
ultrasaurus marked this pull request as ready for review August 22, 2026 02:47
@github-actions

Copy link
Copy Markdown
Contributor

Closing automatically: this PR does not reference an issue.

Work here starts from an issue that a maintainer has approved and assigned —
opening a pull request is not how work is claimed. This is not about the quality
of the change: it costs a maintainer one line to say "go ahead" or "we would fix
that differently", and it saves you writing a change that was never going to land.

What to do:

  1. Open an issue describing the problem. A reproducer and one line is enough.
  2. Wait for a maintainer to comment /approve and assign it to you.
  3. Reopen this PR (or open a new one) with Closes #NNN in the description.

Typo, documentation, CI and revert PRs are exempt and can be opened directly.

See Before you open a pull request.

@github-actions github-actions Bot closed this Aug 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Closing automatically: this project allows one open pull request at a time
per non-maintainer contributor, and you already have 1 open (#976).

This is not a judgement about the change. Review here is serial — one maintainer,
and every open PR needs a full read, a corpus sweep and a green CI cycle, and has to
be re-checked each time something lands ahead of it. Several at once cost far more
in total than the same work landed one at a time.

Please reopen this once your earlier PR resolves. If you have found other unrelated
bugs meanwhile, open issues for them — the diagnosis is valuable on its own and
costs far less to carry than an open PR.

See One open PR at a time.

@ultrasaurus

Copy link
Copy Markdown
Contributor Author

this does reference an issue "Closes #1107" @yfedoseev

My other PR #976 has been waiting for a few weeks. I'd prefer that being merged first.

@ultrasaurus ultrasaurus changed the title fix(1107): failing test: apply_redactions_destructive indirect reference to an indirect reference to an array fix(1107): apply_redactions_destructive indirect reference to an indirect reference to an array Aug 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Closing automatically: this PR does not reference an issue.

Work here starts from an issue that a maintainer has approved and assigned —
opening a pull request is not how work is claimed. This is not about the quality
of the change: it costs a maintainer one line to say "go ahead" or "we would fix
that differently", and it saves you writing a change that was never going to land.

What to do:

  1. Open an issue describing the problem. A reproducer and one line is enough.
  2. Wait for a maintainer to comment /approve and assign it to you.
  3. Reopen this PR (or open a new one) with Closes #NNN in the description.

Typo, documentation, CI and revert PRs are exempt and can be opened directly.

See Before you open a pull request.

@github-actions

Copy link
Copy Markdown
Contributor

Closing automatically: this project allows one open pull request at a time
per non-maintainer contributor, and you already have 1 open (#976).

This is not a judgement about the change. Review here is serial — one maintainer,
and every open PR needs a full read, a corpus sweep and a green CI cycle, and has to
be re-checked each time something lands ahead of it. Several at once cost far more
in total than the same work landed one at a time.

Please reopen this once your earlier PR resolves. If you have found other unrelated
bugs meanwhile, open issues for them — the diagnosis is valuable on its own and
costs far less to carry than an open PR.

See One open PR at a time.

@yfedoseev

Copy link
Copy Markdown
Owner

This is unblocked — reopen it whenever suits you.

Both closures were bugs, and both are fixed on main as of 188322f:

  • The issue link. The check only matched Closes #NNN. You wrote Closes https://github.com/yfedoseev/pdf_oxide/issues/1107, which is GitHub's own closing syntax and would have closed [Bug]: apply_redactions_destructive fails on pages whose /Contents is an indirect reference to an array #1107 on merge — the check simply could not see it. It now accepts the full URL form, GH-, and the past-tense variants GitHub allows. Verified against eight cases, including that a bare Closes # placeholder still correctly fails.
  • It repeated itself. After you pointed out the first closure was wrong, the same bot posted the same message again, because it re-evaluates on reopen with no memory of having already been answered. Both rejection comments now post at most once per PR.
  • It applied retroactively. The gates had no notion of when the policy started, so every PR opened before it was one push away from being auto-closed for failing a rule that did not exist when it was written. That was every open PR in the repo at the time. Both gates now exempt anything created before the policy commit.

That last one matters for your question about ordering. This PR was opened on 21 August, before the policy, so it is exempt from the one-open-PR limit — you do not have to wait for #976 to resolve before reopening. I said the opposite earlier and it is no longer true.

You asked for #976 first and I am not going to reorder that for you; it is still next on my side either way. But the constraint you were working around is gone, so the choice is yours rather than the automation's.

On the substance: the bug is real and worth landing. While working on something unrelated I found the same root cause at a second call site — replace_text_run records only the array object's id in redacted_orphan_ids on a ref-to-array /Contents, never the element stream ids, so the original content survives the GC there too. Your report found more than the one site. Worth deciding whether this PR covers both or whether the second is a follow-up.

@ultrasaurus

Copy link
Copy Markdown
Contributor Author

@yfedoseev I don't see any way for me to re-open this. I only have the option to "Delete branch"

image

@yfedoseev yfedoseev reopened this Aug 23, 2026
@yfedoseev

Copy link
Copy Markdown
Owner

@ultrasaurus — reopened and synced onto current main, so this is ready for you to carry on with. Our automation closed it, so undoing that was ours to do rather than something to ask of you.

I waited for both intake gates to re-run on the reopen before saying it is safe, rather than just reasoning about it — this PR has been closed twice by two different gates, so a third assurance seemed worth actually testing:

Both green, and it stayed open. #1114 fixed both.

How the one-open-PR rule works now, since it is what caught you:

  • Counted: your open PRs that are not drafts, excluding titles beginning docs:, ci:, chore: or revert:.
  • Not counted: drafts, those trivial classes, and anything created before 2026-08-22 08:45 UTC.

That last exemption keys on the PR creation time, which never changes — so no sync, push or reopen can pull an older PR back into scope. It is what just let this one through, and the sync I pushed did not disturb it.

Where that leaves you: this and #976 are both open non-drafts, but both predate the epoch, so neither is at risk. #798 and #967 are drafts and are not counted. The limit only applies to something new — a fresh PR opened from now on is judged, would see two open non-drafts, and would close itself. Worth landing what is open before starting anything new.

On the sync itself: I merged main locally and signed the merge commit rather than using GitHubs "Update branch" button, because that button produces an unsigned merge commit which then fails the DCO check. Worth knowing if you ever use it here.

One thing I want to be straight about: #976 is waiting on our corpus verification, not on you. You answered the review on 21 August and nothing is outstanding at your end. It should not be consuming your quota while it is parked on us — clearing that is my problem, not something for you to wait out.

…rence

apply_redactions_destructive works on pages whose /Contents is an
indirect reference to an array

refactor: created resolve_page_content_elements so that spec-compliant
logic can be shared from
- get_page_content_bytes
- get_page_images, and
- apply_redactions_destructive

test failed before fix:
destructive_redaction_indirect_contents_array_removes_secret_text_and_bytes

add probe for corpus testing

Signed-off-by: Sarah Allen <sarah@ultrasaurus.com>
@ultrasaurus
ultrasaurus force-pushed the get_page_content_bytes branch from 7bc2b82 to 64eacfb Compare August 24, 2026 15:38
@ultrasaurus

ultrasaurus commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

rebased on latest main, tests green locally

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.

[Bug]: apply_redactions_destructive fails on pages whose /Contents is an indirect reference to an array

2 participants