Skip to content

M11: PDF Optimizer can discard an embedded search index (optimize.discard-search-index) - #4440

Open
dotCooCoo wants to merge 8 commits into
storytold:mainfrom
dotCooCoo:feat/discard-search-index
Open

dotCooCoo wants to merge 8 commits into
storytold:mainfrom
dotCooCoo:feat/discard-search-index

Conversation

@dotCooCoo

@dotCooCoo dotCooCoo commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

What this changes

PDF Optimizer ▸ Discard Objects had no option for an embedded search index (optimize.discard-search-index was planned). The reference app's help page lists it in the Discard Objects panel: "Discard Embedded Search Index: Removes embedded search indexes, which reduces the file size." (https://web.archive.org/web/20221217175446/https://helpx.adobe.com/acrobat/using/optimizing-pdfs-acrobat-pro.html).

  • Discard Objects has a new checkbox, "Discard embedded search index", off by default (Settings::discard_search_index).
  • It removes the entries of the catalog's /PieceInfo whose application name contains "index" in any letter case, such as IBM-ODIndexes, and removes /PieceInfo when nothing else is left in it. Other application data and the pages' own /PieceInfo stay.
  • An indirect /PieceInfo is not edited. The catalog gets a direct copy without those entries, and other objects that refer to the original keep it.
  • The full save leaves out the objects that only the removed entries refer to.
  • doc_optimize takes discard_search_index and returns search_indexes.

How you verified it

  • cargo fmt --check
  • clippy with -D warnings on the crates I changed
  • tests pass on the crates I changed
  • cargo xtask layers and cargo xtask assets (if you touched crate boundaries or any asset)

New tests, each seen failing with its part of the change reverted (on main they do not compile):

  • optimize.rs::search_indexes_in_the_catalog_go_and_other_application_data_stays: two index entries go and another application's entry stays; after saving, the two objects of the index that refer to each other are gone, and an object the page also uses stays; with the option off nothing changes.
  • optimize.rs::an_indirect_piece_info_is_left_for_its_other_users: a /PieceInfo shared with a page leaves the catalog and keeps its entry for the page, and a /PieceInfo that is not a dictionary is left alone.
  • automation.rs::optimizing_discards_an_embedded_search_index_through_tools: doc_optimize keeps the index by default and removes it with discard_search_index.

Commands: cargo clippy -p pdfcraft-optimize -p pdfcraft-automation -p pdfcraft-ui-egui --all-targets -- -D warnings; cargo test -p pdfcraft-optimize (34 passed); cargo test -p pdfcraft-automation --test it -- optimiz (7 passed); cargo test -p pdfcraft-ui-egui --lib i18n (63 passed); cargo xtask parity, layers, assets.

On Windows 11 (x86-64) with rustc 1.99.1, on 1ca8455 (this branch with main at 20eebc1c merged in):

  • cargo fmt --all -- --check; cargo clippy -p pdfcraft-optimize -p pdfcraft-automation -p pdfcraft-ui-egui --all-targets -- -D warnings.
  • cargo test -p pdfcraft-optimize -p pdfcraft-engine -p pdfcraft-automation -p pdfcraft-ui-egui -p pdfcraft-cli -p pdfcraft -p pdfcraft-web: 20 test binaries, 2,666 passed, 5 failed, 15 ignored. The 5 failures are pdfcraft-ui-egui print:: tests that save through "Save as PDF"; they fail the same way on main at 9e90b2a5 without this branch.
  • cargo xtask layers, assets and parity.

git merge-tree reports no conflicts with main at 20eebc1c.

On 79d713a (main at 24bb8530 merged in; the one conflict was the doc_optimize description in crates/automation/src/tools.rs, which now carries both main's mono clause and this branch's search_index clause), on Windows 11 (x86-64) with rustc 1.99.1:

  • cargo fmt --all -- --check; cargo check --workspace --tests; cargo clippy -p pdfcraft-optimize -p pdfcraft-automation -p pdfcraft-ui-egui --all-targets -- -D warnings.
  • cargo test -p pdfcraft-optimize (81 passed); cargo test -p pdfcraft-automation --test it -- optimiz (10 passed); cargo test -p pdfcraft-ui-egui --lib i18n (65 passed).
  • cargo xtask layers, assets and parity.

Never-crash

Every PDF, script, MCP call, settings file and keystroke is untrusted input (AGENTS.md §4).

  • No new unwrap/expect/panic!/unreachable!/todo! outside tests, unless it is
    provably infallible with a comment saying why
  • No indexing or slicing with input-derived positions (get, and strings only at char
    boundaries); checked or saturating arithmetic; allocations sized by input are capped;
    recursion is bounded
  • No unsafe
  • Any crash this fixes has a small synthetic regression test

This fixes no crash. The change edits one dictionary and does not walk the object graph; the full save's own reachability walk drops the objects.

Provenance

This is the part that cannot be undone after a merge, and the part a new contributor is
least likely to know about. Please read AGENTS.md §1 before adding any asset.

  • No asset from an Adobe product, and no Adobe icon recreated, traced or imitated
  • No font files committed here (they live in
    storytold/craft-fonts)
  • No GPL/AGPL code copied in — MuPDF, Ghostscript, Poppler, veraPDF and DSS may only be
    run as external oracle processes
  • Behaviour comes from the specification or black-box observation, not from reading
    another implementation's source
  • Every new asset has an ATTRIBUTION.toml entry and cargo xtask assets passes

Security

  • --root confinement still holds on every path, including ones reached by nested
    tool dispatch
  • MCP and the UI control channel stay opt-in, loopback-only and token-authenticated
  • No new outbound network access (that decision is the owner's)

Parity and reach

  • parity/acrobat-features.toml updated in this PR, with notes saying what is still
    missing — shipped means "exists and tested", not "as good as Acrobat"
  • Reachable headlessly through an automation tool where that applies (AGENTS.md §3),
    so agents can drive it and not just the UI
  • Docs updated if this changes a documented behaviour (docs/, crate README)

optimize.discard-search-index is now shipped; crates/optimize/README.md describes it. The new label has no catalog rows yet.

Anything a reviewer should look at first

Summary by CodeRabbit

  • New Features
    • Added an opt-in PDF optimization setting to discard catalog entries associated with embedded search indexes. Other application entries and page-level data are preserved.
    • Optimization results now report how many search-index entries were discarded.
    • Added a checkbox for this setting in the optimization interface, and made the option available through document optimization automation.

PDF Optimizer > Discard Objects gets "Discard embedded search index", off
by default. It removes the entries of the catalog's /PieceInfo whose
application name contains "index" in any letter case, such as
IBM-ODIndexes, and removes /PieceInfo when nothing else is left in it.
Other application data and the pages' own /PieceInfo stay. An indirect
/PieceInfo is not edited: the catalog gets a direct copy without those
entries, and other objects that refer to the original keep it. The full
save leaves out the objects that only the removed entries refer to.

doc_optimize takes discard_search_index and reports search_indexes.
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • Resume reviews
  • Trigger review

No actionable comments were generated in the recent review.

Walkthrough

The optimizer adds a default-off option to remove catalog /PieceInfo entries whose application names contain “index,” regardless of case. It reports the number removed. The UI and automation tool expose the option.

Changes: Search-index removal

Layer / File(s) Summary
Optimizer filtering and validation
crates/optimize/src/lib.rs, crates/optimize/tests/optimize.rs, crates/optimize/README.md
When enabled, the optimizer removes matching catalog /PieceInfo entries and removes /PieceInfo if it is empty. It copies indirect dictionaries before editing. Tests cover default and enabled behavior, retained data, shared objects, cycles, and non-dictionary values.
UI and automation entry points
crates/ui-egui/src/workflows/optimize_ui.rs, crates/automation/src/documents.rs, crates/automation/src/tools.rs, crates/automation/tests/it/automation.rs, parity/acrobat-features.toml
The UI and automation tool accept the option. The automation response includes the removal count. Integration tests check default and enabled behavior, and the parity record marks the feature as shipped.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant doc_optimize
  participant Optimizer
  Caller->>doc_optimize: Set discard_search_index
  doc_optimize->>Optimizer: Pass optimization settings
  Optimizer->>doc_optimize: Return report with search_indexes count
  doc_optimize->>Caller: Return optimization result
Loading

Suggested reviewers: piohrun

Merge Risk: Minimal · previous review f13d6

The option is off by default and removes matching catalog entries when enabled. No merge-blocking risk was identified.

Security architecture risk: Minimal · up to 79d71

The new operation is opt-in and limited to an optimized document copy. Existing editing restrictions and output-path controls remain in place. Shared page data is preserved, and the reviewed transition does not introduce a material security risk.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — Attacker-controlled PDF application names can influence which catalog entries are removed when the caller enables the option. The new effect is bounded to the selected document’s optimized output; those names do not select a filesystem destination or grant additional caller authority.

Trust Boundaries and Controls

  • observed — The existing engine path requires an editable document and refuses signed documents before cloning. Automation resolves the caller-supplied destination with write intent through its existing configured-root checks before optimization; the new boolean does not participate in document lookup or path resolution.

Resilience and Maintainability Implications

  • observed — Automation obtains complete serialized bytes before invoking its unchanged staged output writer. That writer documents sync-before-rename behavior and explicitly does not promise parent-directory durability after power loss. The new option does not introduce a separate publication or recovery mechanism.
Pre-merge checks: Passed 4 · Inconclusive 1

Failed Failed checks: 1 inconclusive

Check name Status Explanation Resolution
Docstring Coverage Inconclusive Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely identifies the main change: the PDF Optimizer can discard an embedded search index through optimize.discard-search-index.
Description check Passed The description follows the required template and documents the behavior, verification commands, tests, crash safety, provenance, security, parity, automation reach, documentation, and reviewer consid…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description follows the required template and documents the behavior, verification commands, tests, crash safety, provenance, security, parity, automation reach, documentation, and reviewer considerations. It also explains why the unchecked crash-regression item does not apply.

Full details: Docstring Coverage

Explanation

Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 5 files. (3 skipped: 2 unsupported, 1 too large.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 79d713a and f13d6db.

Files selected for processing: 6
  • crates/automation/src/tools.rs
  • crates/automation/tests/it/automation.rs
  • crates/optimize/README.md
  • crates/optimize/src/lib.rs
  • crates/optimize/tests/optimize.rs
  • parity/acrobat-features.toml

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

@dotCooCoo

Copy link
Copy Markdown
Contributor Author

Merged main at fc1372a (0f15f97).

  • Conflicts: the doc_optimize description in crates/automation/src/tools.rs, crates/optimize/README.md, the module doc and the end of crates/optimize/src/lib.rs, and the end of crates/optimize/tests/optimize.rs. Each keeps main's new compatibility, object stream and LZW text, functions and tests, and this branch's search index text, functions and tests.
  • cargo xtask ci on 0f15f97 (Windows 11, x86-64): fmt, clippy and the pdfcraft-cli check passed; the test step ran 4,869 passed, 3 failed, 17 ignored. The 3 failures were render-thread tests in combine_grid.rs and an XFA time-budget test, run while other builds held the CPU at 100%; the combine_grid, xfa and frame-budget tests pass when run alone. i18n-report, layers, wasm, assets, deny and parity passed when run one by one.
  • git merge-tree reports no conflicts with main at a7801dc.

@dotCooCoo

Copy link
Copy Markdown
Contributor Author

Merged main (20eebc1c) into the branch as 1ca8455c. main split crates/automation/src/lib.rs into modules, so the discard_search_index option and the search_indexes count now live in crates/automation/src/documents.rs, where doc_optimize moved. The branch merges cleanly with main at 20eebc1c.

Checks on 1ca8455c, on Windows 11 (x86-64) with rustc 1.99:

  • cargo fmt --all -- --check
  • cargo clippy -p pdfcraft-optimize -p pdfcraft-automation -p pdfcraft-ui-egui --all-targets -- -D warnings
  • cargo test -p pdfcraft-optimize -p pdfcraft-engine -p pdfcraft-automation -p pdfcraft-ui-egui -p pdfcraft-cli -p pdfcraft -p pdfcraft-web: 20 test binaries, 2,666 passed, 5 failed, 15 ignored. The 5 failures are the pdfcraft-ui-egui print:: tests that save through "Save as PDF" (current_print_page_saves_the_viewed_page_when_labels_restart, print_as_image_is_an_advanced_choice_that_saves_pictures, print_dialog_lays_out_sheets_and_saves_a_pdf, print_dialog_prints_the_selected_pages, restored_paper_keeps_preview_and_saved_sheet_dimensions). They fail the same way on main at 9e90b2a5 without this branch, and this branch does not touch the print code.
  • cargo xtask layers, assets and parity

# Conflicts:
#	crates/automation/src/tools.rs

@FredApps FredApps 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.

ReviewerA — source-read COMMENT (not an approval)

I read the diff: the PDF Optimizer can discard an embedded search index (discard_search_index, the command optimize.discard-search-index): the automation layer's settings table gains the flag, the tool schema lists it, the result reports search_indexes (the number discarded), and the test optimizing_discards_an_embedded_search_index_through_tools builds a catalogue /PieceInfo with an IBM-ODIndexes and an Illustrator entry, optimises without the option (the count is 0 and both entries stay) and with it (the count is 1 and only Illustrator remains). Acrobat's Optimizer has the same choice ("Discard embedded search indexes"). Testing that the other private data is untouched is the right negative case. The key point is which entries count as search indexes: here the one name IBM-ODIndexes (Acrobat's Catalog index), so state the list and where it comes from, since a wrong match would delete a private application's data. I read the automation side only; the optimiser's implementation is in a part I did not see, so I could not check it removes only that entry and keeps an empty /PieceInfo valid (it should be removed when empty). The PR bundles the CLI, the tool and the engine in a repository where the engine side normally goes first. I have not run it, and no exact-head CI result exists for this head. Nothing was run.

I ran nothing and have no exact-head CI result for this PR, so this is not an approval. Where a diff was long I read its code hunks rather than every generated or documentation line.

Status: waiting_author.

@dotCooCoo

Copy link
Copy Markdown
Contributor Author

Merged main at 0cba87a; conflict in crates/automation/src/tools.rs (the doc_optimize description) resolved. cargo check --workspace --tests, cargo fmt --check and clippy on pdfcraft-optimize and pdfcraft-automation pass; 450 tests in those two crates pass.

@dotCooCoo

Copy link
Copy Markdown
Contributor Author

The branch includes main at 661ded72bb. The merge preserves the search-index option and main's image decode-budget description. The branch merges cleanly with main at 69ddcd89bf.

Filtering an indirect /PieceInfo now writes an indirect replacement and leaves the original dictionary for its other users. A save/reopen regression reproduced an unreadable document catalog when retained application data was deeply nested; that document now reads back with the retained data intact.

Windows checks: workspace test compilation, formatting, optimizer and automation tests (461 passed), and clippy for the optimizer, automation and UI crates.

@dotCooCoo

Copy link
Copy Markdown
Contributor Author

Merged main at ef8c71a; the doc_optimize tool description in crates/automation/src/tools.rs conflicted, resolved by keeping main's JBIG2Globals sentence and this branch's search_index sentence. Checked with cargo check, clippy and the branch's tests.

This branch has not been deployed

No deployments
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.

2 participants