Repository navigation
fix(coding): reject text writes to binary documents - #7109
Merged
Merged
Conversation
|
🚅 Deployed to the ironclaw-pr-7109 environment in ironclaw-ci-preview
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7109
August 4, 2026 05:26
Destroyed
github-actions
Bot
added
size: M
Contributor
Author
🧩 Resolve · PR #7109
Commit 93b3b7a →Committed 93b3b7a to fix the rustfmt failure in the binary-document path check. The original PR commit remains intact, and the worktree is clean. Full Rust tests/clippy were unavailable because cargo and rustc are not installed. Automatic · PR opened + CI failed · attempt 1 of 3 · completed in 3m 16s Run details
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7109
August 4, 2026 05:40
Destroyed
Contributor
Author
🧩 Resolve · PR #7109
The captured CI failures predate the trusted head’s formatting fix. The trusted base is already an ancestor, the worktree is clean, and no merge or additional repair is required. Automatic · PR updated + CI failed · attempt 1 of 3 · completed in 2m 11s Run details
|
serrrfirat
marked this pull request as ready for review
August 4, 2026 07:55
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7109
August 4, 2026 07:55
Destroyed
Contributor
Author
🧩 Resolve · PR #7109
Commit 3fbabfe →Committed 3fbabfe, correcting the stale Automatic · PR updated + CI failed · attempt 1 of 3 · completed in 2m 9s Run details
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7109
August 4, 2026 08:08
Destroyed
cargo fmt requires the call split across lines to satisfy the max-width rule. The prior fix commit addressed the state.rs type mismatch but left this formatting violation.
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7109
August 4, 2026 08:11
Destroyed
Contributor
Author
🔎 Review · PR #7109
Submitted review →Reviewed the complete trusted base-to-head comparison across all three changed files. The binary-document write guard, extracted-versus-raw read-state tracking, fingerprint validation, and caller-level DOCX preservation test are coherent. No concrete actionable defects were found. Automatic · PR updated · attempt 1 of 3 · completed in 1m 25s Run details
|
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
🔍 Review complete · PR #7109
✅ No actionable findings
Reviewed the complete trusted base-to-head comparison across all three changed files. The binary-document write guard, extracted-versus-raw read-state tracking, fingerprint validation, and caller-level DOCX preservation test are coherent. No concrete actionable defects were found.
Validation and technical details
- Inspected the full diff from refs/ironloop/base (74778ba) to refs/ironloop/head (30c3d9f).
- Reviewed surrounding read_file, write_file, apply_patch, read-state, binary-probe, document-extraction, locking, and filesystem-permission code paths.
- Verified all CodingReadStates record and lookup call sites were updated consistently for ReadState and ReadRepresentation.
- Inspected the caller-level DOCX regression test, including its assertion that original bytes remain unchanged after the rejected write.
- git diff --check completed successfully.
- Targeted Rust tests could not run because cargo is unavailable in the review environment (/bin/bash: cargo: command not found).
- Base:
main - Head:
ironloop/issue-6898-3bd5c877d222at30c3d9f - Run:
85b737a2-e7dc-45c6-8d07-81188eb13b19
Contributor
Author
🧩 Resolve · PR #7109
No additional change is required. The current branch already preserves the PR head, includes the trusted-base merge, and contains follow-up commits correcting the read-state type mismatch and formatting failure associated with the captured CI run. The worktree is clean and there are no review threads or merge conflicts to resolve. Automatic · CI failed · attempt 1 of 3 · completed in 1m 40s Run details
|
The upfront is_binary_document_path check rejected all .pdf writes, including new-file authoring. That broke legitimate PDF generation encoded by webui_v2_e2e::agent_produced_workspace_files_are_listable_and_downloadable and reporoducible across the product surface. Issue #6898 targets overwriting existing binary documents whose read_file returned extracted text (fingerprint-bypass corruption), not new-file creation. Split the guard: - is_opaque_binary_document_path (doc/docx/xls/xlsx/ppt/pptx) stays always-reject: text write_file can never produce valid OOXML/OLE. - is_pdf_document_path rejects only when an existing regular file is being overwritten; new PDF creation falls through to the normal write path. - ReadRepresentation guard and reject_binary_probe in verify_read_before_edit remain the core overwrite protection. Tests: - builtin_write_file_rejects_docx_after_extracted_read (unchanged) - builtin_write_file_rejects_existing_pdf_overwrite_without_changing_bytes (new) - builtin_write_file_allows_new_pdf_creation (new) - webui_v2_e2e::agent_produced_workspace_files_are_listable_and_downloadable (unblocked)
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7109
August 4, 2026 08:46
Destroyed
Contributor
Author
🔎 Review · PR #7109
Submitted review →Reviewed the complete trusted base-to-head comparison. No concrete correctness, security, maintainability, or test-coverage defects were found in the binary-document write guards, read-state representation tracking, caller-level regressions, CI planner changes, or guidance cleanup. Automatic · PR updated · attempt 1 of 3 · completed in 1m 58s Run details
|
Contributor
Author
There was a problem hiding this comment.
🔍 Review complete · PR #7109
✅ No actionable findings
Reviewed the complete trusted base-to-head comparison. No concrete correctness, security, maintainability, or test-coverage defects were found in the binary-document write guards, read-state representation tracking, caller-level regressions, CI planner changes, or guidance cleanup.
Validation and technical details
- Inspected all 9 changed files across refs/ironloop/base..refs/ironloop/head and surrounding coding-tool call paths.
- git diff --check refs/ironloop/base..refs/ironloop/head passed.
- Python CI-planner suite passed: 43 tests.
- Focused Rust tests could not be executed because cargo is unavailable in the review environment; their implementation and assertions were inspected directly.
- Base:
main - Head:
ironloop/issue-6898-3bd5c877d222atee8ebff - Run:
cff87734-0698-487f-9647-e3531f67592a
Collaborator
Railway preview acceptance evidenceTested PR head DOCX corruption regression — PASS
This confirms the live preview rejects the corrupting overwrite before mutation and leaves the document readable. New-PDF non-regression — PASS
Scope noteThis browser journey proves the DOCX extension-guard path. It does not independently isolate the representation-aware guard because the DOCX extension check would reject the write first; a focused RTF caller-level regression would still strengthen proof of that second invariant. No frontend, streaming, role/permission, or external-provider cases were run because this PR changes the host-side coding-tool overwrite guard. |
serrrfirat
previously approved these changes
Aug 4, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 4, 2026
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7109
August 4, 2026 09:59
Destroyed
github-actions
Bot
added
size: L
Contributor
Author
🔎 Review · PR #7109
GitHub request failed IronLoop could not complete a required GitHub request. Automatic · PR updated · attempt 1 of 3 · failed after 1m 28s Failure details
|
Collaborator
|
CI fix evidence for
The coverage report is merge-queue-only and is therefore skipped on the PR-head run; the same gate was run locally with the CI toolchain/flags and passed. |
serrrfirat
approved these changes
Aug 4, 2026
4 of 7 tasks
6 of 21 tasks
personal-upstream-sync Bot
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Aug 13, 2026
…ML, and fix the nearai#7109 text-log regression (nearai#7163) * fix(coding): stop the binary-write backstop rejecting ordinary text logs Follow-up to nearai#7109, which shipped with the binary backstop in `verify_read_before_edit` using the STRICT probe (any NUL in the first 8KiB) while `read_file` admits text carrying a few stray NULs via `reject_binary_probe_lenient`. The two disagree about what "binary" means, so a syslog the model had just read successfully became unwritable — and was reported as a "binary document", which it is not. `read_file_tolerates_stray_nul_and_invalid_utf8_in_text_logs` pins the lenient read; this makes the write path agree with it. `apply_patch` keeps applying the strict probe itself, where byte-fidelity for write-back demands it. Also adds the coverage nearai#7109 landed without: - `builtin_write_file_still_overwrites_text_log_with_stray_nul` — fails before this fix, and is the regression above. - `builtin_write_file_rejects_extracted_read_representation_at_unlisted_extension` — the recorded-read-representation guard had no test at all. The existing docx test returns at the extension guard before `verify_read_before_edit` is ever reached, so `ReadState`, the `read_file` plumbing and the representation check were untested. `.rtf` is the one format `read_file` extracts that the extension lists omit, making it that guard's only live surface. - `builtin_apply_patch_rejects_a_binary_document_with_an_actionable_reason` — nearai#6898 named apply_patch's opaque failure as part of the bug, and the fix addresses it, but nothing pinned it. - `reborn_integration_document_edit` — the whole journey at the integration tier: upload a .docx, ask for an edit, and the bytes served by the production `InboundAttachmentReader` (the WebUI download path) are byte-identical to the upload. Refs nearai#6898, nearai#7109 * feat(documents): structure-preserving docx/xlsx/pptx editing and HTML-to-PDF Issue nearai#6898 item 3: the deferred "real document round-trip capability". This is the library layer; capability wiring follows. The governing rule is copy-through: rewrite only the parts an edit targets, copy every other zip entry byte-for-byte. A generator that rebuilds a document from the text a model saw drops everything the model never saw — styles, numbering, headers, images, embedded objects — and the file still opens, so the loss is invisible. Copy-through makes that impossible by construction rather than by diligence, which is why this is not "add docx-rs and write a docx". - `ooxml`: package read/write plus an event-level XML transform. An event the handler does not claim is re-emitted exactly as read, so attribute order, namespace prefixes and self-closing forms survive; a DOM round trip normalizes all three and churns untouched parts. - `docx`: paragraphs with `w:ins`/`w:del` surfaced as typed revisions (flat extraction shows deleted text as if it were still in the contract), and accept/reject. Rejecting a deletion converts `w:delText` back to `w:t` — without that Word renders the restored run empty. Revisions split across runs coalesce into one span. - `xlsx`: shared strings resolved so headers read as text, formula edits that drop the stale cached `<v>` and set `fullCalcOnLoad`, and new cells inserted in ascending column order (Excel repairs, and silently drops content from, an out-of-order row). - `pptx`: slide clone that duplicates the slide's rels part, so the copy inherits its layout — style lives in the layout chain, not the slide, which is why constructing a slide cannot preserve it. Registers the new part in content-types, presentation rels and slide order. - `html_pdf`: PDF is deliberately not edited. A documented HTML subset renders via printpdf's standard-14 fonts. printpdf's `html` feature is left off on purpose: it resolves fonts through rust-fontconfig, which scans system fonts and would make pagination depend on the host. 51 tests, including the copy-through invariants (unrelated parts stay bit-identical, a no-op edit is byte-stable) and every trap named above. Refs nearai#6898 * feat(coding): document_edit and html_to_pdf, and read_file reads OOXML structurally Wires issue nearai#6898 item 3's library layer to the model. Reads unify into read_file; writes stay typed. That asymmetry is the design, not a compromise: - read_file on a .docx/.xlsx/.pptx now returns the ADDRESSABLE STRUCTURE (paragraphs with tracked-change spans, cells with resolved headers and formulas, slides) and records `ReadRepresentation::Structured`. Folded in rather than offered as a `document_read` tool because a model reaches for read_file on whatever path it is handed — a tool it had to know to prefer would go unused while read_file kept returning tag-stripped text that shows a redline's DELETED words as if they were still in the contract. - write_file cannot be folded: its contract is (path, content: &str), so it cannot express "accept the revision in p3". Overloading it means regenerating the document from text — the corruption nearai#6898 banned — or a sometimes-JSON `content`. apply_patch fails for the same reason plus ambiguity: its anchors match extracted text, and mapping a match back to runs is undefined when a string spans a revision boundary. The binary-write ban stays permanent. - document_edit takes typed ops and always writes to a NEW path, so a bad edit can never cost the user the original. It requires a prior Structured read of the source, keeping write_file's mid-air-collision guarantee on a fingerprint over the same raw bytes. - html_to_pdf renders; it refuses to overwrite an existing file, because silently replacing a PDF the user uploaded is the same class of loss the binary-write guard prevents. Two findings from writing the tests, both fixed here: 1. The surface test caught that a builtin capability is invisible to the model without a published input schema — registration alone is not enough. Both new tools now publish one. 2. The xlsx journey caught that set_cell_formula could not create a row. A totals row sits just below the data, so "the row is not there yet" is the ordinary case, not an edge case; the crate fixture happened to have the row already and masked it. Rows are now created in ascending order, with regression tests. Tests: 6 capability tests through real dispatch (including that a Structured read still does NOT authorize a raw write_file overwrite — the two guards must not cancel each other), and four integration journeys on real OOXML fixtures: docx redlines resolved into a clean copy, an xlsx total under a named column, a pptx slide cloned with the source's layout, and a PDF produced by authoring HTML and rendering it. Refs nearai#6898 * test(reborn): register e2e coverage for document_edit and html_to_pdf `reborn_builtin_first_party_capability_e2e_coverage_is_complete` requires every always-on first-party capability to name where its Reborn e2e coverage lives. The two new document capabilities had that coverage — `reborn_integration_document_edit` drives all four journeys — but were not registered, so the guardrail failed. Caught by the pre-push hook, which runs the full workspace suite; the per-crate runs used while developing never touch this test. * ci(planner): classify tests/fixtures document fixtures `Detect Reborn test scope` failed with "unmapped test or CI path: tests/fixtures/contract.docx". The planner deliberately hard-errors on any unclassified path under tests/ to force a per-file decision, and binary document fixtures had no arm — only recorded LLM traces under tests/fixtures/llm_traces/ were mapped. These fixtures are consumed by integration tests through `include_bytes!`, so a changed fixture changes what those tests assert; it schedules a representative integration lane, matching how shared integration support is treated. Adds the matching planner test. * ci: satisfy the panic check in the test-only fixture builder `Fast deterministic checks` flagged four unwraps in `ironclaw_documents::test_fixtures`. The module is `#[cfg(test)]`-gated (`has_cfg_test_module_declaration` agrees), but the checker's main scan path does not consult that for crate-root modules, so it reads them as production code. Uses the inline `// safety:` suppression the checker documents rather than relaxing the checker, which guards a real invariant for everything else. The comments must be on the same line as the call to take effect. * fix(documents): nested paragraphs and leaked revision flags corrupted docx edits Two critical defects from CodeRabbit's review of nearai#7163, both confirmed by tests that fail before the fix. 1. Nested `w:p` lost the outer paragraph and desynchronised ids. Word nests a paragraph inside a paragraph when a run holds a text box (`w:txbxContent`). The reader kept one `current` slot, so the inner Start overwrote the outer and the outer was never emitted; the writer meanwhile counted EVERY `w:p` Start. Read ids and write ids therefore addressed different paragraphs, so an edit landed on the wrong one. The reader now keeps a stack and assigns ids in Start order, matching how the writer counts, and both write paths keep a target stack so a nested paragraph's End restores the enclosing paragraph's state instead of clearing it. Note the mechanism: table cells do NOT reproduce this — `w:tbl`/`w:tc` paragraphs are siblings in document order. Only a text box nests. 2. Revision flags leaked past a dropped subtree and unbalanced the XML. Reject-insert and accept-delete set `dropping = 1` AND the `in_insert`/`in_delete` flag. The dropping branch then consumed the matching End, so the flag was never cleared and stayed set for the rest of the document — deleting a LATER paragraph's `</w:ins>`. Resolving revisions in one paragraph emitted `word/document.xml` with an unclosed element, which Word rejects. The flag is now set only on the unwrap paths, where the End genuinely must be dropped by the handler. On the drop-subtree paths the End is consumed by the dropping branch and no flag is needed. Both defects are invisible to single-paragraph, flat fixtures — which is what the crate's own fixtures were. * fix(documents): preserve cell styles, resolve sheets by relationship, keep text around comments Four more defects from the nearai#7163 review, each with a test that fails before its fix. xlsx: - Replacing a cell dropped its `s` style index, silently reverting a currency or date column to General. That is the precise "preserve what you did not touch" promise the crate exists for, so the style now rides across the replacement. - Sheet names were paired to worksheet parts POSITIONALLY. I shortcut this deliberately and said so in a comment; the reviewer was right that it is wrong. Sheet declaration order does not have to match worksheet file numbering, so an edit could land in the wrong worksheet. Names now resolve through `r:id` in `xl/_rels/workbook.xml.rels`, falling back to positional pairing only when the rels part is absent. html_pdf: - A comment or declaration cleared the buffered text before it, so `<p>hello <!-- note --> world</p>` rendered as `world`. This directly contradicted the module's claim that wrapping markup never swallows content. Whitespace now also collapses across the resulting span join, so the repaired text reads `hello world` rather than `hello world`. - A stray `&` consumed up to ten following characters looking for `;`, swallowing a real entity behind it. The scan now stops at any character that cannot appear in an entity name. * fix(documents): reject duplicate zip entries, stop self-closing tags corrupting reads and rows Three more from the nearai#7163 review. - A duplicate zip entry name kept both names but only the last bytes, so `write()` emitted the same content under both and silently rewrote a package we were asked to preserve. Now rejected at read. - `Event::Empty` latched `in_value`/`in_formula`. A self-closing `<v/>` has no matching `End`, so the flag stayed set and the NEXT cell's text was attributed to the empty one, corrupting every later value in the row. - A self-closing `<row r="N"/>` target produced a SECOND row with the same `r`, which Excel repairs by dropping content. The existing empty row is now replaced in place. * chore: retrigger Railway preview * fix(documents): address review findings * test(reborn): refresh read-file golden payloads --------- Co-authored-by: serrrfirat <f@nuff.tech>
kshitijk4poor
pushed a commit
to NousResearch/hermes-agent
that referenced
this pull request
Aug 14, 2026
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.
skappafrost
pushed a commit
to skappafrost/hermes-agent
that referenced
this pull request
Aug 15, 2026
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.
TrungKiencding
pushed a commit
to TrungKiencding/AgentX-Workmate
that referenced
this pull request
Aug 25, 2026
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. (cherry picked from commit 6d51c83)
bobaba76
pushed a commit
to bobaba76/hermes-agent
that referenced
this pull request
Aug 27, 2026
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.
l3ocifer
pushed a commit
to l3ocifer/frick-ironclaw
that referenced
this pull request
Sep 3, 2026
* fix(coding): reject text writes to binary documents * style(coding): format binary document path check * fix(coding): correct read state guard type * style(coding): format extract_document_text_for_read_file call cargo fmt requires the call split across lines to satisfy the max-width rule. The prior fix commit addressed the state.rs type mismatch but left this formatting violation. * fix(coding): split binary-document guard so new PDF creation stays valid The upfront is_binary_document_path check rejected all .pdf writes, including new-file authoring. That broke legitimate PDF generation encoded by webui_v2_e2e::agent_produced_workspace_files_are_listable_and_downloadable and reporoducible across the product surface. Issue nearai#6898 targets overwriting existing binary documents whose read_file returned extracted text (fingerprint-bypass corruption), not new-file creation. Split the guard: - is_opaque_binary_document_path (doc/docx/xls/xlsx/ppt/pptx) stays always-reject: text write_file can never produce valid OOXML/OLE. - is_pdf_document_path rejects only when an existing regular file is being overwritten; new PDF creation falls through to the normal write path. - ReadRepresentation guard and reject_binary_probe in verify_read_before_edit remain the core overwrite protection. Tests: - builtin_write_file_rejects_docx_after_extracted_read (unchanged) - builtin_write_file_rejects_existing_pdf_overwrite_without_changing_bytes (new) - builtin_write_file_allows_new_pdf_creation (new) - webui_v2_e2e::agent_produced_workspace_files_are_listable_and_downloadable (unblocked) * test(coding): cover binary document guards --------- Co-authored-by: aiworkbot <220660587+aiworkbot@users.noreply.github.com> Co-authored-by: firat.sertgoz <firat.sertgoz@near.ai> Co-authored-by: serrrfirat <f@nuff.tech>
l3ocifer
pushed a commit
to l3ocifer/frick-ironclaw
that referenced
this pull request
Sep 3, 2026
…ML, and fix the nearai#7109 text-log regression (nearai#7163) * fix(coding): stop the binary-write backstop rejecting ordinary text logs Follow-up to nearai#7109, which shipped with the binary backstop in `verify_read_before_edit` using the STRICT probe (any NUL in the first 8KiB) while `read_file` admits text carrying a few stray NULs via `reject_binary_probe_lenient`. The two disagree about what "binary" means, so a syslog the model had just read successfully became unwritable — and was reported as a "binary document", which it is not. `read_file_tolerates_stray_nul_and_invalid_utf8_in_text_logs` pins the lenient read; this makes the write path agree with it. `apply_patch` keeps applying the strict probe itself, where byte-fidelity for write-back demands it. Also adds the coverage nearai#7109 landed without: - `builtin_write_file_still_overwrites_text_log_with_stray_nul` — fails before this fix, and is the regression above. - `builtin_write_file_rejects_extracted_read_representation_at_unlisted_extension` — the recorded-read-representation guard had no test at all. The existing docx test returns at the extension guard before `verify_read_before_edit` is ever reached, so `ReadState`, the `read_file` plumbing and the representation check were untested. `.rtf` is the one format `read_file` extracts that the extension lists omit, making it that guard's only live surface. - `builtin_apply_patch_rejects_a_binary_document_with_an_actionable_reason` — nearai#6898 named apply_patch's opaque failure as part of the bug, and the fix addresses it, but nothing pinned it. - `reborn_integration_document_edit` — the whole journey at the integration tier: upload a .docx, ask for an edit, and the bytes served by the production `InboundAttachmentReader` (the WebUI download path) are byte-identical to the upload. Refs nearai#6898, nearai#7109 * feat(documents): structure-preserving docx/xlsx/pptx editing and HTML-to-PDF Issue nearai#6898 item 3: the deferred "real document round-trip capability". This is the library layer; capability wiring follows. The governing rule is copy-through: rewrite only the parts an edit targets, copy every other zip entry byte-for-byte. A generator that rebuilds a document from the text a model saw drops everything the model never saw — styles, numbering, headers, images, embedded objects — and the file still opens, so the loss is invisible. Copy-through makes that impossible by construction rather than by diligence, which is why this is not "add docx-rs and write a docx". - `ooxml`: package read/write plus an event-level XML transform. An event the handler does not claim is re-emitted exactly as read, so attribute order, namespace prefixes and self-closing forms survive; a DOM round trip normalizes all three and churns untouched parts. - `docx`: paragraphs with `w:ins`/`w:del` surfaced as typed revisions (flat extraction shows deleted text as if it were still in the contract), and accept/reject. Rejecting a deletion converts `w:delText` back to `w:t` — without that Word renders the restored run empty. Revisions split across runs coalesce into one span. - `xlsx`: shared strings resolved so headers read as text, formula edits that drop the stale cached `<v>` and set `fullCalcOnLoad`, and new cells inserted in ascending column order (Excel repairs, and silently drops content from, an out-of-order row). - `pptx`: slide clone that duplicates the slide's rels part, so the copy inherits its layout — style lives in the layout chain, not the slide, which is why constructing a slide cannot preserve it. Registers the new part in content-types, presentation rels and slide order. - `html_pdf`: PDF is deliberately not edited. A documented HTML subset renders via printpdf's standard-14 fonts. printpdf's `html` feature is left off on purpose: it resolves fonts through rust-fontconfig, which scans system fonts and would make pagination depend on the host. 51 tests, including the copy-through invariants (unrelated parts stay bit-identical, a no-op edit is byte-stable) and every trap named above. Refs nearai#6898 * feat(coding): document_edit and html_to_pdf, and read_file reads OOXML structurally Wires issue nearai#6898 item 3's library layer to the model. Reads unify into read_file; writes stay typed. That asymmetry is the design, not a compromise: - read_file on a .docx/.xlsx/.pptx now returns the ADDRESSABLE STRUCTURE (paragraphs with tracked-change spans, cells with resolved headers and formulas, slides) and records `ReadRepresentation::Structured`. Folded in rather than offered as a `document_read` tool because a model reaches for read_file on whatever path it is handed — a tool it had to know to prefer would go unused while read_file kept returning tag-stripped text that shows a redline's DELETED words as if they were still in the contract. - write_file cannot be folded: its contract is (path, content: &str), so it cannot express "accept the revision in p3". Overloading it means regenerating the document from text — the corruption nearai#6898 banned — or a sometimes-JSON `content`. apply_patch fails for the same reason plus ambiguity: its anchors match extracted text, and mapping a match back to runs is undefined when a string spans a revision boundary. The binary-write ban stays permanent. - document_edit takes typed ops and always writes to a NEW path, so a bad edit can never cost the user the original. It requires a prior Structured read of the source, keeping write_file's mid-air-collision guarantee on a fingerprint over the same raw bytes. - html_to_pdf renders; it refuses to overwrite an existing file, because silently replacing a PDF the user uploaded is the same class of loss the binary-write guard prevents. Two findings from writing the tests, both fixed here: 1. The surface test caught that a builtin capability is invisible to the model without a published input schema — registration alone is not enough. Both new tools now publish one. 2. The xlsx journey caught that set_cell_formula could not create a row. A totals row sits just below the data, so "the row is not there yet" is the ordinary case, not an edge case; the crate fixture happened to have the row already and masked it. Rows are now created in ascending order, with regression tests. Tests: 6 capability tests through real dispatch (including that a Structured read still does NOT authorize a raw write_file overwrite — the two guards must not cancel each other), and four integration journeys on real OOXML fixtures: docx redlines resolved into a clean copy, an xlsx total under a named column, a pptx slide cloned with the source's layout, and a PDF produced by authoring HTML and rendering it. Refs nearai#6898 * test(reborn): register e2e coverage for document_edit and html_to_pdf `reborn_builtin_first_party_capability_e2e_coverage_is_complete` requires every always-on first-party capability to name where its Reborn e2e coverage lives. The two new document capabilities had that coverage — `reborn_integration_document_edit` drives all four journeys — but were not registered, so the guardrail failed. Caught by the pre-push hook, which runs the full workspace suite; the per-crate runs used while developing never touch this test. * ci(planner): classify tests/fixtures document fixtures `Detect Reborn test scope` failed with "unmapped test or CI path: tests/fixtures/contract.docx". The planner deliberately hard-errors on any unclassified path under tests/ to force a per-file decision, and binary document fixtures had no arm — only recorded LLM traces under tests/fixtures/llm_traces/ were mapped. These fixtures are consumed by integration tests through `include_bytes!`, so a changed fixture changes what those tests assert; it schedules a representative integration lane, matching how shared integration support is treated. Adds the matching planner test. * ci: satisfy the panic check in the test-only fixture builder `Fast deterministic checks` flagged four unwraps in `ironclaw_documents::test_fixtures`. The module is `#[cfg(test)]`-gated (`has_cfg_test_module_declaration` agrees), but the checker's main scan path does not consult that for crate-root modules, so it reads them as production code. Uses the inline `// safety:` suppression the checker documents rather than relaxing the checker, which guards a real invariant for everything else. The comments must be on the same line as the call to take effect. * fix(documents): nested paragraphs and leaked revision flags corrupted docx edits Two critical defects from CodeRabbit's review of nearai#7163, both confirmed by tests that fail before the fix. 1. Nested `w:p` lost the outer paragraph and desynchronised ids. Word nests a paragraph inside a paragraph when a run holds a text box (`w:txbxContent`). The reader kept one `current` slot, so the inner Start overwrote the outer and the outer was never emitted; the writer meanwhile counted EVERY `w:p` Start. Read ids and write ids therefore addressed different paragraphs, so an edit landed on the wrong one. The reader now keeps a stack and assigns ids in Start order, matching how the writer counts, and both write paths keep a target stack so a nested paragraph's End restores the enclosing paragraph's state instead of clearing it. Note the mechanism: table cells do NOT reproduce this — `w:tbl`/`w:tc` paragraphs are siblings in document order. Only a text box nests. 2. Revision flags leaked past a dropped subtree and unbalanced the XML. Reject-insert and accept-delete set `dropping = 1` AND the `in_insert`/`in_delete` flag. The dropping branch then consumed the matching End, so the flag was never cleared and stayed set for the rest of the document — deleting a LATER paragraph's `</w:ins>`. Resolving revisions in one paragraph emitted `word/document.xml` with an unclosed element, which Word rejects. The flag is now set only on the unwrap paths, where the End genuinely must be dropped by the handler. On the drop-subtree paths the End is consumed by the dropping branch and no flag is needed. Both defects are invisible to single-paragraph, flat fixtures — which is what the crate's own fixtures were. * fix(documents): preserve cell styles, resolve sheets by relationship, keep text around comments Four more defects from the nearai#7163 review, each with a test that fails before its fix. xlsx: - Replacing a cell dropped its `s` style index, silently reverting a currency or date column to General. That is the precise "preserve what you did not touch" promise the crate exists for, so the style now rides across the replacement. - Sheet names were paired to worksheet parts POSITIONALLY. I shortcut this deliberately and said so in a comment; the reviewer was right that it is wrong. Sheet declaration order does not have to match worksheet file numbering, so an edit could land in the wrong worksheet. Names now resolve through `r:id` in `xl/_rels/workbook.xml.rels`, falling back to positional pairing only when the rels part is absent. html_pdf: - A comment or declaration cleared the buffered text before it, so `<p>hello <!-- note --> world</p>` rendered as `world`. This directly contradicted the module's claim that wrapping markup never swallows content. Whitespace now also collapses across the resulting span join, so the repaired text reads `hello world` rather than `hello world`. - A stray `&` consumed up to ten following characters looking for `;`, swallowing a real entity behind it. The scan now stops at any character that cannot appear in an entity name. * fix(documents): reject duplicate zip entries, stop self-closing tags corrupting reads and rows Three more from the nearai#7163 review. - A duplicate zip entry name kept both names but only the last bytes, so `write()` emitted the same content under both and silently rewrote a package we were asked to preserve. Now rejected at read. - `Event::Empty` latched `in_value`/`in_formula`. A self-closing `<v/>` has no matching `End`, so the flag stayed set and the NEXT cell's text was attributed to the empty one, corrupting every later value in the row. - A self-closing `<row r="N"/>` target produced a SECOND row with the same `r`, which Excel repairs by dropping content. The existing empty row is now replaced in place. * chore: retrigger Railway preview * fix(documents): address review findings * test(reborn): refresh read-file golden payloads --------- Co-authored-by: serrrfirat <f@nuff.tech>
melon-xf
added a commit
to melon-xf/hermes-agent
that referenced
this pull request
Sep 3, 2026
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.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
write_filefor known binary document formats with an actionable errorread_filereturned raw or extracted text so extracted reads cannot authorize raw overwritesTest Strategy
Test card
User behavior: Reading an extracted binary document no longer authorizes
write_fileto replace its raw bytes; the tool fails with an actionable error and preserves the document. New PDF creation remains supported.Risk areas:
Tests added or updated:
ee8ebff34c0fa0d615d06cf80eaa742806148a56; evidence is recorded in the PR conversation.builtin_write_file_rejects_docx_after_extracted_read_without_changing_bytes,builtin_write_file_rejects_existing_pdf_overwrite_without_changing_bytes, andbuiltin_write_file_allows_new_pdf_creationdrive the host-runtime caller path.What the tests prove:
Commands run:
cargo fmt --checkcargo test -p ironclaw_extension_support— 156 tests passedcargo llvm-covforironclaw_extension_support— 156 tests passedscripts/ci/reborn_changed_coverage.pywith base coverage — changed lines 95.83% (69/72), changed branches 92.86% (13/14)git diff --checkCompatibility and rollback
Text-file behavior is unchanged. Binary document writes that previously risked corruption now fail closed. New PDF creation remains supported. Rollback is limited to the commits in this PR, with no schema or persistence changes.
Closes #6898