Skip to content

fix(cli): escape JSON error output - #387

Open
NgoQuocViet2001 wants to merge 1 commit into
firecrawl:mainfrom
NgoQuocViet2001:fix-pdf2md-error-json
Open

fix(cli): escape JSON error output#387
NgoQuocViet2001 wants to merge 1 commit into
firecrawl:mainfrom
NgoQuocViet2001:fix-pdf2md-error-json

Conversation

@NgoQuocViet2001

@NgoQuocViet2001 NgoQuocViet2001 commented Aug 14, 2026

Copy link
Copy Markdown

What

The --json exit in pdf2md's main conversion path interpolates the error straight into its object:

Err(e) => {
    if json_output {
        println!(r#"{{"error":"{}"}}"#, e);   // no escaping

A PdfError message carrying a quote or a newline produces JSON that no consumer can parse. missing required dictionary key "Pages" is the everyday case:

before  {"error":"PDF parsing error: missing required dictionary key "Pages"
while loading catalog"}          <- unparseable

after   {"error":"PDF parsing error: missing required dictionary key \"Pages\"\nwhile loading catalog"}

How

Both --json error exits now go through a small format_error_json, placed beside and mirroring the existing format_ocr_error_json:

fn format_error_json(error: &str) -> String {
    format!(r#"{{"error":"{}"}}"#, json_escape(error))
}

The items-json exit was already calling json_escape inline, so routing it through the helper is not a behaviour change there — it just means the two paths cannot drift, and the emitted object is testable rather than only the escaper.

I did not give this one a schema_version, unlike the OCR envelope: that would change the shape of an output that already ships. Happy to add it if you'd rather the two converge.

Rebased and reshaped

This PR was opened before #434 and had gone stale — resetting onto current main resolved the conflict, and it turned out the original patch was aimed at a site you have since fixed. The items-json path escapes on main today; the conversion path is the one still open. So this is now a much smaller change against that remaining site rather than a rebase of the original diff.

Test plan

  • Ran: cargo test --bin pdf2md4 passed, 0 failed, including the existing ocr_json_errors_use_the_same_versioned_envelope.
  • New: convert_json_errors_are_escaped asserts the full emitted object for a message containing both a quote and a newline.
  • Ran: cargo clippy --bin pdf2md — no warnings.
  • Ran: rustfmt --check src/bin/pdf2md.rs — clean.

Not run: cargo fmt --all -- --check and the full cargo test. cargo fmt across this workspace overflows its stack on my Windows machine — it does so on unmodified main too, so it is my environment rather than this change — which is why I checked the one touched file with rustfmt directly. Flagging it rather than letting CI be the first to find out. An earlier revision of this description claimed both had been run; that was carried over from the original August 14 version of this PR and is not accurate for this commit.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No issues found across 1 file

Shadow auto-approve: would auto-approve. Fixes invalid JSON output by escaping error messages, with a shared formatter and a regression test covering quotes/control chars. No new behavior, exposure, or operational tradeoff; human sign-off is unnecessary.

Re-trigger cubic

The --json exit in the main conversion path interpolated the error
straight into its object:

    println!(r#"{{"error":"{}"}}"#, e);

A PdfError message carrying a quote or a newline -- "missing required
dictionary key \"Pages\"" is the common one -- produced JSON that no
consumer could parse. The items-json path next to it already escapes.

Route both --json error exits through a format_error_json helper that
mirrors the existing format_ocr_error_json, so the two paths escape
identically and a test can pin the emitted object rather than the
escaper alone.
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.

1 participant