Repository navigation
fix(skills): request markitdown extras in markdown-converter commands - #43
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. Reviewed September 29, 2026, 8:53 PM ET / September 30, 2026, 00:53 UTC (Revision 4). ClawSweeper reviewWhat this changesThe Markdown conversion skill now requests MarkItDown’s optional dependencies in its command examples and explains how to override incorrect text encoding detection. Merge readiness✅ Ready for maintainer review The reported PDF and Office conversion failure remains in current main, and this PR is a focused candidate fix. The earlier changelog concern does not apply to the current head: the entry was added in a repository-owner commit after the contributor removed their entry. Priority: P2 Review scores
Verification
How this fits togetherThe Markdown conversion skill gives agents commands for turning documents into Markdown. Its examples invoke MarkItDown through uvx, which selects the installed package dependencies before conversion runs. flowchart LR
A[Document or stdin] --> B[Skill command]
B --> C[uvx selects MarkItDown extras]
C --> D[MarkItDown converts input]
D --> E[Markdown output]
Before mergeNone. Agent review detailsSecurityNone. Review metricsNone. Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Technical reviewBest possible solution: Land the focused skill correction with its contributor credit, then resolve the linked failure report through the merged fix. Do we have a high-confidence way to reproduce the issue? Yes. Current main still uses the bare package command, and the linked report records a missing optional-dependency failure for PDF with a successful conversion when the all extra is requested. Is this the best way to solve the issue? Yes. Requesting the existing package extra at the uvx entrypoint addresses the documented commands without adding an installation step or a new option. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 5008579e73be. LabelsLabel changes: No label changes. Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles)
|
|
Fixed — the You were right and I was wrong on this: I read the general changelog rule earlier in The release-note material now lives in the PR body under a "Release note context" heading, per Written by Claude (Anthropic) at the reporter's request. |
Co-authored-by: pereponkin <pereponkin@gmail.com>
b2e82f2 to
dc7f358
Compare
The documented PDF and Office commands installed only MarkItDown's base package, so conversions failed with missing optional dependencies. Request
markitdown[all]in every invocation, including stdin examples, while retaining on-demand execution and quoting that works in POSIX shells, PowerShell, and Windows cmd.Also document the existing charset override for text files whose encoding is detected incorrectly. Refs #41. Thanks @pereponkin for the report, fix, and Windows verification.
Validation: skill metadata validation; synthetic PDF, DOCX, XLSX, PPTX and PDF-stdin conversions; base-only PDF failure reproduced before the fix; full repository CI; independent review with no actionable findings.