Skip to content

fix(media): forward scanned PDF page images in chat turns#96568

Closed
harjothkhara wants to merge 1 commit into
openclaw:mainfrom
harjothkhara:oss/96541-scanned-pdf-images-chat
Closed

fix(media): forward scanned PDF page images in chat turns#96568
harjothkhara wants to merge 1 commit into
openclaw:mainfrom
harjothkhara:oss/96541-scanned-pdf-images-chat

Conversation

@harjothkhara

@harjothkhara harjothkhara commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Closes #96541

What Problem This Solves

Fixes an issue where users sending scanned or image-only PDFs over chat channels would have extracted PDF page images dropped before the model saw them.

Why This Change Was Made

The chat media-understanding path now carries PDF-rendered page images as transient current-turn inline images while preserving the file context block. The normal agent-runner path consumes those images once to avoid duplicate prepare/run merges, and ACP dispatch forwards the same extracted images into ACP runTurn attachments.

OpenResponses behavior is unchanged.

User Impact

Scanned receipts, contracts, and image-only PDFs sent through chat channels can now reach vision-capable models as image inputs instead of only a placeholder.

Evidence

AI-assisted: yes. I used Codex to implement and review this change and understand the resulting diff.

Behavior addressed: scanned / image-only PDF page images extracted on chat channel path are forwarded to the model instead of dropped.

Real environment tested: local source checkout at commit 2cd82bd on branch oss/96541-scanned-pdf-images-chat, using the real media-understanding, current-turn image hydration, ACP dispatch, agent-runner, and OpenResponses test paths with mocked document extraction where the external PDF-render boundary is intentionally isolated.

Exact steps or command run after this patch:

  • node scripts/run-vitest.mjs src/media-understanding/apply.test.ts
  • node scripts/run-vitest.mjs src/auto-reply/reply/current-turn-images.test.ts
  • node scripts/run-vitest.mjs src/auto-reply/reply/dispatch-acp.test.ts
  • node scripts/run-vitest.mjs src/auto-reply/reply/agent-runner-execution.test.ts
  • node scripts/run-vitest.mjs src/gateway/openresponses-http.server.file-only.test.ts
  • git diff --check
  • /Users/harjothk/.cache/codex-runtimes/codex-primary-runtime/dependencies/python/bin/python3 .agents/skills/autoreview/scripts/autoreview --mode local --prompt "Review fix for #96541 after addressing duplicate image merge and ACP dispatch gaps: scanned/image-only PDF extraction on chat path carries extracted PDF page images into inline image payloads for normal agent runner and ACP dispatch, consumes transient extracted images, and preserves existing OpenResponses behavior."

Evidence after fix:

  • apply.test.ts now verifies an image-only PDF extraction result populates CurrentTurnImages and no longer emits the old "images not forwarded" placeholder.
  • current-turn-images.test.ts verifies extracted PDF page images merge into inline image payloads and are not merged twice across prepare/run passes.
  • dispatch-acp.test.ts verifies ACP dispatch passes extracted PDF page images as runTurn attachments.

Observed result after fix: all commands above passed; autoreview clean: no accepted/actionable findings reported.

What was not tested: I did not run a live Telegram, Slack, Discord, or WhatsApp send with a real scanned PDF; the local proof exercises the shared chat media-understanding, normal agent-runner, ACP dispatch, and sibling OpenResponses source paths.

@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 24, 2026, 6:36 PM ET / 22:36 UTC.

Summary
The PR carries extracted scanned-PDF page images from media understanding into current-turn inline image payloads for normal agent-runner and ACP turns.

PR surface: Source +85, Tests +131. Total +216 across 7 files.

Reproducibility: yes. source-reproducible. Current main's chat file-extraction path can receive PDF-rendered images from extractFileContentFromSource but emits only a placeholder text block instead of forwarding image payloads.

Review metrics: 1 noteworthy metric.

  • Stale documented behavior: 1 user-facing docs statement not updated. The PR changes chat-path PDF image forwarding while the media-understanding docs still document the old not-forwarded placeholder behavior.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #96541
Summary: This PR is the open candidate fix for the canonical scanned/image-only PDF chat-channel forwarding bug; the nearby clawpdf pixel-budget issue is related but distinct.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted real behavior proof showing a scanned or image-only PDF attachment reaches a vision-capable model after this patch.
  • Update the media-understanding docs so they no longer state that rendered PDF page images are not forwarded.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists focused tests with mocked document extraction and explicitly says no live chat send with a real scanned PDF was run, so redacted real behavior proof is still needed before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A native Telegram proof would materially show the user-visible scanned-PDF chat behavior that unit tests cannot prove. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis telegram desktop proof: send a scanned PDF to a Telegram agent and verify the reply reflects the rendered page image, not the placeholder.

Risk before merge

  • [P1] No redacted real chat/runtime proof shows a scanned or image-only PDF reaching a vision-capable model after this patch; the PR body only lists tests with mocked document extraction.
  • [P1] The media-understanding docs still tell users that PDF-rendered page images are not forwarded, so the user-facing documentation would be stale after merge.

Maintainer options:

  1. Decide the mitigation before merge
    Land the narrow image-handoff approach after adding redacted real chat or high-fidelity runtime proof and updating the media-understanding docs to describe the new forwarded-image behavior.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The PR needs contributor-supplied real behavior proof and a small docs correction; automation cannot supply proof from the contributor's setup.

Security
Cleared: The diff moves existing extracted image data through in-memory prompt and ACP attachment paths and adds tests; no dependency, workflow, secret, install, or publishing surface changed.

Review findings

  • [P3] Update the media-understanding docs for forwarded PDF images — src/media-understanding/apply.ts:522
Review details

Best possible solution:

Land the narrow image-handoff approach after adding redacted real chat or high-fidelity runtime proof and updating the media-understanding docs to describe the new forwarded-image behavior.

Do we have a high-confidence way to reproduce the issue?

Yes, source-reproducible. Current main's chat file-extraction path can receive PDF-rendered images from extractFileContentFromSource but emits only a placeholder text block instead of forwarding image payloads.

Is this the best way to solve the issue?

Yes, with proof and docs still needed before merge. The implementation reuses existing current-turn inline-image and ACP attachment plumbing rather than adding channel-specific PDF handling.

Full review comments:

  • [P3] Update the media-understanding docs for forwarded PDF images — src/media-understanding/apply.ts:522
    This change now forwards rendered PDF page images, but docs/nodes/media-understanding.md still says this path keeps the images not forwarded to model placeholder. That user-facing docs page will be false after merge, so update it to describe the new chat-path behavior.
    Confidence: 0.91

Overall correctness: patch is correct
Overall confidence: 0.82

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against f2af052ceed9.

Label changes

Label justifications:

  • P2: This PR fixes a bounded media-understanding bug affecting scanned PDF attachments across chat channels without a core outage or security boundary change.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists focused tests with mocked document extraction and explicitly says no live chat send with a real scanned PDF was run, so redacted real behavior proof is still needed before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The shared chat media path affects Telegram-visible scanned PDF replies and can be demonstrated in a short Telegram Desktop proof.
Evidence reviewed

PR surface:

Source +85, Tests +131. Total +216 across 7 files.

View PR surface stats
Area Files Added Removed Net
Source 4 124 39 +85
Tests 3 131 0 +131
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 7 255 39 +216

What I checked:

  • Repository policy: Read the full root AGENTS.md and applied its high-confidence PR review, source-history, sibling-surface, and real behavior proof requirements. (AGENTS.md:1)
  • Current main behavior: Current main still converts extracted PDF page images into the placeholder text '[PDF content rendered to images; images not forwarded to model]' and returns only text blocks from the chat media-understanding path. (src/media-understanding/apply.ts:496, f2af052ceed9)
  • Extractor contract: The shared file extractor returns PDF-rendered image payloads when PDF extraction produces images, so the chat path has model-visible image data available before it drops it. (src/media/input-files.ts:441, f2af052ceed9)
  • Sibling OpenResponses surface: The /v1/responses input_file path already appends file.images into model image payloads, matching the linked issue's parity target. (src/gateway/openresponses-http.ts:629, f2af052ceed9)
  • PR image handoff: On the PR head, extractFileBlocks collects extracted images, removes the old not-forwarded placeholder, and appends the images to CurrentTurnImages for the current turn. (src/media-understanding/apply.ts:514, 2cd82bdbc01e)
  • PR normal runner consumption: On the PR head, resolveCurrentTurnImages consumes transient CurrentTurnImages once and merges them with existing inline images before agent execution. (src/auto-reply/reply/current-turn-images.ts:139, 2cd82bdbc01e)

Likely related people:

  • frankekn: Commit cb18ce7 added the central applyMediaUnderstanding file-attachment extraction path and tests that this PR now changes. (role: introduced chat file-extraction behavior; confidence: high; commits: cb18ce7a8566; files: src/media-understanding/apply.ts, src/media-understanding/apply.test.ts)
  • amknight: Commit e850750 recently adjusted inbound PDF extraction limits on the same applyMediaUnderstanding and extractFileBlocks path. (role: recent media-understanding PDF contributor; confidence: high; commits: e850750754f2; files: src/media-understanding/apply.ts, src/media-understanding/apply.test.ts, src/media-understanding/file-extraction-limits.ts)
  • marchhao: Commit b5ada80 introduced current-turn image hydration, the helper this PR extends to carry extracted PDF page images. (role: current-turn image path contributor; confidence: high; commits: b5ada806dddc; files: src/auto-reply/reply/current-turn-images.ts, src/auto-reply/reply/get-reply-run.media-only.test.ts)
  • NianJiuZst: Commit 55a0c9b restored WebChat image understanding and touched current-turn images, agent runner, ACP dispatch, and media-understanding apply paths. (role: adjacent chat image-understanding contributor; confidence: medium; commits: 55a0c9b1f474; files: src/auto-reply/reply/current-turn-images.ts, src/auto-reply/reply/dispatch-acp.ts, src/auto-reply/reply/get-reply-run.ts)
  • yetval: Commit 02acb0b added the file-only OpenResponses path that already forwards input_file images, and the linked issue uses that sibling surface as the parity target. (role: sibling OpenResponses contributor; confidence: medium; commits: 02acb0bd7455; files: src/gateway/openresponses-http.ts, src/gateway/openresponses-http.server.file-only.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. P2 Normal backlog priority with limited blast radius. labels Jun 24, 2026
@harjothkhara

harjothkhara commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Closing this — the code is ready, but I can't satisfy its required proof gate, so I'm handing it off rather than leaving a stuck PR.

State of the fix (verified independently):

  • Root cause addressed: extractFileBlocks (src/media-understanding/apply.ts) now returns extracted page images and populates ctx.CurrentTurnImages, so rasterized scanned/image-only PDF pages are forwarded instead of replaced with the old "images not forwarded" placeholder.
  • Red-green: the regression (carries rendered PDF page images into current-turn image payloads) fails on main (expected … to contain '[PDF content rendered to images]') and passes on this branch.
  • Wiring traced end-to-end: ctx.CurrentTurnImages → get-reply-run consumes once → followupRun.images → agent-runner-execution → model; ACP path forwards via dispatch-acp; OpenResponses unchanged. No double-merge, no stranding.

Why I'm not pushing it to merge: the mantis: telegram-visible-proof gate is required for this Telegram-visible behavior, and the automated Mantis run recorded that it "could not honestly make the visible Telegram reply depend on whether the scanned PDF page image reached the model" (comparison.pass: false, no artifacts) — i.e., it's "Telegram-visible in principle" but the honest visual proof needs a live vision model + a real scanned PDF whose content drives the reply. I don't have that environment, and I won't manufacture a proof I can't honestly capture.

Handoff: if a maintainer can run the Mantis/live proof (a scanned PDF with a token present only in the rasterized page, confirming the model's reply quotes it), the diff here should clear the gate as-is. Closing to free my open-PR slot; happy for anyone equipped to take this over or for me to reopen if a proof path opens up.

This was referenced Jun 25, 2026
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

mantis: telegram-visible-proof Mantis should capture Telegram visible proof. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: scanned / image-only PDFs are silently dropped on chat channels (rendered page images extracted then discarded)

1 participant

@harjothkhara