Skip to content

fix(file_tools): refuse plain-text writes that corrupt binary documents (port of ironclaw#7109) - #82818

Closed
teknium1 wants to merge 1 commit into
mainfrom
ironclaw-port/binary-doc-write-guard
Closed

teknium1 wants to merge 1 commit into
mainfrom
ironclaw-port/binary-doc-write-guard

Conversation

@teknium1

Copy link
Copy Markdown
Collaborator

Summary

write_file / patch now refuse plain-text writes that would corrupt binary documents — always for opaque container formats (.doc/.docx/.xls/.xlsx/.ppt/.pptx/.odt/.ods/.odp), and for .pdf only when overwriting an existing file (new-PDF creation stays allowed).

Port of nearai/ironclaw#7109.

Root cause: read_file auto-extracts these formats to readable text, so a model plausibly believes it holds the file's contents and writes the edited text back — silently destroying the document container. Proven live on current main before fixing: write_file_tool over a valid .docx left a non-zip corpse (zipfile.is_zipfile → False), and a text write over an existing .pdf clobbered the %PDF header.

Changes

  • tools/binary_extensions.py: OPAQUE_DOCUMENT_EXTENSIONS + has_opaque_document_extension() + is_pdf_path() (pure string checks, no I/O)
  • tools/file_tools.py: _check_binary_document_write() gate, wired into write_file_tool and patch_tool (replace mode + V4A Update/Add headers; Delete/Move skip the guard since they write no text). Error messages point the model to the docx/xlsx/powerpoint/pdf skills and python-docx/openpyxl/python-pptx via terminal — the correct editing path stays fully open.
  • tests/tools/test_binary_document_write_guard.py: guard unit tests + end-to-end write_file/patch coverage with bytes-untouched assertions

Adaptation notes

IronClaw's fix lives in their Rust coding extension with a read-representation fingerprint (ReadRepresentation::ExtractedText). Hermes doesn't track read representation per scope, so the port uses the simpler always-reject rule for opaque containers (matching their is_opaque_binary_document_path half exactly) and the existing-file check for PDF (matching their split-guard follow-up commit that unblocked new-PDF authoring). Terminal-based document editing (python-docx etc.) is unaffected — this guards only the text file tools, which are the exact surface that shows extracted text.

Validation

Scenario Before After
write_file over existing valid .docx silent corruption (zip destroyed) refused, bytes untouched
write_file to new report.docx writes invalid "docx" refused with skill guidance
write_file over existing .pdf %PDF header clobbered refused, bytes untouched
write_file to NEW .pdf allowed still allowed
patch (replace + V4A Update) on .docx corrupts refused
V4A Delete of .docx works still works (guard skipped)

Targeted tests: tests/tools/test_binary_document_write_guard.py + test_file_write_safety.py + test_file_tools.py — 114 passed. E2E probe with real imports + isolated HERMES_HOME confirmed all four scenarios above against the worktree.

Infographic

binary-document-write-guard

Port from nearai/ironclaw#7109: read_file auto-extracts .docx/.xlsx/.pptx
(and PDF via anydoc) to readable text, so a model plausibly believes it
holds the file's contents and writes the edited text back with
write_file/patch — silently destroying the document container. Proven
live on main: write_file over a valid .docx left a non-zip corpse, and a
text write over an existing .pdf clobbered the %PDF header.

- tools/binary_extensions.py: OPAQUE_DOCUMENT_EXTENSIONS +
  has_opaque_document_extension() + is_pdf_path() (pure string checks)
- tools/file_tools.py: _check_binary_document_write() — opaque container
  formats (doc/docx/xls/xlsx/ppt/pptx/odt/ods/odp) always rejected; .pdf
  rejected only when overwriting an existing regular file (new-PDF
  creation stays allowed, matching the upstream split guard). Wired into
  write_file_tool and patch_tool (replace + V4A Update/Add headers;
  Delete/Move skip the guard since they write no text).
- tests/tools/test_binary_document_write_guard.py: guard unit tests +
  end-to-end write_file/patch coverage incl. bytes-untouched assertions.
@github-actions

Copy link
Copy Markdown

૮ >ﻌ< ა ci review

ran on 9b8e631 — fix(file_tools): refuse plain-text writes that corrupt binar

all good!

@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets tool/file File tools (read, write, patch, search) P1 High — major feature broken, no workaround labels Aug 10, 2026
@egilewski

Copy link
Copy Markdown

suggesting changes

tools/read_extract.py:40-46 makes .docm an extractable document, but the new OPAQUE_DOCUMENT_EXTENSIONS in tools/binary_extensions.py:52-55 omits it. That leaves the exact extraction-to-text-write corruption path open: on the current-main replay, write_file accepted a plain-text overwrite of an OOXML-style .docm container, changed its bytes, and left it no longer a ZIP, while the .docx and existing-PDF controls were correctly refused. Because this port intentionally does not track whether read_file returned extracted text, please guard .docm (and assess the other locally extracted opaque container variants) and add a bytes-untouched regression for it.

Security evidence:

  • trust boundary: Model-generated plain text crosses from extracted document content into write_file / patch and an existing binary document.
  • source/sink/invariant: read_file can extract .docm; every text-write sink must refuse overwriting that opaque container before mutation.
  • current-main reproduction: Existing .docx, .docm, and .pdf inputs were overwritten and corrupted; new-PDF creation remained a working positive control.
  • PR-head or patch-replay validation: The integrated replay refused .docx and existing .pdf with bytes unchanged, but accepted .docm, changed its bytes, and destroyed its ZIP container; new-PDF creation still worked.
  • positive/negative cases: Existing .docx and .pdf were preserved, new .pdf was allowed, and existing .docm was destructively accepted.
  • residual bypass search: .docm is explicitly extractable but absent from the guard; other extracted container variants should be checked against the same invariant.
  • reviewer validation: Source inspection and an invocation-level probe confirmed the residual; the focused suite passed 112 tests with 2 skips but has no .docm case.

Not checked:

  • Full test suite
  • CodeRabbit review

Signed: GPT-5.6-sol-xhigh in Codex

@kshitijk4poor

Copy link
Copy Markdown

Merged via #86362 (rebase-merge) — your commits cherry-picked with authorship preserved.

Follow-up commit added 10 missing extensions (.docm, .xlsm, .xlsb, .pptm, .ppsx, .ppsm, .pps, .pot, .rtf, .epub) that read_file auto-extracts via anydoc but were absent from the original OPAQUE_DOCUMENT_EXTENSIONS set — flagged by @egilewski, proven live for .docm. Added bytes-untouched regression test for .docm.

Thanks for the clean port from ironclaw!

@kshitijk4poor

Copy link
Copy Markdown

Addressed in #86362 — added .docm and 9 other missing anydoc-extracted container formats (.xlsm, .xlsb, .pptm, .ppsx, .ppsm, .pps, .pot, .rtf, .epub) to OPAQUE_DOCUMENT_EXTENSIONS. Added a bytes-untouched regression test for .docm specifically. Good catch — that was a real gap in the original set.

kshitijk4poor added a commit that referenced this pull request Aug 14, 2026
OPAQUE_DOCUMENT_EXTENSIONS was missing 10 extensions that read_file
auto-extracts via anydoc: .docm, .xlsm, .xlsb, .pptm, .ppsx, .ppsm,
.pps, .pot, .rtf, .epub. Each has the same corruption path: read_file
shows extracted text, model writes it back, container is destroyed.

Flagged by @egilewski on PR #82818 — proven live for .docm (text write
left a non-zip corpse). Added bytes-untouched regression test for .docm.
skappafrost pushed a commit to skappafrost/hermes-agent that referenced this pull request Aug 15, 2026
OPAQUE_DOCUMENT_EXTENSIONS was missing 10 extensions that read_file
auto-extracts via anydoc: .docm, .xlsm, .xlsb, .pptm, .ppsx, .ppsm,
.pps, .pot, .rtf, .epub. Each has the same corruption path: read_file
shows extracted text, model writes it back, container is destroyed.

Flagged by @egilewski on PR NousResearch#82818 — proven live for .docm (text write
left a non-zip corpse). Added bytes-untouched regression test for .docm.
bobaba76 pushed a commit to bobaba76/hermes-agent that referenced this pull request Aug 27, 2026
OPAQUE_DOCUMENT_EXTENSIONS was missing 10 extensions that read_file
auto-extracts via anydoc: .docm, .xlsm, .xlsb, .pptm, .ppsx, .ppsm,
.pps, .pot, .rtf, .epub. Each has the same corruption path: read_file
shows extracted text, model writes it back, container is destroyed.

Flagged by @egilewski on PR NousResearch#82818 — proven live for .docm (text write
left a non-zip corpse). Added bytes-untouched regression test for .docm.
melon-xf added a commit to melon-xf/hermes-agent that referenced this pull request Sep 3, 2026
OPAQUE_DOCUMENT_EXTENSIONS was missing 10 extensions that read_file
auto-extracts via anydoc: .docm, .xlsm, .xlsb, .pptm, .ppsx, .ppsm,
.pps, .pot, .rtf, .epub. Each has the same corruption path: read_file
shows extracted text, model writes it back, container is destroyed.

Flagged by @egilewski on PR NousResearch#82818 — proven live for .docm (text write
left a non-zip corpse). Added bytes-untouched regression test for .docm.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P1 High — major feature broken, no workaround tool/file File tools (read, write, patch, search) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants