mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-03 08:06:01 +08:00
7d77a752f2
* fix(coding): stop the binary-write backstop rejecting ordinary text logs Follow-up to #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 #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` — #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 #6898, #7109 * feat(documents): structure-preserving docx/xlsx/pptx editing and HTML-to-PDF Issue #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 #6898 * feat(coding): document_edit and html_to_pdf, and read_file reads OOXML structurally Wires issue #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 #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 #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 #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 #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 #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>