Add plugin document-skills - #12
Conversation
Four original workbench Skills (docx, pdf, pptx, xlsx) for creating, reading, editing, and verifying Office and PDF documents with standard open-source Python tooling. - lives at plugins/Hylouis233/document-skills - plugin.json + README + Apache-2.0 LICENSE + four Skills with references - original guidance written for this plugin; no proprietary content reused - no network, no credentials, no MCP server; scripts referenced are the user's own installed Python libraries (python-docx, python-pptx, openpyxl, pypdf, reportlab, pymupdf), not bundled - npm run validate passes; documented patterns smoke-tested end to end
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26c09763c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
hetaoBackend
left a comment
hetaoBackend
left a comment
There was a problem hiding this comment.
暂不合并。当前固定 commit 26c0976 仍有 17 个未解决且未 outdated 的 review threads，其中多项会让文档操作静默漏数据、生成损坏文件或让强制 postcheck 自身失败（PDF 只处理最后一页/未写出 watermark、DOCX 跨 run 替换与 OOXML repack、XLSX range styling/跨 sheet formula audit/固定 Summary sheet、PPTX 模糊匹配与格式保留等）。
请逐项修复或以可运行证据说明不成立，并 resolve 对应 thread；然后更新 end-to-end evidence。仓库 validator 与 ast.parse 只能证明包形状/语法正确，不能替代这些行为正确性检查。修复后我会对新 head 重新执行 npm run check 和文档片段验证。
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2907c51b66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
hetaoBackend
left a comment
hetaoBackend
left a comment
There was a problem hiding this comment.
@Hylouis233 最新 head 的 CI/CodeQL 已通过，且旧 review 中多数问题已修复；但当前仍有会让用户照抄失败或损坏文档的代码：
- PDF AcroForm 路径没有先把 reader pages 与
/AcroFormclone/append 到 writer，还把 reader page 传给 writer update；会报错或生成不完整文件。 - CMYK Pixmap 直接保存 PNG 会失败并中断抽取。
- PPTX table cell 的
.text = ...会重建 text frame，丢 styled runs/hyperlink。 - CSV Sniffer 的返回 dialect 没传给 DictReader，分号 CSV 仍按逗号解析。
- PDF 尺寸验证用四坐标 mediabox tuple 对比 width/height 二元尺寸，无法正确验证 A4/Letter。
- grouped shape、继承字体、编号重启、openpyxl unsupported extensions 仍缺正确处理。
请先修复当前有效 thread，并为每种格式最危险的 1–2 个 snippet 增加最小可运行 fixture 测试。绿灯目前只能证明包结构。
Review fixes (all fixture-verified in tests/): - PDF: AcroForm route now clones pages + /AcroForm into the writer via append() and fills fields on writer pages; mediabox postcheck reduced to width/height pairs; CMYK/ICC pixmaps converted to RGB before PNG save - PPTX: table cells edited at run level (cell.text rebuild proven lossy); inventory walker recurses into group shapes; font triage resolves inherited fonts with theme major/minor fallback - XLSX: sniffed CSV dialect passed to DictReader; round_trip_losses() detects parts an openpyxl round trip drops before saving - DOCX: numbering restart via cloned <w:num> + startOverride, rendered proof in fixture (restarts at 1 vs style-reuse continuing at 4) Depth references (original content, no proprietary material): - docx cjk.md: east-asian font slot, 字号 table, char-based indent, fixed line spacing, GB/T 9704 page geometry, font availability risks - docx scenes.md: academic paper / resume / official document / contract skeletons with scene-specific verification - xlsx formatting.md: conditional formatting rules, structured tables, honest pivot aggregation routes (openpyxl cannot create pivots) tests/: one runnable fixture per format, 30 assertions, all passing locally (pdf 9, pptx 10, xlsx 6, docx 5 incl. soffice-rendered proof)
|
Round-2 review feedback is addressed in f803513 (18 files, +954). Every item was fixed and covered by a runnable fixture, per the request for minimal tests of the most dangerous snippets per format: Fixes
Fixtures — Depth references added (original content): All round-2 threads have been replied to and resolved/outdated. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f803513495
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- xlsx edit: round_trip_changes() now also detects extensions stripped from retained parts (x14/extLst markers), not just dropped archive members - docx scenes: signature blocks get row-level cantSplit + keep_with_next on all rows (rendered fixture proves the table stays on one page) - docx cjk: tofu postcheck switched to glyph-coverage (fontTools cmap) since text extraction cannot detect missing-glyph boxes; style snippet now defines font names before use (NameError fix) - pptx edit: locator candidate collection recurses into groups with a stable nested path for the uniqueness assertion - xlsx formatting: aggregation formulas build sheet refs from ws.title (quoting when needed) instead of hard-coded Data! - xlsx read: profiles every sheet by default, not just sheetnames[0] - pdf create: escape() rule for plain text into Paragraph; fixture shows unescaped markup silently swallows <...> runs Fixtures extended to 44 assertions, all passing locally
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f0900859c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64049ebb7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b671a22e7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12d9542c43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2afc97a6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- pdf extract: real table detection via page.find_tables() with span fallback - pdf postcheck: interactive-only pages (AcroForm widgets) exempt from text gate - docx read: unified block walker yields tables inside w:sdtContent - README: drop the untagged-ReportLab accessibility claim, state the limitation - xlsx: fullCalcOnLoad contract so manual-calc workbooks recalculate on open - fixtures: table detection, widget exemption, sdt table walker, calc flags (all green)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 746e663d1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- pptx analyze: sparse XY/bubble cache points keep their idx so x/y/bubble values pair correctly across blank points - docx cjk: Hangul (jamo, compatibility jamo, extended-A, syllables) routes through the eastAsia font slot - xlsx edit: extension detection matches namespace URIs and the local name extLst instead of arbitrary XML prefixes - xlsx formatting: header-only sheets skip conditional formatting instead of building inverted ranges openpyxl rejects - docx read: table extraction walks real w:tc elements and annotates gridSpan/vMerge instead of the merge-expanded row.cells view - pdf transform: stamps are scaled and centered per destination page via merge_transformed_page; rotated pages flagged for visual verification - fixtures extended for all six (with negative controls)
|
Cross-audit round complete. This pass re-verified every open review thread against the code, closed the five remaining items from the last round in 746e663 (PDF table detection via |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6b0fdb7ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- pdf postcheck: real overflow check - text blocks extracted through an enlarged clip rectangle and compared to the page box (plain extraction silently drops fully off-page text) - docx read: table cells rebuilt per paragraph with visible tab/break markers instead of concatenated w:t text - docx/pptx health checks: explicit require() raises so every archive limit survives python -O - pptx analyze: run fonts resolve a:latin/a:ea/a:cs by the run's scripts before falling back to the Latin-only font.name; triage walks table cells (graphic frames have no text frame) - docx SKILL: pandoc-only installs can serve read/extract requests - pdf extract: inline content-stream images enumerated via dict image blocks alongside the XObject loop; coordinate spans position-sorted before emission - xlsx read: implausible sheet <dimension> reset before streaming - fixtures extended for the overflow check (with negative control), cell paragraph boundaries, dimension corruption, script faces, table triage
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98af2fc093
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # Insert/delete does not adjust formulas or non-cell dependencies. Fail closed until every | ||
| # reported reference that can intersect the shifted region has an explicit rewrite plan. | ||
| references_before = non_cell_references(wb) |
There was a problem hiding this comment.
When the workbook contains an ordinary cell formula that references rows at or below row 5, this gate allows the insertion because non_cell_references() deliberately excludes cell formulas; insert_rows(5) then moves cells without translating their formulas, producing stale ranges and incorrect results. The later loop only prints the already-modified formulas and has no pre-edit snapshot with which to identify or repair them, so inventory intersecting cell formulas before allowing the structural edit.
Useful? React with 👍 / 👎.
| value_ws.reset_dimensions() | ||
| formula_ws.reset_dimensions() | ||
| print(f"--- {sheet_name} --- implausible dimension {declared!r}; reset, real extent:") | ||
| print(f"--- {sheet_name} --- dims:", value_ws.calculate_dimension()) |
There was a problem hiding this comment.
When the revised bad-dimension branch runs, reset_dimensions() clears a read-only worksheet's cached max_row and max_column, so the immediate calculate_dimension() call raises ValueError: Worksheet is unsized, use calculate_dimension(force=True) instead of profiling the recovered data. Fresh evidence after the earlier dimension fix is this new unforced call on line 28; use calculate_dimension(force=True) or iterate first and report the discovered extent.
Useful? React with 👍 / 👎.
| if any(tag in ("Hans", "Hant", "Jpan", "Hang") for tag in tags) and "eastAsia" in declared: | ||
| return declared["eastAsia"] | ||
| if any(tag in ("Arab", "Hebr", "Deva") for tag in tags) and "complexScript" in declared: | ||
| return declared["complexScript"] | ||
| return declared.get("latin") |
There was a problem hiding this comment.
When one run contains both Latin and CJK or complex-script text and declares separate faces, the new script-first branch returns only the East Asian or complex-script face and omits the Latin face that renders the other characters. Fresh evidence in the revised resolver is that it still returns one scalar face, so common text such as Q3 测试 yields only the CJK font and the promised effective-font inventory remains incomplete; return the distinct applicable faces per script instead.
Useful? React with 👍 / 👎.
| # or a:cs must be resolved per the scripts present in its own text first. | ||
| def explicit_run_face(run, text): | ||
| """The run's direct a:latin/a:ea/a:cs typeface chosen by its scripts (or None).""" | ||
| rPr = run._r.find(qn("a:rPr")) |
There was a problem hiding this comment.
When the font-triage code block is run independently, as the separate fenced example suggests, it fails on the first run with NameError: name 'qn' is not defined; it later also calls iter_shapes, which is defined only in the earlier content-inventory block. Import qn and include the recursive shape walker in this block so the advertised triage route can execute without first running an unrelated extraction snippet in the same interpreter.
Useful? React with 👍 / 👎.
| [[cell.text for cell in row.cells] for row in sh.table.rows] | ||
| for sh in shapes if sh.has_table |
There was a problem hiding this comment.
When a slide table contains merged cells, this rectangular cell.text matrix does not record which cell is the merge origin or how many rows and columns it spans; python-pptx exposes continuation cells separately, so a multi-column heading is emitted as one populated cell plus indistinguishable empty cells. That loses table structure during the advertised extraction and can cause summaries or repurposed tables to assign headings to the wrong columns; include is_merge_origin, is_spanned, span_width, and span_height metadata.
Useful? React with 👍 / 👎.
hetaoBackend
left a comment
hetaoBackend
left a comment
There was a problem hiding this comment.
已复核当前 head 5b89545，暂不能合入。当前 head 仍有多项会导致文档内容遗漏、误改或验证失真的未解决问题，包括 DOCX 超链接/altChunk/表格网格处理、PPTX SmartArt 清点、PDF 旋转坐标与字体嵌入判断、XLSX 公式校验和 drawing anchor 迁移等。另一个门禁问题是仓库 npm check 并未执行本 PR 新增的 4 个 Python fixture；本地仅 PPTX fixture 可直接运行并通过，DOCX/PDF/XLSX 因依赖未安装无法形成独立验证。请先修复当前 review findings，并把四类 fixture 接入可重复的 CI 检查，再请求复核。
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88a3723709
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| temporary_path = output_path.with_suffix(output_path.suffix + ".tmp") | ||
| with temporary_path.open("w", newline="", encoding="utf-8") as output: | ||
| writer = csv.writer(output) | ||
| for formula_row, value_row in zip(formula_ws.iter_rows(), value_ws.iter_rows()): |
There was a problem hiding this comment.
When converting a workbook from a non-Excel producer whose <dimension> metadata is plausible but truncated, both read-only worksheets retain that declared bound, so iter_rows() silently stops before later populated cells. Because both streams truncate identically, zip() detects no mismatch and the resulting CSV is accepted despite missing rows or columns; call reset_dimensions() on both worksheets before streaming, as the workbook-read route does.
Useful? React with 👍 / 👎.
| if widget.get("/T") is not None: | ||
| return str(widget["/T"]) |
There was a problem hiding this comment.
When a form uses a hierarchical field such as application.applicant_name, each /T contains only one partial name, but this function returns the first leaf it encounters instead of joining the parent chain. A caller using the fully qualified name exposed by get_fields() therefore finds no target page and raises KeyError even though the field is fillable; construct the qualified name from all ancestor /T values before comparing.
Useful? React with 👍 / 👎.
| assert not unresolved, f"font files not resolved per run: {unresolved}" | ||
| assert not missing, f"glyph missing from the run's effective font: {missing}" |
There was a problem hiding this comment.
When this mandatory glyph audit runs with python -O or PYTHONOPTIMIZE, both assertions are removed, so unresolved font files and missing glyphs are silently accepted and a document containing tofu can pass the documented postcheck. Use explicit conditionals that raise exceptions so the audit remains active in optimized environments.
Useful? React with 👍 / 👎.
| if r.is_encrypted: | ||
| if not password: | ||
| raise RuntimeError("set PDF_PASSWORD so the encrypted output can be postchecked") |
There was a problem hiding this comment.
When an output PDF is permission-encrypted with an empty user password, as is common for files that open without a password but restrict editing or printing, is_encrypted remains true and this branch rejects it before attempting empty-password authentication. Try the empty password first and require PDF_PASSWORD only if that authentication fails, otherwise a valid, normally viewable output can never pass the mandatory postcheck.
Useful? React with 👍 / 👎.
| for formula_cell, value_cell in zip(formula_row, value_row): | ||
| if formula_cell.data_type == "f" and value_cell.value is None: | ||
| missing_caches.append(formula_cell.coordinate) | ||
| writer.writerow([cell.value for cell in value_row]) |
There was a problem hiding this comment.
When a worksheet contains literal text beginning with =, +, -, or @, this export writes the prefix unchanged, so opening the resulting CSV in Excel or LibreOffice interprets the cell as a formula rather than the original data and can execute attacker-controlled spreadsheet expressions. Apply CSV-injection neutralization for spreadsheet-targeted exports, or explicitly require a raw-data mode before preserving those prefixes.
Useful? React with 👍 / 👎.
|
|
||
| red_fill = PatternFill(start_color="FFC7CE", end_color="FFC7CE", fill_type="solid") | ||
| red_font = Font(color="9C0006") | ||
| last = ws.max_row # real data boundary, not the column |
There was a problem hiding this comment.
When an existing worksheet has a stray styled or previously cleared cell far below its real data, ws.max_row reflects that cell rather than the populated boundary. The snippet can consequently attach several conditional-formatting rules to nearly a million blank rows, bloating the workbook and slowing spreadsheet viewers; scan backward for the last row containing actual data or use the source table's declared range before constructing these rules.
Useful? React with 👍 / 👎.
| infos = z.infolist() | ||
| names = {info.filename for info in infos} |
There was a problem hiding this comment.
When an untrusted DOCX contains hundreds of thousands of distinct zero-byte members, every current size and compression-ratio limit still passes because the total uncompressed size remains tiny. Materializing the complete central directory and iterating every member can then consume unbounded memory and CPU despite this being presented as a bounded health check; impose a compressed-file-size and member-count limit before processing the entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: beade83f43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ph for ph in slide.placeholders | ||
| if ph.placeholder_format.type in types | ||
| ] | ||
| assert len(matches) == 1, f"expected one placeholder of {types}, found {len(matches)}" |
There was a problem hiding this comment.
When this creation route runs with python -O or PYTHONOPTIMIZE and a supplied layout contains multiple matching placeholders, Python removes this assertion and matches[0] silently selects an arbitrary placeholder; with no match it instead fails later with an unhelpful IndexError. Use an explicit conditional exception so template ambiguity cannot populate the wrong placeholder.
Useful? React with 👍 / 👎.
| annotations = list(page.annots() or ()) | ||
| links = page.get_links() | ||
| print(page.number, page.rect, "text_len:", len(page.get_text()), | ||
| "images:", len(page.get_images()), "drawings:", len(drawings), |
There was a problem hiding this comment.
When a PDF page contains only inline images, page.get_images() reports no image because those images live directly in the content stream rather than the page's XObject resources. The documented blank-page predicate can therefore classify a visibly populated page as blank; include type-1 image blocks from page.get_text("dict") or use an equivalent rendered-content check.
Useful? React with 👍 / 👎.
| ) | ||
|
|
||
| with zipfile.ZipFile("input.pptx") as archive: | ||
| infos = archive.infolist() |
There was a problem hiding this comment.
When an untrusted PPTX contains hundreds of thousands of distinct zero-byte members, the per-entry, total-uncompressed-size, and compression-ratio limits all pass while infolist(), the name set, and the subsequent loop consume unbounded memory and CPU. Add a compressed-file-size limit before opening the archive and a member-count limit before processing its entries so this remains a genuinely bounded health check.
Useful? React with 👍 / 👎.
| # extent so valid high-y portrait text is not flagged on a rotated page. | ||
| page_box = fitz.Rect(0, 0, page.cropbox.width, page.cropbox.height) | ||
| clip = fitz.Rect( | ||
| page_box.x0 - 2000, page_box.y0 - 2000, |
There was a problem hiding this comment.
When a generated page contains an image or vector drawing that extends beyond the page box, this mandatory gate examines only text blocks, so the clipped graphical content passes even though shared rule 4 requires all overflowing content to be detected. Include image and drawing placement bounds in the check, or require a rendered visual comparison that can detect clipped non-text content.
Useful? React with 👍 / 👎.
| yield from walk(paragraph._p) | ||
|
|
||
| def paragraph_text(paragraph): | ||
| return "".join(run.text for run in iter_paragraph_runs(paragraph)) |
There was a problem hiding this comment.
When visible content is encoded as a <w:sym> run, such as a Wingdings checkbox or legacy mathematical symbol, Run.text does not expose that element and this concatenation silently drops it from the extracted paragraph. Inspect w:sym children and resolve their font/character codes, or explicitly report them as unreadable so summaries do not omit visible symbols.
Useful? React with 👍 / 👎.
| return sorted( | ||
| (name, label) | ||
| for name in common_names | ||
| for label in before.get(name, set()) - after.get(name, set()) |
There was a problem hiding this comment.
When a retained worksheet contains multiple extension records and openpyxl strips only one while another keeps the same extLst, x14 namespace, or markup-compatibility marker present, this set subtraction reports no change. The guard can therefore authorize an overwrite that loses an unsupported feature; inventory extension entries by URI and relevant child content rather than comparing only three coarse marker labels.
Useful? React with 👍 / 👎.
| start = OxmlElement("w:startOverride") | ||
| start.set(qn("w:val"), "1") | ||
| override.append(start) | ||
| clone.append(override) |
There was a problem hiding this comment.
When the selected base numbering instance already has a <w:lvlOverride w:ilvl="0">, cloning it and appending another override for the same level leaves two competing level-zero definitions. Word or LibreOffice may honor the inherited override rather than the new startOverride=1, so the second list can continue at the wrong number; replace or update the cloned level-zero override instead of blindly appending another one.
Useful? React with 👍 / 👎.
| s = add_slide(title_only_layout) | ||
| s.shapes.title.text = "Regional service health" | ||
| rows, cols = 4, 3 | ||
| tbl_shape = s.shapes.add_table(rows, cols, Inches(0.5), Inches(1.5), Inches(9), Inches(3.5)) |
There was a problem hiding this comment.
When this skeleton is run as written, it creates a four-row table but fills only the header, leaving nine body cells empty. The mandatory PPTX postcheck then reports those cell text frames as unexpected empties, so the documented creation route cannot pass its own verification; populate the three data rows or create only the header row until data is supplied.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 032a97a94c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if overflow_doc.needs_pass: | ||
| if not password: | ||
| raise RuntimeError("set PDF_PASSWORD so the encrypted output can be overflow-checked") | ||
| overflow_doc.authenticate(password) |
There was a problem hiding this comment.
For a permission-encrypted PDF with an empty user password, the earlier r.decrypt("") succeeds, but this independently opened PyMuPDF document still reports needs_pass; when PDF_PASSWORD is unset, this branch therefore rejects a valid PDF before checking overflow. Fresh evidence after the earlier blank-password repair is that only the pypdf reader was authenticated; authenticate overflow_doc with "" first, requiring the environment password only if that fails.
Useful? React with 👍 / 👎.
| for b in page.get_text("dict")["blocks"]: | ||
| if b["type"] != 1: | ||
| continue | ||
| ext = b.get("ext") or "png" | ||
| with open(f"img-p{page_number}-inline-{b['number']}.{ext}", "wb") as fh: | ||
| fh.write(b["image"]) |
There was a problem hiding this comment.
When a page contains an ordinary image XObject, page.get_text("dict") includes a type-1 block for that image as well as for inline images, so this loop exports it again under an inline filename after the get_images() loop already wrote it. This produces duplicate, misleading extraction results for normal image-bearing PDFs; restrict this pass to blocks whose image xref identifies inline content.
Useful? React with 👍 / 👎.
| rows = value_ws.iter_rows(values_only=True) | ||
| header = next(rows, None) | ||
| print("header:", header) | ||
| for i, row in enumerate(rows): | ||
| if i >= 5: break | ||
| print(row) |
There was a problem hiding this comment.
When a worksheet's first populated row is below row 6, iter_rows() still begins at row 1 after dimensions are reset, so header and all five sample rows printed here are blank even though discover_dimension() already found the actual lower bound. The required first-pass profile consequently omits the sheet's real header and data; retain the discovered minimum row or skip leading empty rows before taking the header and samples.
Useful? React with 👍 / 👎.
| if formula_cell.data_type == "f" and value_cell.value is None: | ||
| missing_caches.append(formula_cell.coordinate) |
There was a problem hiding this comment.
When a valid formula such as =IF(A1="","",A1) has a cached empty-string result, openpyxl's data_only=True view also exposes that result as None. This predicate therefore treats a legitimate displayed blank as an absent cache and deletes the temporary CSV instead of exporting it; inspect the underlying cell XML/cache presence or otherwise distinguish cached blanks before rejecting the conversion.
Useful? React with 👍 / 👎.
| if x_source is None: # category/value chart | ||
| return {"name": series.name, "values": list(series.values)} |
There was a problem hiding this comment.
When a category/value chart references workbook or external data but omits the optional <c:numCache>, accessing series.values through python-pptx dereferences the missing cache and aborts the entire slide inventory. The XY branch already reports unavailable caches explicitly; apply the same guarded extraction to category series so a valid uncached chart is reported as unavailable rather than preventing the rest of the deck from being analyzed.
Useful? React with 👍 / 👎.
| if not condition: | ||
| raise ValueError(message) | ||
|
|
||
| page_count = len(r.pages) |
There was a problem hiding this comment.
When a task requests an exact page count, such as a one-page handout, this variable is never compared with an expected value, so an output with extra or missing pages can pass every mandatory check as long as the remaining pages contain text and use the expected geometry. Add an expected_page_count input and fail explicitly when len(r.pages) differs from it.
Useful? React with 👍 / 👎.
| print(page.number, page.rect, "text_len:", len(page.get_text()), | ||
| "images:", len(page.get_images()), "drawings:", len(drawings), |
There was a problem hiding this comment.
When identically sized pages use different /Rotate values, page.rect swaps width and height for the rotated pages, so the instructed consistency check reports mixed paper sizes even though every page has the same media geometry. This is common in scanned or intentionally mixed-orientation documents and can produce a false duplex-printing warning; compare unrotated media or crop boxes and report rotation separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f50cecf3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if doc.needs_pass: | ||
| password = os.environ.get("PDF_PASSWORD") | ||
| if not password or not doc.authenticate(password): | ||
| raise RuntimeError("set PDF_PASSWORD to the correct password before extracting") |
There was a problem hiding this comment.
When a permission-encrypted PDF uses an empty user password and PDF_PASSWORD is unset, this branch raises without attempting doc.authenticate(""), even though the file opens normally. The repaired postcheck and inspection paths already try the empty password first; apply the same sequence here so extraction supports these PDFs.
Useful? React with 👍 / 👎.
| check = PdfReader("filled.pdf") | ||
| value = str((check.get_fields() or {}).get("applicant_name", {}).get("/V", "")) | ||
| if value.strip("/") != "Ada Byron": |
There was a problem hiding this comment.
When field_name is changed to any other simple or qualified key, the update can succeed but this verification still looks up the literal applicant_name, producing a false failure. The parent-chain matcher now supports qualified fields, but the final lookup must use field_name as well.
Useful? React with 👍 / 👎.
| if row_index is None or column_index is None: | ||
| continue | ||
| min_row = row_index if min_row is None else min(min_row, row_index) | ||
| min_column = column_index if min_column is None else min(min_column, column_index) |
There was a problem hiding this comment.
When a read-only worksheet contains a styled-but-empty cell above its actual data, that cell still has row and column coordinates, so this code marks its row as populated even though its value is None. The new min_row=first_populated_row iterator then starts at that blank row and can still print a blank header and samples; only update the discovered bounds for cells containing real values or formulas.
Useful? React with 👍 / 👎.
| for formula_row, value_row in zip(formula_ws.iter_rows(), value_ws.iter_rows()): | ||
| for formula_cell, value_cell in zip(formula_row, value_row): | ||
| if formula_cell.data_type == "f" and value_cell.value is None: | ||
| missing_cache_count += 1 |
There was a problem hiding this comment.
When a formula has a valid cached empty-string result, the data_only=True cell is also None, so this profiler reports it as having no cached value. The CSV route now inspects worksheet XML to distinguish cached blanks from missing caches, but this independent read route still uses the ambiguous value alone and should apply the same cache-presence check.
Useful? React with 👍 / 👎.
| categories = [] if has_xy_values else [ | ||
| [str(level) for level in label] for label in plot.categories.flattened_labels | ||
| ] |
There was a problem hiding this comment.
When a category/value chart retains a worksheet formula but omits the optional category strCache or numCache, dereferencing plot.categories.flattened_labels can abort the entire slide inventory. The value-cache guard above now handles uncached series values, but category labels remain unguarded; report their cache as unavailable instead of preventing the rest of the deck from being analyzed.
Useful? React with 👍 / 👎.
| for shape in shapes: | ||
| if shape.has_text_frame: | ||
| yield shape.text_frame | ||
| if getattr(shape, "has_table", False): | ||
| for row in shape.table.rows: |
There was a problem hiding this comment.
When a deck's custom font appears only in a chart title, chart label, or SmartArt node, this walker emits no candidate for it because chart and diagram graphic frames have neither an ordinary text frame nor a table. The triage can consequently report that it inspected effective fonts while missing visible text that may substitute on another machine; traverse chart text properties and SmartArt text runs as well, or explicitly report them as unresolved.
Useful? React with 👍 / 👎.
| mixing. Note: ReportLab output is not tagged PDF/UA — when the user needs an accessible | ||
| (screen-reader-ready) PDF, the pdf Skill says to report that limitation honestly instead of | ||
| claiming accessibility. |
There was a problem hiding this comment.
When a user requests a screen-reader-ready PDF, the actual skills/pdf/SKILL.md contains no accessibility or tagging guard, despite this README claiming that the skill instructs the agent to report ReportLab's PDF/UA limitation. Because the runtime skill can therefore proceed with ordinary untagged ReportLab output without warning, add the limitation and refusal/fallback behavior to the PDF skill itself.
Useful? React with 👍 / 👎.
| files = sorted( | ||
| (path for path in src.rglob("*") if path.is_file() and path != content_types), | ||
| key=lambda path: path.relative_to(src).as_posix(), |
There was a problem hiding this comment.
When work/ already contains files from a previous document, extracting another DOCX there overwrites matching members but does not remove members absent from the new archive, and this rglob then repacks those stale files. The output can silently contain undeclared parts or confidential media from the earlier document; create a new empty temporary directory for every extraction or refuse to proceed when the destination is nonempty.
Useful? React with 👍 / 👎.
| def face_from_rpr(rpr, slot): | ||
| if rpr is None: | ||
| return None | ||
| rfonts = rpr.find(qn("w:rFonts")) | ||
| return None if rfonts is None else rfonts.get(qn("w:" + slot)) |
There was a problem hiding this comment.
When a run declares w:asciiTheme, w:hAnsiTheme, or w:eastAsiaTheme instead of a literal face, this helper treats the direct formatting as absent and can fall through to a style-level font that Word does not actually use. The glyph audit may therefore pass against the wrong cmap; resolve the corresponding theme face or fail closed whenever a direct theme attribute is present.
Useful? React with 👍 / 👎.
| with open("output.csv", "w", newline="", encoding="utf-8") as f: | ||
| writer = csv.writer(f) | ||
| rows = [["Region", "Units", "Note"], ["EU", 120, "=2+2"]] | ||
| writer.writerows([spreadsheet_csv_field(value) for value in row] for row in rows) |
There was a problem hiding this comment.
When the requested output is .tsv, following this writing route with only the filename changed still uses csv.writer's comma delimiter, producing comma-separated content under a TSV extension. Since the skill advertises TSV support and routes it here, derive the delimiter from the requested format or pass delimiter="\t" explicitly for TSV output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 943c278e7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| print(f"{ws.title} dims:", ws.dimensions) | ||
| formulas = [ | ||
| (c.coordinate, formula_text(c.value)) | ||
| for row in ws.iter_rows() for c in row if c.data_type == "f" |
There was a problem hiding this comment.
When an edited workbook retains a styled or previously cleared cell near the worksheet limits, this mandatory iter_rows() traverses the entire rectangle through max_row and max_column; for example, a sparse cell at XFD1048576 makes openpyxl attempt billions of cell visits and can exhaust memory before verification completes. Iterate the worksheet's populated cells or otherwise establish bounded real-data dimensions before collecting formulas.
Useful? React with 👍 / 👎.
| if cached_numeric_points(value_source) is None: | ||
| return {"name": series.name, "values": None, "cache_status": "unavailable"} | ||
| return {"name": series.name, "values": list(series.values)} |
There was a problem hiding this comment.
When a chart series title references a worksheet cell but omits the optional string cache, accessing series.name can dereference that missing cache and abort the entire deck inventory. Every return path in series_content() reads the name, including the branches intended to tolerate unavailable value caches, so resolve the series title defensively and report it as unavailable when its cache is absent.
Useful? React with 👍 / 👎.
| if number not in intentionally_raster_only_pages | ||
| and not text | ||
| and widget_count(r.pages[number - 1]) == 0 |
There was a problem hiding this comment.
When an otherwise blank page contains only a hidden, invisible, or no-view AcroForm widget, widget_count() is nonzero and this condition exempts the page from the extractable-text requirement. Such metadata widgets are not usable visible page content, so an accidentally blank generated page can pass the mandatory postcheck; count only widgets whose annotation flags and appearance make them visible.
Useful? React with 👍 / 👎.
| with ZipFile(input_path) as archive: | ||
| archive.extractall(src) |
There was a problem hiding this comment.
When Tier 2 is used on an untrusted or corrupted DOCX, this extractall() runs before any member-size, total-size, compression-ratio, or member-count checks, so a small archive bomb can exhaust disk space or CPU even though the separate review route contains a bounded health check. Run that bounded validation before extraction, or stream each member while enforcing equivalent limits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6a148b2b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| names = {info.filename for info in infos} | ||
| require(len(names) == len(infos), "duplicate archive member names are unsafe") | ||
| require("[Content_Types].xml" in names and "word/document.xml" in names, | ||
| "required DOCX package parts are missing") |
There was a problem hiding this comment.
When an input contains both word/document.xml and a distinct name such as ../word/document.xml, this set considers them unique and the required-member check passes, but ZipFile.extractall() later normalizes both names to the same destination and silently lets the latter overwrite the canonical document part. Reject non-canonical member paths and collisions after path normalization before extraction so Tier 2 edits cannot operate on a different part than the one validated.
Useful? React with 👍 / 👎.
| return [ | ||
| (int(point.get("idx")), float(value.text)) | ||
| for point in points | ||
| if point.get("idx") is not None and (value := point.find(qn("c:v"))) is not None |
There was a problem hiding this comment.
When a valid chart cache contains an Excel error value such as #N/A (commonly used to suppress a plotted point), float(value.text) raises ValueError and aborts the inventory before later charts or slides are analyzed. Parse each point defensively and preserve error/blank markers, rather than requiring every <c:v> payload in a numeric cache to be convertible to a Python float.
Useful? React with 👍 / 👎.
| def cached_category_labels(plot): | ||
| """Return flattened labels, or None when a formula has no category cache.""" | ||
| category_nodes = plot._element.xpath("./c:ser[1]/c:cat") |
There was a problem hiding this comment.
When two series in the same category plot reference different category ranges, this hard-coded ser[1] lookup and plot.categories access record only the first series' labels while the output lists values for every series. The resulting inventory can pair later series with the wrong categories and corrupt summaries or repurposed data; extract and associate the category cache per series, or explicitly reject plots whose series categories differ.
Useful? React with 👍 / 👎.
| def chart_axis_titles(chart): | ||
| """Return titles for axes the chart actually exposes (pie charts have none).""" | ||
| titles = {} | ||
| for label, attribute in (("category", "category_axis"), ("value", "value_axis")): |
There was a problem hiding this comment.
When a combination chart uses a secondary category or value axis, the singular chart.category_axis and chart.value_axis properties expose only one axis of each type, so this inventory silently omits visible secondary-axis titles. That can remove essential units or scale context from a deck summary; traverse all axis elements and report their IDs, positions, and titles rather than returning only the primary pair.
Useful? React with 👍 / 👎.
| FORMULA_PREFIXES = ("=", "+", "-", "@") | ||
|
|
||
| def spreadsheet_csv_field(value, *, mode="safe"): | ||
| if mode not in {"safe", "raw"}: | ||
| raise ValueError("mode must be 'safe' or 'raw'") | ||
| if mode == "safe" and isinstance(value, str) and value.startswith(FORMULA_PREFIXES): | ||
| return "'" + value |
There was a problem hiding this comment.
When an untrusted string begins with a tab or carriage return followed by =, +, -, or @, the advertised safe mode leaves it unchanged even though spreadsheet applications can ignore those leading control characters and interpret the remainder as a formula. Include formula-triggering control prefixes in the check, or normalize and inspect leading control characters before serializing fields that are claimed to be safe to open.
Useful? React with 👍 / 👎.
| def character_tags(character): | ||
| codepoint = ord(character) | ||
| if 0x3040 <= codepoint <= 0x30FF: | ||
| return ["Jpan"] | ||
| if 0x1100 <= codepoint <= 0x11FF or 0xAC00 <= codepoint <= 0xD7AF: | ||
| return ["Hang"] | ||
| if 0x2F00 <= codepoint <= 0x9FFF: | ||
| return ["Hans", "Hant", "Jpan", "Hang"] # locale disambiguates Han |
There was a problem hiding this comment.
When slide text contains CJK Compatibility Ideographs or supplementary Han characters such as those in Extension B (U+20000 and above), character_tags() returns no East Asian tag because the Han range stops at U+9FFF. required_slots() consequently treats those glyphs as Latin and reports candidates from the wrong theme slot, so the font triage can miss the exact rare characters most likely to render as tofu; extend the Han ranges to cover compatibility and supplementary ideographs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88a48952a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| labels = [node.text for node in root.iter(qn("a:t")) if node.text] | ||
| return {"name": shape.name, "status": "ok", "text": labels} | ||
|
|
||
| prs = Presentation("input.pptx") |
There was a problem hiding this comment.
When analyzing an untrusted or corrupted deck, Presentation("input.pptx") decompresses and parses package members before the bounded package check later in this document runs, so a ZIP bomb or oversized XML part can exhaust memory or CPU despite the advertised limits. Move the bounded validation ahead of this call and open the presentation only after it succeeds.
Useful? React with 👍 / 👎.
| chart_title = ( | ||
| chart.chart_title.text_frame.text | ||
| if chart.has_title else "" | ||
| ) |
There was a problem hiding this comment.
When a chart title uses the valid <c:tx><c:strRef> form instead of rich text, reading chart_title.text_frame invokes python-pptx's rich-text accessor rather than reading the cached referenced value, so the inventory can report an empty title and mutate the in-memory title choice. Parse literal/rich and strRef titles defensively, as chart_axis_text() already does for axis titles.
Useful? React with 👍 / 👎.
| for row in sheet.iter_rows(): | ||
| for cell in row: | ||
| if cell.data_type == "f": |
There was a problem hiding this comment.
When an input workbook contains a styled or previously cleared cell near the worksheet limits, this iter_rows() traverses the full rectangle through max_row and max_column before an insertion can be audited; a cell at XFD1048576 can therefore make the edit route exhaust memory or hang. Iterate the worksheet's sparse populated-cell store or establish bounded real-data dimensions before scanning formulas.
Useful? React with 👍 / 👎.
| is_blank = not ( | ||
| page.get_text().strip() or page.get_images() or image_blocks or drawings | ||
| or widgets or annotations or links | ||
| ) |
There was a problem hiding this comment.
When a page contains only a hidden, invisible, no-view, zero-area, or off-page widget/annotation, these truthy collections classify it as nonblank even though it has no visible or usable content. Fresh evidence beyond the earlier postcheck issue is that the postcheck now filters widget flags and rendered visibility, while this independent inspection route still counts every widget, annotation, and link; apply equivalent visibility filtering here.
Useful? React with 👍 / 👎.
| if node.tag == qn("w:t"): | ||
| pieces.append(node.text or "") | ||
| elif node.tag == qn("w:tab"): | ||
| pieces.append("<tab>") | ||
| elif node.tag in (qn("w:br"), qn("w:cr")): |
There was a problem hiding this comment.
When visible text in a table cell contains a <w:noBreakHyphen>, this cell-specific extractor handles only ordinary text, tabs, and breaks and silently drops the hyphen, potentially joining two words and corrupting extracted copy or summaries. Reuse run_text()/paragraph_text() for each cell paragraph or explicitly emit the nonbreaking-hyphen character here.
Useful? React with 👍 / 👎.
| clip = fitz.Rect( | ||
| page_box.x0 - 2000, page_box.y0 - 2000, | ||
| page_box.x1 + 2000, page_box.y1 + 2000, | ||
| ) |
There was a problem hiding this comment.
When content is positioned more than 2,000 points beyond a page edge—for example after a unit-conversion or absolute-positioning error—this finite clip excludes it from get_text("blocks"), so the mandatory overflow check can pass even though the content is completely off-page. Use PyMuPDF's unbounded/infinite clip or otherwise inspect all positioned text rather than imposing an arbitrary search margin.
Useful? React with 👍 / 👎.
| shapes = list(iter_shapes(slide.shapes)) # flattened; groups are common in template decks | ||
| text = [sh.text_frame.text for sh in shapes if sh.has_text_frame and sh.text_frame.text] |
There was a problem hiding this comment.
When visible copy such as a legal disclaimer, confidentiality label, or persistent footer lives on the slide layout or master, walking only slide.shapes omits it because inherited shapes are not members of that collection. The resulting extraction and repurposed markdown can silently lose visible text; inventory applicable layout/master shapes as well, while avoiding placeholders overridden by the slide and honoring master-visibility settings.
Useful? React with 👍 / 👎.
| chart.title = "Revenue by product" | ||
| chart.y_axis.title = "Revenue" | ||
| data = Reference(ws, min_col=5, min_row=1, max_row=last) # includes header for series name | ||
| cats = Reference(ws, min_col=2, min_row=2, max_row=last) |
There was a problem hiding this comment.
With the example data, Widget occurs in both the EU and US rows, but the chart binds directly to each transaction row while labeling itself “Revenue by product.” This produces two separate Widget bars rather than one product total and can misrepresent the requested comparison; aggregate revenue by product first or rename the chart to describe row-level sales.
Useful? React with 👍 / 👎.
💡 Codex Review Validate decks before the edit route parses them
When an edit is requested for an untrusted or corrupted deck, this call parses and decompresses the package before enforcing the archive/member/XML bounds defined in Ignore unpainted image resources in blank-page checks
When a page retains an unused image XObject in its resource dictionary after editing, Exclude hidden shapes from non-text inventories
When a chart, table, picture, or SmartArt object has Apply the chart-point budget to XY series
When a valid scatter or bubble deck contains many series, each X/Y cache may contribute up to Report broken picture relationships instead of aborting
When a malformed deck contains a picture whose embed relationship is missing, Treat Type3 glyph programs as embedded PDF content
When inspecting a PDF that uses a Type3 font, ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27b3edcc0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return cached | ||
|
|
||
| input_path = "input.xlsx" | ||
| formula_wb = openpyxl.load_workbook(input_path, read_only=True, data_only=False) |
There was a problem hiding this comment.
When the read route receives an untrusted or corrupted workbook, load_workbook() parses and decompresses package parts such as shared strings and styles before any size, compression-ratio, member-count, or XML-part limit is enforced; read_only=True does not protect this initialization path. The XLSX routes contain no bounded package validator analogous to the DOCX/PPTX preflights, so a small ZIP bomb can exhaust memory or CPU before profiling begins.
Useful? React with 👍 / 👎.
| """Scan an untrusted read-only stream without relying on its <dimension>.""" | ||
| worksheet.reset_dimensions() | ||
| min_row = min_column = max_row = max_column = None | ||
| for row in worksheet.iter_rows(): |
There was a problem hiding this comment.
When a worksheet has a styled or cleared cell at an extreme coordinate such as XFD1048576, this unbounded iter_rows() must synthesize the rectangular gaps before lines 91–94 can discard the empty cell. Thus the newly added value filter fixes header selection but not the resource exhaustion: profiling can still generate billions of empty cells and hang before discovering the logical extent; scan worksheet XML or another sparse representation instead.
Useful? React with 👍 / 👎.
| drawings = page.get_drawings() | ||
| widgets, annotations, links, interaction_visibility_unknown = viewable_interactives(page) | ||
| is_blank = not ( | ||
| page.get_text().strip() or visible_images or drawings |
There was a problem hiding this comment.
When an otherwise blank page contains only a vector path that is fully off-page, clipped out, or has no visible paint, page.get_drawings() still returns a truthy record and this predicate reports the page as nonblank. The new visibility filtering covers images and interactive objects but not drawings, so generated blank pages can evade inspection; require a visible page intersection/rendered contribution for each drawing before counting it.
Useful? React with 👍 / 👎.
| or 0xF900 <= codepoint <= 0xFAFF # CJK compatibility ideographs | ||
| or 0xFE30 <= codepoint <= 0xFE6F # CJK compatibility and small forms | ||
| or 0xFF00 <= codepoint <= 0xFFEF # fullwidth and halfwidth forms | ||
| or 0x20000 <= codepoint <= 0x3134F # CJK unified ideograph extensions |
There was a problem hiding this comment.
When DOCX text contains a Han character at U+31350 or later, this cutoff classifies it as hAnsi rather than eastAsia, even though the PPTX audit in this same plugin already covers the newer supplementary Han range through U+33479. The glyph audit therefore resolves and checks the wrong effective font and can miss tofu for these rare characters; include the newer Han extensions in the East Asian slot mapping.
Useful? React with 👍 / 👎.
| if shape.has_table: | ||
| for row_index, row in enumerate(shape.table.rows): | ||
| for column_index, cell in enumerate(row.cells): | ||
| yield f"{path}/table[{row_index},{column_index}]", cell.text_frame |
There was a problem hiding this comment.
When the requested text is in a merged PowerPoint table cell, row.cells exposes the merge-origin proxy at every covered grid position, so this loop adds the same underlying text frame to candidates multiple times and the uniqueness gate rejects an otherwise unambiguous edit. The analyzer and postcheck already distinguish cell.is_spanned; apply the same filter here so only the merge origin is treated as an editable target.
Useful? React with 👍 / 👎.
| else: | ||
| yield from iter_part_blocks(child, parent) |
There was a problem hiding this comment.
When a DOCX contains mc:AlternateContent, this unconditional recursion visits both its mc:Choice and mc:Fallback branches even though a conforming consumer renders only one. Documents carrying fallback paragraphs, runs, or tables can therefore have mutually exclusive content duplicated or combined in the extracted text and summaries; select a supported Choice branch, otherwise use the Fallback or report the content as unresolved.
Useful? React with 👍 / 👎.
| reference for reference in cell_formulas_before | ||
| if formula_may_intersect_rows(reference[1], reference[3], ws.title, 5) | ||
| ] | ||
| unaudited_references = intersecting_formulas + non_cell_references(wb) |
There was a problem hiding this comment.
When a workbook contains an internal hyperlink whose destination is at or below the inserted row, openpyxl does not rewrite the hyperlink location, but non_cell_references() omits cell hyperlinks entirely. The safety gate can therefore allow insert_rows(5) while leaving a link such as #'Data'!A10 pointing at the wrong record after that record moves to row 11; inventory internal hyperlink locations and require an explicit rewrite plan just like other non-cell dependencies.
Useful? React with 👍 / 👎.
|
|
||
| def table_content(table): | ||
| rows = [] | ||
| for row in table.rows: |
There was a problem hiding this comment.
When a Word table uses row-level or cell-level structured document tags, table.rows and row._tr.tc_lst expose only direct <w:tr> and <w:tc> children, so rows or cells beneath <w:sdtContent> are silently omitted even though the extractor advertises content-control-aware traversal. Walk the table's physical XML through content-control wrappers before building the row and cell inventory.
Useful? React with 👍 / 👎.
Address the latest review round: add bounded XLSX/OPC package preflight with compression, member, materialization and range-token budgets; route all existing-package loads through validated snapshots; harden DOCX CJK/locator, PDF Type3 and PPTX merged-slot guidance; cover CSV formula neutralization, missing dimension scanning and internal hyperlink rewrites with real OPC fixtures.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5cd131415
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }) | ||
| return rows | ||
|
|
||
| doc = Document("input.docx") |
There was a problem hiding this comment.
When this route receives an untrusted or corrupted DOCX, Document("input.docx") decompresses and parses package parts before any member-size, compression-ratio, or total-size bounds run. The bounded validator elsewhere in this plugin is used only by the raw-edit/review routes, so a small ZIP bomb can exhaust memory or CPU during an ordinary read or summary request; validate a private snapshot before passing that same handle to python-docx.
Useful? React with 👍 / 👎.
| from openpyxl.utils.cell import coordinate_to_tuple, range_boundaries | ||
|
|
||
| approved_feature_loss = False # set True only after showing the inventory to the user | ||
| wb, dropped_parts, stripped_extensions = load_with_round_trip_audit("input.xlsx") |
There was a problem hiding this comment.
When an input workbook contains rich-text cells, this call uses openpyxl's default rich_text=False, so loading and saving flattens their per-run formatting. The audit still reports no loss because the worksheet/shared-string parts remain present and no worksheet extension record disappears, allowing an unrelated edit to silently strip rich text throughout the workbook; load both audit and editable copies with rich_text=True or explicitly detect and gate this loss.
Useful? React with 👍 / 👎.
| drawings, visible_drawings, drawing_visibility_unknown = viewable_drawings(page) | ||
| widgets, annotations, links, interaction_visibility_unknown = viewable_interactives(page) | ||
| is_blank = not ( | ||
| page.get_text().strip() or visible_images or visible_drawings |
There was a problem hiding this comment.
When a page contains only text using invisible rendering mode, zero opacity, or clipping that prevents it from painting, page.get_text() can still return that text and this predicate classifies the visually blank page as nonblank. The image, drawing, and annotation branches already verify rendered visibility, so text needs an equivalent visibility/intersection check rather than treating every extractable string as visible content.
Useful? React with 👍 / 👎.
| "formula output is not configured to recalculate in spreadsheet viewers", | ||
| ) | ||
| for ws in wb.worksheets: | ||
| print(f"{ws.title} dims:", ws.dimensions) |
There was a problem hiding this comment.
When an edit leaves an unintended value, formula, or styled cell outside the requested output range, this mandatory postcheck only prints ws.dimensions and still exits successfully. That contradicts the following requirement to confirm that the used range matches expectations and lets stale or accidentally appended content pass verification; accept expected bounds per sheet and fail when the reported dimensions differ.
Useful? React with 👍 / 👎.
| require(info.file_size <= MAX_ENTRY, f"oversized part: {info.filename}") | ||
| ratio = info.file_size / max(info.compress_size, 1) | ||
| require(ratio <= MAX_COMPRESSION_RATIO, f"suspicious compression ratio: {info.filename}") | ||
| is_xml = info.filename.endswith((".xml", ".rels")) |
There was a problem hiding this comment.
When a DOCX contains an XML package part with a non-.xml suffix, an uppercase suffix, or an XML content-type override on another extension, this health check treats it as binary and never parses it. A malformed relationship target or custom XML part can therefore pass the advertised well-formedness check even though Word may repair or reject the document; parse [Content_Types].xml first and classify parts by their declared content type in addition to conventional suffixes.
Useful? React with 👍 / 👎.
|
本 PR 已迁移至 MiniMax-AI/MiniMax-Code-Plugins#6（head |
|
关闭说明：本 PR 已完成迁移，后续在 MiniMax-AI/MiniMax-Code-Plugins#6 继续 review。当前 head |
What changes
New Plugin
Hylouis233/document-skills- a Skill-only portable Agent Plugin bundling four originalworkbench Skills:
docx,pdf,pptx,xlsx.All guidance in this Plugin was written originally for this contribution. It contains no content
from Anthropic's document skills or any other proprietary skill pack - the docx/pdf/pptx/xlsx
names describe the formats, and the patterns are built around standard open-source Python
libraries (python-docx, python-pptx, openpyxl, pypdf, ReportLab, PyMuPDF) and public format
knowledge.
User value
Users ask agents for document deliverables and get silent corruption: hand-edited ZIP XML that
no viewer opens, pasted values where spreadsheet formulas belong, screenshot-style PDFs with no
extractable text, decks whose text overflows every slide. The four Skills route each task to the
right tool, enforce the container contracts, and - critically - require re-opening and verifying
the artifact before handing it back.
Example prompt:
Expected result: the agent loads the workbook with openpyxl, inspects sheets and headers, writes
real
SUMIF/COUNTIFformulas, adds a nativeBarChart, applies number formats, and reportsthe changed ranges plus the verification it ran.
Expected result: generated with ReportLab flowables, measured, and verified with pypdf to be
exactly 1 page with extractable text.
Skill inventory (4 Skills, 13 reference files, 1333 lines):
docx- create (python-docx outline + styles + TOC field), edit (two-tier: structural editsvs. safe OOXML surgery), read (pandoc / python-docx), review (symptom-driven repair table),
mandatory postcheck with optional
sofficePDF smoke test.pdf- one-tool-per-job routing: ReportLab for creation, PyMuPDF for extraction/inspection,pypdf for split/merge/rotate/watermark/encrypt/forms; text-first rule; page-geometry and
overflow checks in postcheck.
pptx- seven workhorse slide patterns, narrow in-place editing with asserted shape matching,real chart parts over chart pictures, text-fit rules, slide-count and render verification.
xlsx- formulas-are-formulas contract,data_onlycaveats, typed values with explicitnumber formats, native charts bound to ranges, structural-edit formula audit, CSV/TSV route
with a messy-data cleanup contract.
Plugin submission checklist
plugins/<github-owner>/<plugin-name>.plugin.jsonname matches the Plugin directory.README.mdincludes a real example prompt and expected result.LICENSEandplugin.jsondeclare an open-source license (Apache-2.0).(Python 3.9+ and the six libraries; optional LibreOffice/pandoc; Windows/macOS/Linux).
binaries are included.
TODOhas been replaced.Network and data behavior
No network access, no credentials, no bundled executables. The Skills instruct the agent to use
the user's own installed Python libraries and operate only on files the user points at; temporary
artifacts go to the system temp directory.
soffice/pandocrender checks are optional anddegrade gracefully when absent.
Evidence
The one failing test (
contributor can scaffold a hosted Skill plugin with one command,test/hosted-plugins.test.mjs) is a pre-existing failure on unmodifiedmain: the test assertsa POSIX path separator while
path.relativereturnsplugins\alice\hello-worldon Windows.Verified on a pristine shallow clone of this repository's
main- same single failure, allother 11 tests pass. No repository file outside
plugins/Hylouis233/document-skillsis touchedby this PR.
Manual evidence - the documented patterns were executed end to end on Windows (Python 3.13,
python-docx 1.2.0, python-pptx 1.0.2, openpyxl 3.1.5, pypdf 6.9.2, reportlab 4.4.10,
pymupdf 1.27.2, LibreOffice present):
Every Python snippet embedded in the Skills was also parsed with
ast.parse- zero syntaxerrors. Staged files verified UTF-8 without BOM, LF line endings.