Feat/implement docx support task#330
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughThis PR introduces comprehensive Microsoft Word document (.docx) support to the codebase by implementing a complete DOCX processing module with parsing, reading, writing, and editing capabilities. It integrates DOCX handling into the file handler architecture, adds a new Changes
Sequence DiagramsequenceDiagram
actor Client
participant Server
participant FileHandler as DocxFileHandler
participant Parsers
participant ZipUtils as ZipArchive
participant Builders as Markdown Builder
rect rgba(100, 150, 200, 0.5)
Note over Client,Builders: DOCX Reading Flow
Client->>Server: read_file with .docx path
Server->>FileHandler: read(docxPath)
FileHandler->>ZipUtils: createZipFromBuffer(buffer)
FileHandler->>Parsers: parseDocxStructure(buffer)
Parsers->>ZipUtils: readZipFileText(document.xml)
Parsers->>ZipUtils: extractImagesFromZip()
Parsers->>Parsers: parseParagraphElement()<br/>parseTableElement()
Parsers-->>FileHandler: DocxStructure
FileHandler->>Builders: parseDocxToMarkdown(source)
Builders-->>FileHandler: markdown + metadata + images
FileHandler-->>Server: FileResult with DOCX metadata
Server-->>Client: markdown content + metadata
end
rect rgba(200, 150, 100, 0.5)
Note over Client,Builders: DOCX Writing/Editing Flow
Client->>Server: write_docx with operations
Server->>FileHandler: write(docxPath, operations)
alt Markdown Content
FileHandler->>Builders: createDocxFromMarkdown(markdown)
Builders-->>FileHandler: Buffer
else DOCX Operations
FileHandler->>FileHandler: editDocxWithOperations()
FileHandler->>Parsers: parseDocxStructure(buffer)
FileHandler->>FileHandler: applyOperationToStructure()
FileHandler->>Builders: buildDocxFromStructure()
Builders-->>FileHandler: Buffer
end
FileHandler-->>Server: void (write complete)
Server-->>Client: success confirmation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| title: "Write/Modify DOCX", | ||
| readOnlyHint: false, | ||
| destructiveHint: true, | ||
| openWorldHint: false, |
There was a problem hiding this comment.
Suggestion: The write_docx tool is annotated with openWorldHint: false even though insertImage operations in the underlying implementation can reach out to HTTP/HTTPS URLs, so clients are misled into thinking the tool is strictly local, which is a security-relevant metadata bug; the hint should reflect that the tool may access the network. [security]
Severity Level: Major ⚠️
- ⚠️ ListTools metadata misleads clients about network access.
- ❌ write_docx image insertion may fetch remote images.
- ⚠️ Client UI policies may allow unsafe operations.| openWorldHint: false, | |
| openWorldHint: true, |
Steps of Reproduction ✅
1. Start the MCP server with the PR changes applied (see src/server.ts).
- Inspect tool discovery by calling ListToolsRequestSchema handler which returns tool
metadata including the write_docx tool. The write_docx tool's annotations block is
present at src/server.ts:508-513 and shows openWorldHint: false.
2. Discover the tool in a client: call the MCP List Tools flow (ListToolsRequestSchema).
- Client receives the write_docx tool metadata from src/server.ts and will interpret
openWorldHint:false (no network access expected).
3. Invoke the tool via CallToolRequest with name "write_docx". The server routes this to
the handler at src/server.ts:1398 where the switch case calls
handlers.handleWriteDocx(args).
4. The handler path leads into the DOCX implementation that accepts insertImage operations
and intentionally does not validate HTTP(S) URLs. In src/tools/filesystem.ts (within the
writeDocx modification branch) the code only skips validation for paths starting with
'data:' or 'http' (so remote HTTP URLs are allowed to pass through), e.g. the insertImage
loop that checks:
if (o.type === 'insertImage' && o.imagePath) {
if (!o.imagePath.startsWith('data:') && !o.imagePath.startsWith('http')) {
o.imagePath = await validatePath(o.imagePath);
}
}
This means a client calling write_docx with an insertImage
imagePath="http://example.com/img.png" will cause the system to accept the remote URL
and the DOCX creation/update pipeline may fetch it.
Note: The existing annotations value openWorldHint:false appears to be an oversight
relative to the filesystem behavior above (the code intentionally treats http URLs as
external and doesn't validate them as local). The suggestion is not frivolous: it aligns
metadata with the actual runtime behavior.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/server.ts
**Line:** 512:512
**Comment:**
*Security: The write_docx tool is annotated with openWorldHint: false even though insertImage operations in the underlying implementation can reach out to HTTP/HTTPS URLs, so clients are misled into thinking the tool is strictly local, which is a security-relevant metadata bug; the hint should reflect that the tool may access the network.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| children: headerCells.map((cell) => { | ||
| const textRuns = parseInlineFormatting(cell.trim()); | ||
| textRuns.forEach(run => { | ||
| (run as any).bold = true; |
There was a problem hiding this comment.
Suggestion: Table header text is intended to be bolded, but the code assigns to a bold property on TextRun instances instead of using the docx API (e.g., the bold() method), so the formatting is likely ignored and headers won't actually appear bold in the generated DOCX. [logic error]
Severity Level: Major ⚠️
- ❌ Table headers not bold in generated DOCX.
- ⚠️ createDocxFromMarkdown used for DOCX generation (lines 28-145).
- ⚠️ Affects tables created via createTableFromMarkdown (lines 209-260).| (run as any).bold = true; | |
| if (typeof (run as any).bold === 'function') { | |
| (run as any).bold(); | |
| } |
Steps of Reproduction ✅
1. Import and call createDocxFromMarkdown in src/tools/docx/builders/markdown-builder.ts
(function at lines 28-145) with markdown that contains a table (header row + separator).
The table detection lives at lines ~49-79 in the same file.
2. Execution reaches createTableFromMarkdown(lines: string[]) at
src/tools/docx/builders/markdown-builder.ts (function at lines 209-260). The header cell
mapping runs at lines 220-234.
3. While building the header row, the code at lines 221-224 sets (run as any).bold = true
on TextRun instances returned by parseInlineFormatting (lines 262-307).
4. The produced Document is packed via Packer.toBuffer (line 140). Inspecting the
generated .docx shows header text not visually bold because assigning a plain property to
existing TextRun instances does not update the docx run formatting (the code never
reconstructs TextRun with bold set).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/tools/docx/builders/markdown-builder.ts
**Line:** 223:223
**Comment:**
*Logic Error: Table header text is intended to be bolded, but the code assigns to a `bold` property on `TextRun` instances instead of using the docx API (e.g., the `bold()` method), so the formatting is likely ignored and headers won't actually appear bold in the generated DOCX.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| public readonly context?: Record<string, unknown> | ||
| ) { | ||
| super(message); | ||
| this.name = 'DocxError'; |
There was a problem hiding this comment.
Suggestion: When extending Error in TypeScript/JavaScript, failing to reset the prototype can cause instanceof checks against the custom error to fail in some runtimes/targets, so error instanceof DocxError in your wrapper may not work reliably and wrapped errors could be mishandled. [possible bug]
Severity Level: Major ⚠️
- ❌ Error identity lost in withErrorContext (src/tools/docx/errors.ts:60).
- ⚠️ Docx operation error handling may misclassify errors.
- ⚠️ createDocxError callers may receive wrapped errors.| this.name = 'DocxError'; | |
| Object.setPrototypeOf(this, new.target.prototype); |
Steps of Reproduction ✅
1. Create an operation that throws a DocxError using the helper in this file: call
createDocxError(...) at src/tools/docx/errors.ts:83 (function createDocxError) to produce
a new DocxError instance.
2. Invoke the wrapper withErrorContext at src/tools/docx/errors.ts:60 by passing an
operation that immediately throws the DocxError from step 1. The try/catch in
withErrorContext is defined at src/tools/docx/errors.ts:65-78.
3. In the catch block (src/tools/docx/errors.ts:67-71) the code checks `if (error
instanceof DocxError)`; because the DocxError constructor does not call
Object.setPrototypeOf, some transpilation targets/runtimes may produce an instance for
which instanceof returns false.
4. As a result the wrapper proceeds to wrap the caught value into a new DocxError at
src/tools/docx/errors.ts:72-76 instead of re-throwing the original instance, losing
original identity/context and altering upstream error-handling behavior.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/tools/docx/errors.ts
**Line:** 16:16
**Comment:**
*Possible Bug: When extending `Error` in TypeScript/JavaScript, failing to reset the prototype can cause `instanceof` checks against the custom error to fail in some runtimes/targets, so `error instanceof DocxError` in your wrapper may not work reliably and wrapped errors could be mishandled.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| for (let i = 0; i < operations.length; i++) { | ||
| const op = operations[i]; | ||
| const validatedOp = DocxReplaceTextOperationSchema.parse(op); | ||
| const tempPath = `${docxPath}.tmp${i}`; |
There was a problem hiding this comment.
Suggestion: Using a deterministic temp file name based only on the original path and loop index can cause race conditions and corrupted results when multiple concurrent edits target the same DOCX file, since different calls will overwrite each other's temp files. [race condition]
Severity Level: Major ⚠️
- ❌ Concurrent replaceText edits may corrupt DOCX output.
- ⚠️ replaceText fast-path used by batch text updates.
- ⚠️ Automated editors running parallel tasks affected.| const tempPath = `${docxPath}.tmp${i}`; | |
| const tempPath = `${docxPath}.tmp-${process.pid}-${Date.now()}-${Math.random()}-${i}`; |
Steps of Reproduction ✅
1. Invoke editDocxWithOperations at src/tools/docx/operations/index.ts:14 with a list of
replaceText operations so the "fast XML" branch is taken (operations.length > 0 and
allTextReplacements true).
2. The function enters the loop at src/tools/docx/operations/index.ts:37 and creates
deterministic temp files named `${docxPath}.tmp0`, `${docxPath}.tmp1`, ... at line 40.
3. Start two concurrent calls to editDocxWithOperations in parallel (for the same
docxPath) from application code (both calling src/tools/docx/operations/index.ts:14).
Because temp names are identical, one call's fs.writeFile at line 42 can overwrite the
other's temp file before replaceTextInDocxXml at line 43 reads it.
4. The overlapping writes produce incorrect intermediate ZIPs being passed to
replaceTextInDocxXml (src/tools/docx/operations/index.ts:43), resulting in corrupted or
partially-applied replacements and an incorrect final Buffer returned.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/tools/docx/operations/index.ts
**Line:** 40:40
**Comment:**
*Race Condition: Using a deterministic temp file name based only on the original path and loop index can cause race conditions and corrupted results when multiple concurrent edits target the same DOCX file, since different calls will overwrite each other's temp files.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| const targetPath = rel.target.startsWith('word/') | ||
| ? rel.target | ||
| : `word/${rel.target.replace(/^\/?/, '')}`; |
There was a problem hiding this comment.
Suggestion: When resolving image relationships, absolute targets like "/word/media/image1.png" will end up with a duplicated "word/" prefix ("word/word/media/image1.png"), causing image loading to silently fail for such documents. [logic error]
Severity Level: Major ⚠️
- ❌ parseDocxToMarkdown image extraction fails.
- ⚠️ Images missing in markdown output.
- ⚠️ Downstream features lose embedded images.| const targetPath = rel.target.startsWith('word/') | |
| ? rel.target | |
| : `word/${rel.target.replace(/^\/?/, '')}`; | |
| const normalizedTarget = rel.target.replace(/^\/+/, ''); | |
| const targetPath = normalizedTarget.startsWith('word/') | |
| ? normalizedTarget | |
| : `word/${normalizedTarget}`; |
Steps of Reproduction ✅
1. Prepare a DOCX that references media with an absolute relationship Target (e.g.
"/word/media/image1.png"). See convert pipeline entry `parseDocxToMarkdown()` in
src/tools/docx/markdown.ts at 358-436 which calls buildImageResolver() (buildImageResolver
defined at 193-225).
2. Call parseDocxToMarkdown('/path/to/doc-with-absolute-rel.docx') (function at
src/tools/docx/markdown.ts:358). The function loads the DOCX and builds relMap via
extractRelationshipMap() at 176-191.
3. During markdown conversion, extractTextFromRun() (227-257) triggers resolveImage(relId)
which calls the resolver returned by buildImageResolver() (193-225). The resolver executes
the targetPath logic at lines 208-211.
4. If rel.target begins with a leading slash (e.g. "/word/media/image1.png"), current code
builds targetPath as `word/${rel.target.replace(/^\/?/, '')}` producing
"word/word/media/image1.png". readZipFileBuffer(zip, targetPath) (line 211) then fails to
find the file and image extraction silently returns ''.
Explanation: this is a concrete execution path: parseDocxToMarkdown -> buildImageResolver
-> resolver -> targetPath lines (208-211). The problem reproduces whenever a DOCX contains
relationship Targets that start with '/'.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/tools/docx/markdown.ts
**Line:** 208:210
**Comment:**
*Logic Error: When resolving image relationships, absolute targets like "/word/media/image1.png" will end up with a duplicated "word/" prefix ("word/word/media/image1.png"), causing image loading to silently fail for such documents.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| buffer: Buffer; | ||
| width?: number; | ||
| height?: number; | ||
| altText: string; |
There was a problem hiding this comment.
Suggestion: The prepared image representation requires altText while the insert-image operation makes altText optional, which forces callers to fabricate dummy values or use unsafe non-null assertions and can result in undefined being treated as a string downstream. Making altText optional in PreparedImage aligns the types and correctly models that alt text may be absent. [type error]
Severity Level: Major ⚠️
- ❌ DOCX insert-image operations may embed undefined alt text.
- ⚠️ Image-embedding XML serialization may omit alt attribute.
- ⚠️ Reader/writer image metadata inconsistent between steps.| altText: string; | |
| altText?: string; |
Steps of Reproduction ✅
1. Inspect type definitions in src/tools/docx/types.ts lines 122-128: PreparedImage
requires altText (line 122..128).
2. Inspect DocxInsertImageOperation in the same file at lines 152-158 where altText is
optional (line 152..158 shows altText?: string). This is a concrete mismatch inside the
same module.
3. Real-world trigger: build a docx image insertion flow that constructs a
DocxInsertImageOperation without altText (valid per the current type at lines 152-158),
and then pass that operation to the code path that converts image operations into
PreparedImage instances (the project's image-preparation / builder code reads the
operation and creates PreparedImage objects). At conversion time the code may assume
PreparedImage.altText is always a string (because the type requires it), so it either:
- uses a non-null assertion (causing undefined to be passed forward), or
- fabricates a dummy string, or
- crashes/behaves incorrectly when downstream string APIs receive undefined.
Note: the conversion function is outside this types file; the mismatch is provable by
static analysis of lines 122-128 vs 152-158 in this file.
4. Observable failure modes: downstream image-embedding logic (image sizing/alt-text
serialization) will receive undefined for alt text, potentially causing incorrect XML
attributes or runtime errors in code that expects a string.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/tools/docx/types.ts
**Line:** 126:126
**Comment:**
*Type Error: The prepared image representation requires `altText` while the insert-image operation makes `altText` optional, which forces callers to fabricate dummy values or use unsafe non-null assertions and can result in undefined being treated as a string downstream. Making `altText` optional in `PreparedImage` aligns the types and correctly models that alt text may be absent.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| export interface DocxInsertTableOperation { | ||
| type: 'insertTable'; | ||
| markdownTable?: string; | ||
| rows?: string[][]; | ||
| } |
There was a problem hiding this comment.
Suggestion: The insert-table operation type allows both markdownTable and rows to be omitted, so code that assumes at least one data source will exist can receive an operation with no table data and fail at runtime; defining DocxInsertTableOperation as a union that requires either markdownTable or rows prevents constructing such invalid operations. [logic error]
Severity Level: Major ⚠️
- ❌ Insert-table operations may cause runtime exceptions.
- ⚠️ DOCX update feature can produce empty/invalid tables.
- ⚠️ User-provided table inputs may be silently ignored.| export interface DocxInsertTableOperation { | |
| type: 'insertTable'; | |
| markdownTable?: string; | |
| rows?: string[][]; | |
| } | |
| export type DocxInsertTableOperation = | |
| | { | |
| type: 'insertTable'; | |
| markdownTable: string; | |
| rows?: string[][]; | |
| } | |
| | { | |
| type: 'insertTable'; | |
| rows: string[][]; | |
| markdownTable?: string; | |
| }; |
Steps of Reproduction ✅
1. Examine table-op type in src/tools/docx/types.ts lines 146-150:
DocxInsertTableOperation currently allows both markdownTable and rows to be absent.
2. Create an operation object at runtime: const op = { type: 'insertTable' } — this is
permitted by the current interface (no required payload).
3. Pass op into the DOCX update pipeline that renders insertTable operations (the code
that applies DocxOperation lists to a document). That renderer will expect either
markdownTable or rows to exist and attempt to iterate or parse it.
4. Runtime result: renderer tries to read rows or parse markdownTable and either throws a
TypeError (e.g., cannot iterate undefined) or produces an empty/invalid table in the
output DOCX.
Note: types.ts shows the permissive shape (lines 146-150). The concrete renderer code is
outside this file, but this invalid operation shape is a realistic producer-consumer
mismatch when building operations from user input or client code.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/tools/docx/types.ts
**Line:** 146:150
**Comment:**
*Logic Error: The insert-table operation type allows both `markdownTable` and `rows` to be omitted, so code that assumes at least one data source will exist can receive an operation with no table data and fail at runtime; defining `DocxInsertTableOperation` as a union that requires either `markdownTable` or `rows` prevents constructing such invalid operations.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
|
||
| return { | ||
| content, | ||
| mimeType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', |
There was a problem hiding this comment.
Suggestion: The read method returns markdown text as content but sets the MIME type to a DOCX binary MIME, which can cause downstream consumers to treat the response as a binary Word document instead of markdown text, leading to misrendering or incorrect handling of the data. [logic error]
Severity Level: Major ⚠️
- ❌ DocxFileHandler.read returns wrong MIME type.
- ⚠️ File preview/renderers mis-handle returned content.
- ⚠️ Saving response as .docx produces invalid files.| mimeType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', | |
| mimeType: 'text/markdown', |
Steps of Reproduction ✅
1. Create a short script that imports the class declared at `src/utils/files/docx.ts:22`
(class DocxFileHandler) and calls its read method defined at `src/utils/files/docx.ts:36`:
- Example code:
const h = new DocxFileHandler();
const res = await h.read('/tmp/sample.docx');
2. When read() executes it calls `parseDocxToMarkdown()` at `src/utils/files/docx.ts:39`,
which delegates to `src/tools/docx/markdown.ts:358` (function parseDocxToMarkdown) and
returns a markdown string in `res.content`.
3. Inspect the returned object: `res.content` contains markdown (see parseDocxToMarkdown
result) but `res.mimeType` is set to
`'application/vnd.openxmlformats-officedocument.wordprocessingml.document'` as returned at
`src/utils/files/docx.ts:71..74`.
4. Reproduce the consumer-visible mismatch concretely: write `res.content` to a file using
a small script that chooses extension based on `res.mimeType` (e.g., writes to
/tmp/output.docx because mimeType indicates DOCX). Attempt to open `/tmp/output.docx` in
Word — the file will contain markdown text, not a valid DOCX archive, demonstrating the
incorrect MIME labeling and resulting misbehavior.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/utils/files/docx.ts
**Line:** 73:73
**Comment:**
*Logic Error: The read method returns markdown text as content but sets the MIME type to a DOCX binary MIME, which can cause downstream consumers to treat the response as a binary Word document instead of markdown text, leading to misrendering or incorrect handling of the data.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| '.gif': 'image/gif', | ||
| '.bmp': 'image/bmp', | ||
| '.webp': 'image/webp', | ||
| }; |
There was a problem hiding this comment.
Suggestion: SVG image files are accepted as valid in the validator but are not mapped to the correct MIME type in the image preparation logic, so they are incorrectly labeled as PNG when embedded, which can break downstream consumers that rely on the MIME type. [logic error]
Severity Level: Major ⚠️
- ⚠️ Prepared image MIME may mismatch content.
- ⚠️ Downstream consumers relying on mimeType affected.
- ⚠️ validateImageFile permits SVGs but mapping inconsistent.| }; | |
| '.svg': 'image/svg+xml', |
Steps of Reproduction ✅
1. Open src/tools/docx/utils.ts and locate validateImageFile at lines 96-104: it lists
validExtensions and includes '.svg' (line 98).
2. Locate prepareImageForDocx at lines 269-346; after reading a local image it uses the
MIME mapping at lines 321-331.
3. Provide a local SVG path (e.g., './assets/vector.svg') to prepareImageForDocx from a
test or REPL. validateImageFile will accept '.svg' (line ~98) and fs.readFile will
succeed.
4. After reading, execution reaches the mimeTypes mapping (lines 321-331) which lacks
'.svg', so mimeType becomes the default 'image/png', producing an incorrect MIME type for
the SVG file.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/tools/docx/utils.ts
**Line:** 330:330
**Comment:**
*Logic Error: SVG image files are accepted as valid in the validator but are not mapped to the correct MIME type in the image preparation logic, so they are incorrectly labeled as PNG when embedded, which can break downstream consumers that rely on the MIME type.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| // Summary | ||
| const passed = results.filter(r => r === true).length; | ||
| const total = results.length; | ||
|
|
There was a problem hiding this comment.
Suggestion: Skipped tests (when the sample DOCX file is missing and the test functions return without a boolean) are still pushed into the results array and counted in total, so the summary and exit code will treat skipped tests as failed, causing the script to exit with status 1 even when all executed tests passed. The fix is to exclude undefined (skipped) entries from the results used for the summary. [logic error]
Severity Level: Major ⚠️
- ❌ Test runner reports false failures and exits non-zero.
- ⚠️ Local developer test runs show misleading summary.
- ⚠️ CI jobs running this script may fail unexpectedly.| // Summary | |
| const passed = results.filter(r => r === true).length; | |
| const total = results.length; | |
| // Summary (ignore skipped tests that returned undefined) | |
| const effectiveResults = results.filter(r => r !== undefined); | |
| const passed = effectiveResults.filter(r => r === true).length; | |
| const total = effectiveResults.length; |
Steps of Reproduction ✅
1. Run the test script: execute `node test/test-docx-reading.js`. The file calls
`runAllTests()` (see `test/test-docx-reading.js:236`), which in turn executes the test
functions.
2. `runAllTests()` pushes each test result into `results` at
`test/test-docx-reading.js:215-219` (lines with `results.push(await
testBasicDocxReading());` ... `results.push(await testMetadataExtraction());`).
3. If the sample DOCX file is missing, `testBasicDocxReading()` returns early without a
boolean (`return;`) inside its inner catch block at `test/test-docx-reading.js:41-46` (the
`await fs.access(samplePath)` catch). The function therefore yields `undefined`.
4. That `undefined` is appended to `results`. The summary code at
`test/test-docx-reading.js:222-223` computes `passed = results.filter(r => r ===
true).length` and `total = results.length`, so skipped (`undefined`) entries increase
`total` and cause `passed !== total`.
5. The script then calls `process.exit(passed === total ? 0 : 1)` at
`test/test-docx-reading.js:234`, causing the process to exit with status 1 even if all
executed (non-skipped) tests returned true. Observed effect: a run with skipped tests
reports failures and returns non-zero exit code.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** test/test-docx-reading.js
**Line:** 220:223
**Comment:**
*Logic Error: Skipped tests (when the sample DOCX file is missing and the test functions return without a boolean) are still pushed into the `results` array and counted in `total`, so the summary and exit code will treat skipped tests as failed, causing the script to exit with status 1 even when all executed tests passed. The fix is to exclude `undefined` (skipped) entries from the results used for the summary.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/filesystem.ts (1)
780-857:⚠️ Potential issue | 🟡 MinorDOCX metadata is not surfaced in
getFileInfo.The function handles metadata for PDF (line 843), Excel (line 832), image (line 838), and binary (line 851) file types, but there's no corresponding block for DOCX. This means DOCX-specific metadata (author, title, sections, images, etc.) won't appear in
getFileInforesults even though it's defined inFileMetadata.Proposed addition after the PDF block (after line 848)
// For PDF files if (fileInfo.metadata.isPdf) { info.isPdf = true; info.totalPages = fileInfo.metadata.totalPages; if (fileInfo.metadata.title) info.title = fileInfo.metadata.title; if (fileInfo.metadata.author) info.author = fileInfo.metadata.author; } + + // For DOCX files + if (fileInfo.metadata.isDocx) { + info.isDocx = true; + if (fileInfo.metadata.title) info.title = fileInfo.metadata.title; + if (fileInfo.metadata.author) info.author = fileInfo.metadata.author; + if (fileInfo.metadata.subject) info.subject = fileInfo.metadata.subject; + if (fileInfo.metadata.sectionCount !== undefined) info.sectionCount = fileInfo.metadata.sectionCount; + if (fileInfo.metadata.imageCount !== undefined) info.imageCount = fileInfo.metadata.imageCount; + }
🤖 Fix all issues with AI agents
In `@src/tools/docx/builders/markdown-builder.ts`:
- Around line 220-224: The header mapping mutates constructed TextRun objects
((run as any).bold = true) which does nothing; instead update the code to create
bold runs at construction time—either call the TextRun fluent API (e.g., use the
run.bold() chain) when building runs inside the header mapping or refactor
parseInlineFormatting to accept an optional bold flag and have it construct and
return runs with bold set. Update the headerCells.map usage to pass that flag
into parseInlineFormatting or to apply the fluent .bold() on each returned run
immediately when creating the runs so header text is actually bold in the
generated docx.
In `@src/tools/docx/markdown.ts`:
- Around line 88-191: Remove the duplicated helper implementations and import
the canonical implementations from the existing modules: replace local isUrl
with the export from src/tools/docx/utils.ts; replace readZipFileText and
readZipFileBuffer with the exports from parsers/zip-reader.ts; replace
getMimeTypeForTarget with the export from parsers/image-extractor.ts; replace
getElementChildren, getAttributeValue, getHeadingLevelFromParagraph and
extractRelationshipMap with the exports from parsers/xml-parser.ts — update the
top-level imports and delete the local function definitions (isUrl,
readZipFileText, readZipFileBuffer, getMimeTypeForTarget, getElementChildren,
getAttributeValue, getHeadingLevelFromParagraph, extractRelationshipMap) so this
module reuses the single source of truth.
- Around line 94-102: The loadDocxToBuffer function should validate HTTP
responses when source is a URL: after fetching in loadDocxToBuffer (and using
isUrl to detect URLs), check response.ok and if false throw or return an error
that includes response.status and response.statusText (and optionally the URL)
before calling response.arrayBuffer(), so non-2xx responses (404/500) are
surfaced instead of silently returning an error page body.
- Around line 279-302: convertTableToMarkdown currently uses recursive
getElementsByTagName('w:tr'|'w:tc'|'w:p') which pulls in nested table
rows/cells/paras; replace those calls with a non‑recursive child iteration (e.g.
use a getElementChildren helper or loop over element.childNodes and filter by
node.nodeName === 'w:tr' / 'w:tc' / 'w:p') so only direct children are
processed; update the loops in convertTableToMarkdown that build rowNodes, cells
and paragraphs to use this child-filtering approach, keeping use of
extractParagraphText and escapeTableCell unchanged.
In `@src/tools/docx/operations/index.ts`:
- Around line 34-52: The loop writes temporary files and leaks them on errors
and is wasteful; update the logic so replaceTextInDocxXml is called with an
in-memory Buffer instead of writing temp files (either add a new API like
replaceTextInDocxXmlBuffer(buffer, search, replace) or overload
replaceTextInDocxXml to accept a Buffer), then call that with modifiedBuffer in
the for-loop (using DocxReplaceTextOperationSchema.parse(op) to get validatedOp)
and remove tempPath writes/unlinks; if you can't change replaceTextInDocxXml
immediately, wrap the per-iteration work (writeFile, call replaceTextInDocxXml,
unlink) in a try/finally to ensure tempPath is always unlinked even when
replaceTextInDocxXml throws, referencing modifiedBuffer, tempPath,
replaceTextInDocxXml and DocxReplaceTextOperationSchema to locate the code to
change.
In `@src/tools/docx/operations/operation-handlers.ts`:
- Around line 66-74: Merging appendStructure.relationships directly can
overwrite existing keys; instead generate new unique relIds for each entry from
appendStructure.relationships, insert them into structure.relationships and
structure.images using the new keys, and update any references in appended
elements to point to the new relIds before pushing them into structure.elements.
Concretely, inside the merge logic that iterates appendStructure.relationships
and appendStructure.elements, create a function to produce a newRelId (e.g.,
based on existing keys in structure.relationships), map oldRelId -> newRelId,
set structure.relationships.set(newRelId, rel) and for any matching entry in
appendStructure.images move imgBuffer to structure.images.set(newRelId,
imgBuffer), then walk each element in appendStructure.elements and replace
references to the oldRelId (attributes like rId, r:embed, relationshipId, href
targets or similar) with newRelId before pushing elements into
structure.elements so no collisions occur.
In `@src/tools/docx/operations/xml-replacer.ts`:
- Around line 44-48: The current naive replacement uses documentXml.replace(...)
to produce modifiedXml and will miss targets split across multiple <w:r>/<w:t>
runs (variables involved: documentXml, modifiedXml, escapedSearch,
escapedReplace); change the implementation to either (a) pre-process the XML to
merge adjacent run text nodes into contiguous searchable text per paragraph/run
group and perform replacements on that merged text, then re-split or reassign
the replaced text back into the original <w:t> elements, or (b) implement a
run-aware search that walks <w:t> elements, concatenates their text to match
escapedSearch across run boundaries, performs the substitution, and writes the
result back across the same runs; alternatively, if you opt not to implement
merging, add a clear comment and documentation near xml-replacer.ts explaining
this limitation and that documentXml.replace cannot match text spanning multiple
runs (mention escapedSearch/escapedReplace so reviewers can find the relevant
variables).
- Around line 34-48: The code uses escapedSearch in new RegExp(...) but
escapeXml doesn't escape regex metacharacters, causing incorrect matches and
ReDoS risk; fix by treating the search as a literal: either escape regex
metacharacters after escapeXml (add an escapeRegex helper and call it on
escapedSearch before building the RegExp) or avoid RegExp entirely and use a
literal replace (String.prototype.replaceAll or split/join) when creating
modifiedXml from documentXml; update references to
escapedSearch/escapedReplace/modifiedXml accordingly.
In `@src/tools/docx/parsers/paragraph-parser.ts`:
- Around line 83-94: The code forces every image to 600×400 and swallows errors;
update the w:drawing / w:pict branch (where resolveImageRelId(child), images,
and ImageRun are used) to first try to read the drawing extent (search the child
for the a:ext cx/cy attributes) and convert those EMU values to pixels (use a
named constant like EMU_PER_PIXEL) to compute width/height; if extent is
missing, fall back to a configurable default or the image's intrinsic
dimensions. Replace the empty catch with proper logging (e.g., logger.warn or
console.warn) that includes the relId and error details so failures are visible,
and only skip the image after logging.
In `@src/tools/docx/parsers/table-parser.ts`:
- Around line 25-33: The parser is using getElementsByTagName which recurses
into nested tables and pulls rows/cells/paragraphs from inner tables; update the
logic in the table parsing flow to iterate only direct children instead of using
getElementsByTagName: in the block that builds rows from table (where rowNodes =
table.getElementsByTagName('w:tr')), replace that recursive lookup with a
direct-child iteration (e.g., an element children filter) so only immediate
'w:tr' children are processed; apply the same change inside parseTableRow
(replace getElementsByTagName('w:tc') with direct-child iteration for immediate
'w:tc' cells) and inside parseTableCell (replace getElementsByTagName('w:p')
with direct-child iteration for immediate 'w:p' paragraphs) so nested w:tbl
content is not mixed into parent table parsing.
In `@src/tools/docx/types.ts`:
- Around line 47-92: The four interfaces DocxMetadata, DocxImage, DocxSection,
and DocxParseResult are duplicated; remove the duplicate declarations from
markdown.ts and have markdown.ts import those types from types.ts instead,
updating any local references to use the imported types; ensure index.ts
continues to re-export the canonical types (from types.ts) so public API remains
the same and adjust any internal imports that were pointing to ./types.js to
reference the single types.ts definitions.
In `@src/tools/docx/utils.ts`:
- Around line 250-258: PreparedImage is duplicated in utils.ts and types.ts;
remove the duplicate declaration from utils.ts and instead import {
PreparedImage } from the existing src/tools/docx/types.ts where needed (e.g., in
functions like any helpers in utils that currently reference PreparedImage).
Update utils.ts exports so it no longer re-exports PreparedImage (ensure
index.ts and operations.ts still re-export from types.ts), and run a quick
compile to fix any missing import sites and adjust import paths to reference the
single source of truth PreparedImage in types.ts.
In `@src/tools/filesystem.ts`:
- Around line 961-974: The loop handling insertImage operations silently
swallows validatePath errors and keeps unvalidated imagePath; instead let
validation failures surface so the operation fails: remove or rethrow inside the
try/catch around validatePath for the insertImage branch (referencing
validatePath, o.imagePath, and the insertImage operation) so errors propagate
like writePdf's sourcePdfPath validation, and tighten the URL check from
startsWith('http') to explicit startsWith('http://') || startsWith('https://')
to avoid false positives.
In `@test/test-docx-reading.js`:
- Around line 39-47: When the sample DOCX is missing, each test
(testBasicDocxReading, testFormattingPreservation, testMetadataExtraction)
should explicitly return a distinct sentinel (e.g., the string 'skipped' or a
SKIPPED Symbol) instead of undefined; update the early-exit blocks that
currently log "Sample DOCX file not found" to return that sentinel. Then update
runAllTests to treat results strictly (count passes where r === true, failures
where r === false, and skipped where r === 'skipped' or the SKIPPED Symbol) and
report/exit accordingly so skipped tests are not treated as failures. Ensure all
three test functions and runAllTests reference the same sentinel constant
(SKIPPED) to keep behavior consistent.
🟡 Minor comments (10)
src/tools/filesystem.ts-986-993 (1)
986-993:⚠️ Potential issue | 🟡 MinorRemove unused
outputPathoption or document its purpose.
editDocxWithOperationsacceptsoutputPathin its options but never uses it—onlybaseDiris referenced. The function returns aBuffer, and the caller correctly writes it to the target path. However, the unusedoutputPathoption inDocxEditOptionscreates a misleading API contract.Either remove the option or clarify its intended purpose if it's meant for future use.
test/test-docx-reading.js-125-138 (1)
125-138:⚠️ Potential issue | 🟡 MinorTests 2 and 4 have no assertions — they always pass.
testFormattingPreservation(and similarlytestMetadataExtraction) only logs whether formatting markers are detected but never asserts or returnsfalseon unexpected results. These are diagnostic scripts, not tests. If the sample DOCX contains bold text and parsing breaks, this test would still report✓.Consider adding actual assertions, e.g.:
const hasBold = result.markdown.includes('**'); + if (!hasBold && !hasItalic && !hasHeadings && !hasLists) { + log(colors.red, '✗ No formatting detected — expected at least one marker'); + return false; + }src/handlers/filesystem-handlers.ts-213-214 (1)
213-214:⚠️ Potential issue | 🟡 MinorDOCX files in
readMultipleFileslack image embedding, creating inconsistency with single-file reads.The summary (line 213-214) correctly labels DOCX files, but the content loop (lines 229-259) has no DOCX-specific handling. DOCX results fall through to the generic
elsetext branch (line 252), dropping embedded images. Meanwhile,handleReadFileextracts and includes these images (lines 130-138).The issue stems from
readMultipleFilesonly creating apayloadfor PDF files (line 564); DOCX files getpayload: undefineddespite images being available in metadata. To maintain parity, add a DOCX branch to extract images:Suggested fix
} else if (result.isImage && result.mimeType) { // For image files, add an image content item contentItems.push({ type: "image", data: result.content, mimeType: result.mimeType }); + } else if (result.isDocx && result.payload?.images?.length) { + for (const image of result.payload.images) { + contentItems.push({ type: "image", data: image.data, mimeType: image.mimeType }); + } + contentItems.push({ type: "text", text: `\n--- ${result.path} contents: ---\n${result.content}` }); } else { // For text files, add a text summary(Note: This assumes
payload.imagesis populated for DOCX, which may require updating line 564 to include DOCX image data in the payload.)Also applies to: 229-259
src/tools/docx/parsers/paragraph-parser.ts-79-82 (1)
79-82:⚠️ Potential issue | 🟡 MinorRemove redundant
text: '\n'when usingbreakproperty.
new TextRun({ text: '\n', break: 1 })includes unnecessarytext: '\n'. In docx v8.5.0, thebreakproperty itself inserts the line break element; thetext: '\n'won't render as a line break in the document. Usenew TextRun({ break: 1 })instead.src/tools/docx/markdown.ts-457-503 (1)
457-503:⚠️ Potential issue | 🟡 MinorDouble ZIP parsing and ReDoS-susceptible regex in metadata extraction.
Two issues:
Unnecessary second ZIP library: The buffer is already loaded with
PizZipat Line 390, butextractMetadataloads it again withJSZip(Line 460). Reuse the existingPizZipzip instance instead.Static analysis flagged ReDoS on the dynamically constructed regexes (Lines 468, 474, 480). While the
tagvalues are hardcoded strings from Lines 492–499 and the risk is low, the DOMParser (already imported at Line 13) should be used for parsing core properties XML instead of fragile regex.src/server.ts-447-514 (1)
447-514:⚠️ Potential issue | 🟡 Minor
write_docxdefault-overwrites the original, butwrite_pdfforbids it — inconsistent UX.The
write_docxdescription (Lines 460-463) encourages overwriting the original file by default whenoutputPathis omitted, whilewrite_pdf(Lines 393, 403) explicitly states "NEVER overwrite the original file" and requires a new filename inoutputPath.This inconsistency could confuse LLM clients that learn patterns from one tool and apply them to the other. Consider aligning the default behavior, or at minimum making the distinction very prominent in the description.
src/tools/docx/markdown.ts-524-526 (1)
524-526:⚠️ Potential issue | 🟡 MinorNo-op regex: list formatting replacement is identity.
markdown = markdown.replace(/\n([*-]\s)/g, '\n$1');This replaces
\nfollowed by*or-with… the exact same thing. If the intent was to ensure a blank line before lists, the replacement should be'\n\n$1'or similar.src/tools/docx/utils.ts-96-104 (1)
96-104:⚠️ Potential issue | 🟡 MinorSVG accepted by
validateImageFilebut missing from MIME type map inprepareImageForDocx.
validateImageFile(line 98) includes.svgin itsvalidExtensions, so SVG files pass validation. However, the MIME type map insideprepareImageForDocx(lines 323-330) doesn't include.svg, causing it to default toimage/png— which is incorrect for SVG files and may produce corrupt image output.Additionally, note that the
docxlibrary'sImageRunmay not support SVG at all. Either add.svgto the MIME map or remove it from the valid extensions to keep them consistent.🐛 Option A: Add SVG to the MIME map
'.bmp': 'image/bmp', '.webp': 'image/webp', + '.svg': 'image/svg+xml', };🐛 Option B: Remove SVG from valid extensions if unsupported by docx library
- const validExtensions = ['.png', '.jpg', '.jpeg', '.gif', '.bmp', '.webp', '.svg']; + const validExtensions = ['.png', '.jpg', '.jpeg', '.gif', '.bmp', '.webp'];Also applies to: 321-331
src/tools/docx/builders/markdown-builder.ts-262-307 (1)
262-307:⚠️ Potential issue | 🟡 MinorInline formatting parser has unclosed-marker fallback gap.
When
**is found at line 269 but no closing**exists, the code falls through without consuming the**characters —currentTextdoesn't accumulate them, andiisn't advanced past them. This causes the first*to be re-evaluated at line 284 as a potential italic marker, producing garbled output for malformed input like**unclosed.🛡️ Suggested fix: fall through to accumulate the literal characters
if (text.substring(i, i + 2) === '**') { if (currentText) { runs.push(new TextRun({ text: currentText })); currentText = ''; } const closeIndex = text.indexOf('**', i + 2); if (closeIndex !== -1) { const boldText = text.substring(i + 2, closeIndex); runs.push(new TextRun({ text: boldText, bold: true })); i = closeIndex + 2; continue; + } else { + // No closing **, treat as literal text + currentText += '**'; + i += 2; + continue; } } // Check for *italic* if (text[i] === '*' && text[i + 1] !== '*') { if (currentText) { runs.push(new TextRun({ text: currentText })); currentText = ''; } const closeIndex = text.indexOf('*', i + 1); if (closeIndex !== -1) { const italicText = text.substring(i + 1, closeIndex); runs.push(new TextRun({ text: italicText, italics: true })); i = closeIndex + 1; continue; + } else { + // No closing *, treat as literal text + currentText += '*'; + i += 1; + continue; } }src/utils/files/docx.ts-243-250 (1)
243-250:⚠️ Potential issue | 🟡 MinorImport
DocxErrorCodeand useDocxErrorCode.GET_INFO_FAILEDinstead of the string literal.Line 247 uses the string literal
'GET_INFO_FAILED'instead ofDocxErrorCode.GET_INFO_FAILED. This is inconsistent with the rest of the codebase, where all other files (src/tools/docx/structure.ts, utils.ts, operations/*) properly import and use the enum. Add the import and use the enum constant to maintain type safety and consistency.
🧹 Nitpick comments (17)
src/utils/files/base.ts (1)
133-154: Consider the memory impact of embedding full image data in metadata.The
imagesfield stores base64 image data (data: string) directly in the metadata object. For DOCX files with many or large embedded images, this could significantly increase memory usage when the metadata is serialized or passed around. Consider whether a summary (e.g., justid,mimeType,originalSize) would suffice for metadata, with full image data accessible on-demand via a separate API.Also,
authorandtitle(already present from PDF metadata at lines 128-129) appear to be reused for DOCX without an explicit comment — this is fine if intentional, but worth a note for clarity.src/tools/docx/operations/operation-handlers.ts (1)
16-22: UnusedcreateRequire— therequirebinding on line 18 is never used.The
docxlibrary is imported via ESM on line 20. The CJSrequirefromcreateRequireis not referenced afterward. Remove the dead code.Proposed fix
-import { createRequire } from 'module'; - -const require = createRequire(import.meta.url); -// `@ts-ignore` -import * as docx from 'docx'; +// `@ts-ignore` +import * as docx from 'docx';src/tools/docx/parsers/zip-reader.ts (1)
16-18: Consider wrappingnew PizZip(buffer)with error context.If the buffer isn't a valid ZIP (e.g., corrupted DOCX),
PizZipwill throw a raw error without DOCX-specific context. Since theDocxError/withErrorContextpattern is established in this codebase, wrapping here would improve error diagnostics.Proposed improvement
+import { DocxError, DocxErrorCode } from '../errors.js'; + export function createZipFromBuffer(buffer: Buffer): ZipArchive { - return new PizZip(buffer); + try { + return new PizZip(buffer); + } catch (error) { + throw new DocxError( + `Failed to parse DOCX as ZIP archive: ${error instanceof Error ? error.message : String(error)}`, + DocxErrorCode.INVALID_DOCX, + { originalError: error instanceof Error ? error.stack : undefined } + ); + } }src/tools/docx/errors.ts (1)
9-18: Consider narrowingcodefromstringtoDocxErrorCodefor type safety.The constructor accepts
code: string, but the codebase definesDocxErrorCodeenum for all known codes. Narrowing the type would prevent passing arbitrary strings and ensure all errors use standardized codes.Proposed change
export class DocxError extends Error { constructor( message: string, - public readonly code: string, + public readonly code: DocxErrorCode, public readonly context?: Record<string, unknown> ) {This would also require updating
withErrorContext(line 62) to accept onlyDocxErrorCodeinstead ofDocxErrorCode | string.test/test-docx-reading.js (1)
6-6: Tests import from compileddist/output.Importing from
../dist/tools/docx/markdown.jscouples tests to a prior build step. If the build is stale, tests will pass against old code. Consider using a test framework that supports TypeScript directly (e.g., vitest, tsx) or document the build prerequisite.src/tools/docx/parsers/paragraph-parser.ts (1)
1-7:createRequireis imported but unused.Line 1 imports
createRequireand line 3 creates arequirefunction, but neither is actually used — thedocxlibrary is imported via the ESimportstatement on line 5. This is dead code.Proposed fix
-import { createRequire } from 'module'; - -const require = createRequire(import.meta.url); // `@ts-ignore` import * as docx from 'docx';src/tools/docx/operations/index.ts (1)
3-3: Unused import:zfromzod.
zis imported but never referenced in this file.♻️ Remove unused import
-import { z } from 'zod';src/tools/docx/markdown.ts (1)
391-395: Docxtemplater is instantiated only for validation, then discarded.The instance is never used for actual parsing — all work is done via raw XML. This adds unnecessary startup cost and a dependency. If the intent is just to validate the DOCX structure, consider removing this or making it opt-in.
src/tools/docx/parsers/table-parser.ts (1)
6-8: UnusedcreateRequireimport.
createRequireis imported andrequireis assigned (Lines 6–8), but neither is used —docxis loaded via ES import at Line 10.♻️ Remove unused import
-import { createRequire } from 'module'; - -const require = createRequire(import.meta.url);src/tools/docx/builders/markdown-builder.ts (1)
1-5: UnusedcreateRequire/require— dead code.
createRequireis imported and called on lines 1 and 3, but the resultingrequirefunction is never used anywhere in this file. This looks like a leftover from an earlier approach.🧹 Remove unused import and variable
-import { createRequire } from 'module'; - -const require = createRequire(import.meta.url); // `@ts-ignore` import * as docx from 'docx';src/tools/docx/structure.ts (1)
9-15: UnusedcreateRequire/require— same dead code as inmarkdown-builder.ts.
requireis created but never called. OnlyDocumentandPackerare actually used, and they come from the ESMimport * as docx.🧹 Remove unused import and variable
-import { createRequire } from 'module'; - -const require = createRequire(import.meta.url); -// `@ts-ignore` -import * as docx from 'docx'; +// `@ts-ignore` - docx library has incomplete type definitions +import * as docx from 'docx'; const { Document, Packer } = docx as any;src/utils/files/docx.ts (3)
28-31: Parameterpathshadows the importedpathmodule.In
canHandle(and similarly inread,write,editRange,getInfo), the parameter is namedpath, which shadows the Node.jspathmodule imported on line 7. This is a maintenance hazard — if any method later needspath.extname()or similar, the shadowed import will cause subtle bugs.Also, the variable
exton line 29 is actually the full lowercased path, not just the extension.♻️ Rename parameter to `filePath` across all methods
- canHandle(path: string): boolean { - const ext = path.toLowerCase(); + canHandle(filePath: string): boolean { + const ext = filePath.toLowerCase(); return this.extensions.some(e => ext.endsWith(e)); }Apply the same rename (
path→filePath) toread,write,editRange, andgetInfo.
111-140: No runtime validation ofDocxOperation[]inwriteandeditRange.When
contentis an array (line 133), it's cast toDocxOperation[]without any runtime check that the elements have validtypefields. Passing malformed objects (e.g.,[{ foo: "bar" }]) would propagate toeditDocxWithOperationsand produce confusing errors.A lightweight guard (e.g., checking that each element has a
typeproperty matching known operation types) would improve error messages for callers.
89-99:readsilently swallows errors and returns them as content.On parse failure, the handler returns a
FileResultwith the error message ascontentandmimeType: 'text/plain'. The caller has no easy programmatic way to distinguish success from failure without inspectingmetadata.error. Consider whether this should throw or at least document this contract clearly, since otherFileHandlerimplementations may throw.src/tools/docx/index.ts (1)
55-79: Consider narrowing the public API surface for utilities.This barrel exports ~16 utility functions (e.g.,
escapeRegExp,fileExists,formatFileSize,parseDataUrl,splitMarkdownLines) that are general-purpose helpers unlikely to be needed by external consumers. Exposing them increases the API surface to maintain and couples consumers to internal implementation details.Consider keeping only DOCX-specific utilities (like
prepareImageForDocx,createImageRun,isDocxPath) in the public API, and importing the rest directly from./utils.jswhere needed internally.src/tools/docx/utils.ts (1)
334-341: Hardcoded default image dimensions (600×400) with no override path.
prepareImageForDocxalways returnswidth: 600, height: 400regardless of the actual image dimensions. While the comment says "can be customized," the function signature provides no way to pass desired dimensions. This means all embedded images will be rendered at the same fixed size.Consider accepting optional
width/heightparameters (they already exist onPreparedImage), or detecting actual image dimensions from the buffer.src/tools/docx/types.ts (1)
6-14: All runtime type aliases areany— reduces type safety across the DOCX module.While this is pragmatic given incomplete
docxlibrary typings, it means no compile-time checks for misuse ofDocxParagraph,DocxTable, etc. throughout the codebase.Consider gradually replacing these with more specific types as the module stabilizes (e.g.,
import type { Paragraph } from 'docx'if available, or at least structural types capturing the properties you depend on).
| children: headerCells.map((cell) => { | ||
| const textRuns = parseInlineFormatting(cell.trim()); | ||
| textRuns.forEach(run => { | ||
| (run as any).bold = true; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
docx npm library TextRun bold property mutation after construction
💡 Result:
In docx (dolanmiu/docx) you generally can’t reliably “flip” bold by mutating some options object after a TextRun is constructed:
- The run styling options type (
IRunStylePropertiesOptions) marks properties likeboldasReadonly, i.e. they’re intended to be set at creation time (or via provided builder methods), not mutated later. [1] - The supported way to apply bold after construction is the fluent/builder API:
new TextRun("Name:").bold()(this mutates theTextRuninstance via its method, not by mutating your original options). [2]
What to do instead
- Set it at construction:
const r = new TextRun({ text: "Hello", bold: true });
- Or use the builder method:
const r = new TextRun("Hello").bold();
- If you need to change bold conditionally after the fact (especially to turn it off), the simplest approach is usually to create a new
TextRunwith the desiredboldvalue, because the API surface is primarily “set properties when building the run”, not “mutable style setters for every property”.
Sources: [1] [2]
🏁 Script executed:
# Find the markdown-builder.ts file and examine lines around 220-224
find . -name "markdown-builder.ts" -type fRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 121
🏁 Script executed:
# Once found, read the file to understand the context
head -250 src/tools/docx/builders/markdown-builder.ts | tail -50Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1459
🏁 Script executed:
# Search for parseInlineFormatting function definition
rg "function parseInlineFormatting|parseInlineFormatting\s*=" -A 20Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1654
🏁 Script executed:
# Check if there are any uses of .bold() method in the codebase for TextRun
rg "\.bold\(" src/ --type tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 58
🌐 Web query:
docx TextRun.bold() builder method fluent API
💡 Result:
In the dolanmiu docx (JS/TS) library, TextRun supports a fluent/builder method .bold() (no arguments) that sets the run’s bold formatting to true:
import { TextRun } from "docx";
const run = new TextRun("Name:").bold();This is shown in the project’s “Styling with JS” fluent API docs, where .bold() is listed as a run-formatting method. [1]
If you’re using the object/options form instead of fluent chaining, bold is also available as a boolean run property (bold?: boolean):
const run = new TextRun({ text: "Name:", bold: true });The bold option is part of IRunStylePropertiesOptions. [2]
Sources: [1] [2]
Bug: Mutating bold on constructed TextRun has no effect.
In the docx library, TextRun properties are Readonly and the internal XML representation is built at construction time. Assigning (run as any).bold = true after the fact does not update the output. Header cells will not render as bold.
Use the supported fluent API instead:
children: headerCells.map((cell) => {
const textRuns = parseInlineFormatting(cell.trim());
- textRuns.forEach(run => {
- (run as any).bold = true;
- });
+ textRuns.forEach(run => run.bold());
return new TableCell({Alternatively, refactor parseInlineFormatting to accept an optional bold flag and construct runs with that property set at initialization.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| children: headerCells.map((cell) => { | |
| const textRuns = parseInlineFormatting(cell.trim()); | |
| textRuns.forEach(run => { | |
| (run as any).bold = true; | |
| }); | |
| children: headerCells.map((cell) => { | |
| const textRuns = parseInlineFormatting(cell.trim()); | |
| textRuns.forEach(run => run.bold()); | |
| return new TableCell({ |
🤖 Prompt for AI Agents
In `@src/tools/docx/builders/markdown-builder.ts` around lines 220 - 224, The
header mapping mutates constructed TextRun objects ((run as any).bold = true)
which does nothing; instead update the code to create bold runs at construction
time—either call the TextRun fluent API (e.g., use the run.bold() chain) when
building runs inside the header mapping or refactor parseInlineFormatting to
accept an optional bold flag and have it construct and return runs with bold
set. Update the headerCells.map usage to pass that flag into
parseInlineFormatting or to apply the fluent .bold() on each returned run
immediately when creating the runs so header text is actually bold in the
generated docx.
| const isUrl = (source: string): boolean => | ||
| source.startsWith('http://') || source.startsWith('https://'); | ||
|
|
||
| /** | ||
| * Load DOCX file as buffer | ||
| */ | ||
| async function loadDocxToBuffer(source: string): Promise<Buffer> { | ||
| if (isUrl(source)) { | ||
| const response = await fetch(source); | ||
| const arrayBuffer = await response.arrayBuffer(); | ||
| return Buffer.from(arrayBuffer); | ||
| } else { | ||
| return await fs.readFile(source); | ||
| } | ||
| } | ||
|
|
||
| function readZipFileText(zip: any, filePath: string): string | null { | ||
| const file = zip.file(filePath); | ||
| if (!file) return null; | ||
| if (typeof file.asText === 'function') { | ||
| return file.asText(); | ||
| } | ||
| if (typeof file.asBinary === 'function') { | ||
| return Buffer.from(file.asBinary(), 'binary').toString('utf8'); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| function readZipFileBuffer(zip: any, filePath: string): Buffer | null { | ||
| const file = zip.file(filePath); | ||
| if (!file) return null; | ||
| if (typeof file.asUint8Array === 'function') { | ||
| return Buffer.from(file.asUint8Array()); | ||
| } | ||
| if (typeof file.asNodeBuffer === 'function') { | ||
| return file.asNodeBuffer(); | ||
| } | ||
| if (typeof file.asBinary === 'function') { | ||
| return Buffer.from(file.asBinary(), 'binary'); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| function getMimeTypeForTarget(target: string): string { | ||
| const ext = path.extname(target).toLowerCase(); | ||
| const mimeTypes: Record<string, string> = { | ||
| '.png': 'image/png', | ||
| '.jpg': 'image/jpeg', | ||
| '.jpeg': 'image/jpeg', | ||
| '.gif': 'image/gif', | ||
| '.bmp': 'image/bmp', | ||
| '.webp': 'image/webp', | ||
| '.svg': 'image/svg+xml', | ||
| }; | ||
| return mimeTypes[ext] || 'application/octet-stream'; | ||
| } | ||
|
|
||
| function escapeTableCell(text: string): string { | ||
| return text.replace(/\|/g, '\\|').replace(/\r?\n/g, '<br>'); | ||
| } | ||
|
|
||
| function getElementChildren(node: Node): Element[] { | ||
| const children: Element[] = []; | ||
| for (let i = 0; i < node.childNodes.length; i++) { | ||
| const child = node.childNodes[i]; | ||
| if (child.nodeType === 1) { | ||
| children.push(child as Element); | ||
| } | ||
| } | ||
| return children; | ||
| } | ||
|
|
||
| function getAttributeValue(node: Element, name: string): string | null { | ||
| return node.getAttribute(name) || node.getAttribute(`w:${name}`) || null; | ||
| } | ||
|
|
||
| function getHeadingLevelFromParagraph(paragraph: Element): number | null { | ||
| const pPr = paragraph.getElementsByTagName('w:pPr')[0]; | ||
| if (!pPr) return null; | ||
| const pStyle = pPr.getElementsByTagName('w:pStyle')[0]; | ||
| if (!pStyle) return null; | ||
| const styleVal = getAttributeValue(pStyle, 'val'); | ||
| if (!styleVal) return null; | ||
| const match = styleVal.match(/heading\s*([1-6])/i); | ||
| if (!match) return null; | ||
| return Number(match[1]); | ||
| } | ||
|
|
||
| function extractRelationshipMap(relsXml: string | null): Map<string, { target: string; type: string }> { | ||
| const relMap = new Map<string, { target: string; type: string }>(); | ||
| if (!relsXml) return relMap; | ||
| const relDoc = new DOMParser().parseFromString(relsXml, 'application/xml'); | ||
| const rels = relDoc.getElementsByTagName('Relationship'); | ||
| for (let i = 0; i < rels.length; i++) { | ||
| const rel = rels[i]; | ||
| const id = rel.getAttribute('Id'); | ||
| const type = rel.getAttribute('Type') || ''; | ||
| const target = rel.getAttribute('Target') || ''; | ||
| if (id && target) { | ||
| relMap.set(id, { target, type }); | ||
| } | ||
| } | ||
| return relMap; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Significant code duplication with the parsers/ modules.
The following functions are copy-pasted from already-exported modules in this PR:
| Function | Duplicate of |
|---|---|
isUrl (L88) |
src/tools/docx/utils.ts |
readZipFileText (L104) |
src/tools/docx/parsers/zip-reader.ts |
readZipFileBuffer (L116) |
src/tools/docx/parsers/zip-reader.ts |
getMimeTypeForTarget (L131) |
src/tools/docx/parsers/image-extractor.ts |
getElementChildren (L149) |
src/tools/docx/parsers/xml-parser.ts |
getAttributeValue (L160) |
src/tools/docx/parsers/xml-parser.ts |
getHeadingLevelFromParagraph (L164) |
src/tools/docx/parsers/xml-parser.ts |
extractRelationshipMap (L176) |
src/tools/docx/parsers/xml-parser.ts |
Import from the canonical sources instead to avoid drift and reduce the module's size by ~100 lines.
🤖 Prompt for AI Agents
In `@src/tools/docx/markdown.ts` around lines 88 - 191, Remove the duplicated
helper implementations and import the canonical implementations from the
existing modules: replace local isUrl with the export from
src/tools/docx/utils.ts; replace readZipFileText and readZipFileBuffer with the
exports from parsers/zip-reader.ts; replace getMimeTypeForTarget with the export
from parsers/image-extractor.ts; replace getElementChildren, getAttributeValue,
getHeadingLevelFromParagraph and extractRelationshipMap with the exports from
parsers/xml-parser.ts — update the top-level imports and delete the local
function definitions (isUrl, readZipFileText, readZipFileBuffer,
getMimeTypeForTarget, getElementChildren, getAttributeValue,
getHeadingLevelFromParagraph, extractRelationshipMap) so this module reuses the
single source of truth.
| async function loadDocxToBuffer(source: string): Promise<Buffer> { | ||
| if (isUrl(source)) { | ||
| const response = await fetch(source); | ||
| const arrayBuffer = await response.arrayBuffer(); | ||
| return Buffer.from(arrayBuffer); | ||
| } else { | ||
| return await fs.readFile(source); | ||
| } | ||
| } |
There was a problem hiding this comment.
loadDocxToBuffer from URL: no response status check.
If fetch returns a non-2xx response (404, 500, etc.), response.arrayBuffer() will silently succeed with the error page body. Check response.ok before consuming the body.
🛡️ Proposed fix
async function loadDocxToBuffer(source: string): Promise<Buffer> {
if (isUrl(source)) {
const response = await fetch(source);
+ if (!response.ok) {
+ throw new Error(`Failed to fetch DOCX from URL: ${response.status} ${response.statusText}`);
+ }
const arrayBuffer = await response.arrayBuffer();
return Buffer.from(arrayBuffer);
} else {🤖 Prompt for AI Agents
In `@src/tools/docx/markdown.ts` around lines 94 - 102, The loadDocxToBuffer
function should validate HTTP responses when source is a URL: after fetching in
loadDocxToBuffer (and using isUrl to detect URLs), check response.ok and if
false throw or return an error that includes response.status and
response.statusText (and optionally the URL) before calling
response.arrayBuffer(), so non-2xx responses (404/500) are surfaced instead of
silently returning an error page body.
| function convertTableToMarkdown(table: Element, resolveImage: (relId: string | null) => string): string | null { | ||
| const rows: string[][] = []; | ||
| const rowNodes = table.getElementsByTagName('w:tr'); | ||
| for (let i = 0; i < rowNodes.length; i++) { | ||
| const row = rowNodes[i]; | ||
| const cells = row.getElementsByTagName('w:tc'); | ||
| const rowCells: string[] = []; | ||
| for (let j = 0; j < cells.length; j++) { | ||
| const cell = cells[j]; | ||
| const paragraphs = cell.getElementsByTagName('w:p'); | ||
| const cellTexts: string[] = []; | ||
| for (let k = 0; k < paragraphs.length; k++) { | ||
| const text = extractParagraphText(paragraphs[k], resolveImage).trim(); | ||
| if (text) { | ||
| cellTexts.push(text); | ||
| } | ||
| } | ||
| const combined = cellTexts.length > 0 ? cellTexts.join('<br>') : ' '; | ||
| rowCells.push(escapeTableCell(combined)); | ||
| } | ||
| if (rowCells.length > 0) { | ||
| rows.push(rowCells); | ||
| } | ||
| } |
There was a problem hiding this comment.
getElementsByTagName('w:tr') is recursive and will pick up nested table rows.
If a DOCX table contains nested tables, getElementsByTagName traverses into nested w:tbl elements and returns all w:tr descendants, mixing inner and outer rows. The same applies to Line 284 for w:tc and Line 288 for w:p.
Use direct children iteration (like getElementChildren) and filter by nodeName instead:
♻️ Sketch of non-recursive row iteration
- const rowNodes = table.getElementsByTagName('w:tr');
- for (let i = 0; i < rowNodes.length; i++) {
- const row = rowNodes[i];
+ const rowNodes = getElementChildren(table).filter(c => c.nodeName === 'w:tr');
+ for (const row of rowNodes) {Apply the same pattern for w:tc within a row and w:p within a cell.
🤖 Prompt for AI Agents
In `@src/tools/docx/markdown.ts` around lines 279 - 302, convertTableToMarkdown
currently uses recursive getElementsByTagName('w:tr'|'w:tc'|'w:p') which pulls
in nested table rows/cells/paras; replace those calls with a non‑recursive child
iteration (e.g. use a getElementChildren helper or loop over element.childNodes
and filter by node.nodeName === 'w:tr' / 'w:tc' / 'w:p') so only direct children
are processed; update the loops in convertTableToMarkdown that build rowNodes,
cells and paragraphs to use this child-filtering approach, keeping use of
extractParagraphText and escapeTableCell unchanged.
| if (allTextReplacements && operations.length > 0) { | ||
| let modifiedBuffer: Buffer = await fs.readFile(docxPath); | ||
|
|
||
| for (let i = 0; i < operations.length; i++) { | ||
| const op = operations[i]; | ||
| const validatedOp = DocxReplaceTextOperationSchema.parse(op); | ||
| const tempPath = `${docxPath}.tmp${i}`; | ||
|
|
||
| await fs.writeFile(tempPath, modifiedBuffer); | ||
| modifiedBuffer = await replaceTextInDocxXml( | ||
| tempPath, | ||
| validatedOp.search, | ||
| validatedOp.replace | ||
| ); | ||
|
|
||
| await fs.unlink(tempPath).catch(() => {}); | ||
| } | ||
|
|
||
| return modifiedBuffer; |
There was a problem hiding this comment.
Temp files leak on error and are unnecessary given the in-memory buffer.
Two concerns with the temp-file loop:
- Leak on error: If
replaceTextInDocxXmlthrows at iterationi, the temp file written at Line 42 is never cleaned up becauseunlinkat Line 49 is skipped. - Unnecessary disk I/O: You already hold
modifiedBufferin memory (Line 35). Writing it to disk only to havereplaceTextInDocxXmlread it back is wasteful. Consider refactoringreplaceTextInDocxXml(or adding a variant) to accept aBufferdirectly.
At minimum, wrap the loop body in try/finally to ensure cleanup:
🛡️ Proposed fix for temp file cleanup
for (let i = 0; i < operations.length; i++) {
const op = operations[i];
const validatedOp = DocxReplaceTextOperationSchema.parse(op);
const tempPath = `${docxPath}.tmp${i}`;
-
- await fs.writeFile(tempPath, modifiedBuffer);
- modifiedBuffer = await replaceTextInDocxXml(
- tempPath,
- validatedOp.search,
- validatedOp.replace
- );
-
- await fs.unlink(tempPath).catch(() => {});
+
+ try {
+ await fs.writeFile(tempPath, modifiedBuffer);
+ modifiedBuffer = await replaceTextInDocxXml(
+ tempPath,
+ validatedOp.search,
+ validatedOp.replace
+ );
+ } finally {
+ await fs.unlink(tempPath).catch(() => {});
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (allTextReplacements && operations.length > 0) { | |
| let modifiedBuffer: Buffer = await fs.readFile(docxPath); | |
| for (let i = 0; i < operations.length; i++) { | |
| const op = operations[i]; | |
| const validatedOp = DocxReplaceTextOperationSchema.parse(op); | |
| const tempPath = `${docxPath}.tmp${i}`; | |
| await fs.writeFile(tempPath, modifiedBuffer); | |
| modifiedBuffer = await replaceTextInDocxXml( | |
| tempPath, | |
| validatedOp.search, | |
| validatedOp.replace | |
| ); | |
| await fs.unlink(tempPath).catch(() => {}); | |
| } | |
| return modifiedBuffer; | |
| if (allTextReplacements && operations.length > 0) { | |
| let modifiedBuffer: Buffer = await fs.readFile(docxPath); | |
| for (let i = 0; i < operations.length; i++) { | |
| const op = operations[i]; | |
| const validatedOp = DocxReplaceTextOperationSchema.parse(op); | |
| const tempPath = `${docxPath}.tmp${i}`; | |
| try { | |
| await fs.writeFile(tempPath, modifiedBuffer); | |
| modifiedBuffer = await replaceTextInDocxXml( | |
| tempPath, | |
| validatedOp.search, | |
| validatedOp.replace | |
| ); | |
| } finally { | |
| await fs.unlink(tempPath).catch(() => {}); | |
| } | |
| } | |
| return modifiedBuffer; |
🤖 Prompt for AI Agents
In `@src/tools/docx/operations/index.ts` around lines 34 - 52, The loop writes
temporary files and leaks them on errors and is wasteful; update the logic so
replaceTextInDocxXml is called with an in-memory Buffer instead of writing temp
files (either add a new API like replaceTextInDocxXmlBuffer(buffer, search,
replace) or overload replaceTextInDocxXml to accept a Buffer), then call that
with modifiedBuffer in the for-loop (using
DocxReplaceTextOperationSchema.parse(op) to get validatedOp) and remove tempPath
writes/unlinks; if you can't change replaceTextInDocxXml immediately, wrap the
per-iteration work (writeFile, call replaceTextInDocxXml, unlink) in a
try/finally to ensure tempPath is always unlinked even when replaceTextInDocxXml
throws, referencing modifiedBuffer, tempPath, replaceTextInDocxXml and
DocxReplaceTextOperationSchema to locate the code to change.
| const rowNodes = table.getElementsByTagName('w:tr'); | ||
|
|
||
| for (let i = 0; i < rowNodes.length; i++) { | ||
| const rowNode = rowNodes[i]; | ||
| const row = parseTableRow(rowNode, images); | ||
| if (row) { | ||
| rows.push(row); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same nested getElementsByTagName issue — rows from nested tables will be included.
getElementsByTagName('w:tr') at Line 25 recurses into nested w:tbl elements. The same applies to w:tc (Line 53) and w:p (Line 80). Use direct-child iteration to avoid mixing rows/cells from nested tables.
♻️ Proposed fix using getElementChildren
+import { getElementChildren } from './xml-parser.js';
+
export function parseTableElement(
table: Element,
images: Map<string, Buffer>
): DocxTable | null {
const rows: InstanceType<typeof TableRow>[] = [];
- const rowNodes = table.getElementsByTagName('w:tr');
-
- for (let i = 0; i < rowNodes.length; i++) {
- const rowNode = rowNodes[i];
+ const rowNodes = getElementChildren(table).filter(c => c.nodeName === 'w:tr');
+
+ for (const rowNode of rowNodes) {
const row = parseTableRow(rowNode, images);Apply the same to parseTableRow (for w:tc) and parseTableCell (for w:p).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const rowNodes = table.getElementsByTagName('w:tr'); | |
| for (let i = 0; i < rowNodes.length; i++) { | |
| const rowNode = rowNodes[i]; | |
| const row = parseTableRow(rowNode, images); | |
| if (row) { | |
| rows.push(row); | |
| } | |
| } | |
| import { getElementChildren } from './xml-parser.js'; | |
| export function parseTableElement( | |
| table: Element, | |
| images: Map<string, Buffer> | |
| ): DocxTable | null { | |
| const rows: InstanceType<typeof TableRow>[] = []; | |
| const rowNodes = getElementChildren(table).filter(c => c.nodeName === 'w:tr'); | |
| for (const rowNode of rowNodes) { | |
| const row = parseTableRow(rowNode, images); | |
| if (row) { | |
| rows.push(row); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/tools/docx/parsers/table-parser.ts` around lines 25 - 33, The parser is
using getElementsByTagName which recurses into nested tables and pulls
rows/cells/paragraphs from inner tables; update the logic in the table parsing
flow to iterate only direct children instead of using getElementsByTagName: in
the block that builds rows from table (where rowNodes =
table.getElementsByTagName('w:tr')), replace that recursive lookup with a
direct-child iteration (e.g., an element children filter) so only immediate
'w:tr' children are processed; apply the same change inside parseTableRow
(replace getElementsByTagName('w:tc') with direct-child iteration for immediate
'w:tc' cells) and inside parseTableCell (replace getElementsByTagName('w:p')
with direct-child iteration for immediate 'w:p' paragraphs) so nested w:tbl
content is not mixed into parent table parsing.
| /** | ||
| * DOCX metadata structure | ||
| */ | ||
| export interface DocxMetadata { | ||
| title?: string; | ||
| author?: string; | ||
| creator?: string; | ||
| subject?: string; | ||
| description?: string; | ||
| creationDate?: Date; | ||
| modificationDate?: Date; | ||
| lastModifiedBy?: string; | ||
| revision?: string; | ||
| fileSize?: number; | ||
| } | ||
|
|
||
| /** | ||
| * Embedded image information | ||
| */ | ||
| export interface DocxImage { | ||
| id: string; | ||
| data: string; // Base64-encoded | ||
| mimeType: string; | ||
| altText?: string; | ||
| originalSize?: number; | ||
| } | ||
|
|
||
| /** | ||
| * DOCX section/paragraph structure | ||
| */ | ||
| export interface DocxSection { | ||
| type: 'heading' | 'paragraph' | 'list' | 'table' | 'image'; | ||
| content: string; | ||
| level?: number; | ||
| images?: DocxImage[]; | ||
| } | ||
|
|
||
| /** | ||
| * Complete DOCX parse result | ||
| */ | ||
| export interface DocxParseResult { | ||
| markdown: string; | ||
| metadata: DocxMetadata; | ||
| images: DocxImage[]; | ||
| sections?: DocxSection[]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
DocxMetadata, DocxImage, DocxSection, and DocxParseResult are duplicated in markdown.ts.
These four interfaces are defined identically in both src/tools/docx/types.ts (here) and src/tools/docx/markdown.ts (lines 17-82). The barrel index.ts re-exports them from markdown.ts, making these copies in types.ts unused via the public API — but other internal files importing from ./types.js would get a different (identical but distinct) type.
Consolidate into a single canonical location. Since types.ts is the intended central type file, consider having markdown.ts import from types.ts instead of re-declaring them.
🤖 Prompt for AI Agents
In `@src/tools/docx/types.ts` around lines 47 - 92, The four interfaces
DocxMetadata, DocxImage, DocxSection, and DocxParseResult are duplicated; remove
the duplicate declarations from markdown.ts and have markdown.ts import those
types from types.ts instead, updating any local references to use the imported
types; ensure index.ts continues to re-export the canonical types (from
types.ts) so public API remains the same and adjust any internal imports that
were pointing to ./types.js to reference the single types.ts definitions.
| * Image data prepared for DOCX embedding | ||
| */ | ||
| export interface PreparedImage { | ||
| buffer: Buffer; | ||
| width?: number; | ||
| height?: number; | ||
| altText: string; | ||
| mimeType: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
PreparedImage interface is duplicated — defined both here and in types.ts.
This interface is defined identically at src/tools/docx/types.ts:122-128 and here at lines 252-258. Both are exported, and the barrel index.ts re-exports from both utils.ts (line 78) and types.ts (indirectly via operations.ts). This DRY violation will cause drift as one copy gets updated but not the other.
♻️ Remove the duplicate and import from `types.ts`
-/**
- * Image data prepared for DOCX embedding
- */
-export interface PreparedImage {
- buffer: Buffer;
- width?: number;
- height?: number;
- altText: string;
- mimeType: string;
-}
+// Re-export PreparedImage from the canonical location
+export type { PreparedImage } from './types.js';🤖 Prompt for AI Agents
In `@src/tools/docx/utils.ts` around lines 250 - 258, PreparedImage is duplicated
in utils.ts and types.ts; remove the duplicate declaration from utils.ts and
instead import { PreparedImage } from the existing src/tools/docx/types.ts where
needed (e.g., in functions like any helpers in utils that currently reference
PreparedImage). Update utils.ts exports so it no longer re-exports PreparedImage
(ensure index.ts and operations.ts still re-export from types.ts), and run a
quick compile to fix any missing import sites and adjust import paths to
reference the single source of truth PreparedImage in types.ts.
| for (const o of content) { | ||
| if (o.type === 'insertImage' && o.imagePath) { | ||
| // Only validate local paths (not data URLs or web URLs) | ||
| if (!o.imagePath.startsWith('data:') && !o.imagePath.startsWith('http')) { | ||
| try { | ||
| o.imagePath = await validatePath(o.imagePath); | ||
| } catch { | ||
| // If validation fails, keep original path | ||
| // Error will be caught during operation execution | ||
| } | ||
| } | ||
| } | ||
| operations.push(o); | ||
| } |
There was a problem hiding this comment.
Silent error swallowing on path validation weakens security guarantees.
When validatePath fails for an image path (lines 965-970), the error is silently caught and the original unvalidated path is retained. This is inconsistent with how writePdf handles sourcePdfPath validation (it does not catch errors), and it could mask real path-traversal violations. If the path fails validation, the operation should fail explicitly rather than proceeding with an unvalidated path.
Proposed fix
if (o.type === 'insertImage' && o.imagePath) {
// Only validate local paths (not data URLs or web URLs)
- if (!o.imagePath.startsWith('data:') && !o.imagePath.startsWith('http')) {
- try {
- o.imagePath = await validatePath(o.imagePath);
- } catch {
- // If validation fails, keep original path
- // Error will be caught during operation execution
- }
+ if (!o.imagePath.startsWith('data:') && !o.imagePath.startsWith('http://') && !o.imagePath.startsWith('https://')) {
+ o.imagePath = await validatePath(o.imagePath);
}
}Note: The startsWith('http') check is also too loose — it matches any string starting with "http" (e.g., httpfoo://...). The fix above tightens this to http:// and https://.
🤖 Prompt for AI Agents
In `@src/tools/filesystem.ts` around lines 961 - 974, The loop handling
insertImage operations silently swallows validatePath errors and keeps
unvalidated imagePath; instead let validation failures surface so the operation
fails: remove or rethrow inside the try/catch around validatePath for the
insertImage branch (referencing validatePath, o.imagePath, and the insertImage
operation) so errors propagate like writePdf's sourcePdfPath validation, and
tighten the URL check from startsWith('http') to explicit startsWith('http://')
|| startsWith('https://') to avoid false positives.
| try { | ||
| // Check if sample file exists | ||
| try { | ||
| await fs.access(samplePath); | ||
| } catch { | ||
| log(colors.yellow, 'Sample DOCX file not found, skipping test'); | ||
| log(colors.cyan, `Expected location: ${samplePath}`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Skipped tests are silently counted as failures.
When sample.docx is absent, testBasicDocxReading, testFormattingPreservation, and testMetadataExtraction return undefined (no explicit return value). In runAllTests, the check results.filter(r => r === true) counts undefined as a failure. This means the suite always exits non-zero in any environment without the sample file (e.g. CI), making it impossible to distinguish "skipped" from "genuinely failed."
Consider returning a distinct sentinel or tracking skipped tests separately:
Proposed fix sketch for runAllTests
- const passed = results.filter(r => r === true).length;
- const total = results.length;
+ const passed = results.filter(r => r === true).length;
+ const skipped = results.filter(r => r === undefined || r === null).length;
+ const failed = results.filter(r => r === false).length;
+ const total = results.length;
- if (passed === total) {
+ if (failed === 0 && passed > 0) {
log(colors.green, `\n✓ All tests passed (${passed}/${total})`);
+ } else if (failed === 0) {
+ log(colors.yellow, `\n⚠ All tests skipped (missing sample.docx)`);
} else {
- log(colors.yellow, `\n⚠ ${passed}/${total} tests passed`);
+ log(colors.yellow, `\n⚠ ${passed} passed, ${failed} failed, ${skipped} skipped`);
}
- process.exit(passed === total ? 0 : 1);
+ process.exit(failed > 0 ? 1 : 0);Also applies to: 112-117, 172-177
🤖 Prompt for AI Agents
In `@test/test-docx-reading.js` around lines 39 - 47, When the sample DOCX is
missing, each test (testBasicDocxReading, testFormattingPreservation,
testMetadataExtraction) should explicitly return a distinct sentinel (e.g., the
string 'skipped' or a SKIPPED Symbol) instead of undefined; update the
early-exit blocks that currently log "Sample DOCX file not found" to return that
sentinel. Then update runAllTests to treat results strictly (count passes where
r === true, failures where r === false, and skipped where r === 'skipped' or the
SKIPPED Symbol) and report/exit accordingly so skipped tests are not treated as
failures. Ensure all three test functions and runAllTests reference the same
sentinel constant (SKIPPED) to keep behavior consistent.
2 participants
User description
read: Convert existing DOCX files into markdown plus metadata, while preserving structure (headings, tables, images).
for reading and parsing, I combined pizzip (DOCX as ZIP), @xmldom/xmldom (XML parsing of WordprocessingML), and docx (to represent content as Document, Paragraph, Table, ImageRun, etc.).
create: Generate new DOCX files from markdown content, including headings, tables, images, and basic text formatting.
for creation, markdown is parsed line‑by‑line to detect tables, headings, images, and paragraphs, which are then mapped to the appropriate docx primitives and assembled into a new Document.
update: Apply high‑level operations to existing DOCX files (append markdown, insert tables, insert images, replace text) in a way that maintains the original layout.
for updates, there are two paths: a fast XML path that edits word/document.xml directly for simple replaceText operations, and a structure path that uses the DocxStructure representation for structural changes like appending sections or inserting tables/images.
all edits are expressed as high‑level operations validated via zod schemas: replaceText, appendMarkdown, insertTable, and insertImage.
when all operations are replaceText, the system uses efficient XML replacement, which is safe for formatting and tables.
when any structural operation is present, the system parses the existing DOCX to DocxStructure, applies each operation (e.g., convert the appended markdown into a temporary structure and merge it), and then rebuilds the DOCX from the updated element list.
CodeAnt-AI Description
Add full DOCX (Word) file support: read, create, and edit .docx files
What Changed
Impact
✅ Read DOCX as Markdown with images and metadata✅ Create Word documents from Markdown✅ Apply safe edits to DOCX (search/replace, append, insert tables/images)💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Release Notes
write_docxtool enables creating new Word documents or modifying existing ones with multiple editing operations.