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 · |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds full DOCX support across the stack: dependencies, DOM & ZIP utilities, readers/writers/creators, many patch ops and builders, schema and server tool integration, filesystem/file-handler wiring, and handler-level call routing for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Handler as handleReadDocx
participant Zip as loadDocxZip/getDocumentXml
participant Dom as parseXml
participant Reader as readDocxOutline
Client->>Server: call read_docx { path }
Server->>Handler: invoke handleReadDocx(args)
Handler->>Zip: loadDocxZip(path)
Zip-->>Handler: PizZip
Handler->>Zip: getDocumentXml(zip)
Zip-->>Handler: document.xml
Handler->>Dom: parseXml(document.xml)
Dom-->>Handler: Document DOM
Handler->>Reader: readDocxOutline(path)
Reader-->>Handler: ReadDocxResult
Handler-->>Server: JSON response
Server-->>Client: outline + metadata
sequenceDiagram
participant Client
participant Server
participant Handler as handleWriteDocx
participant Writer as writeDocxPatched
participant Zip as loadDocxZip/getDocumentXml/saveDocxZip
participant Dom as parseXml/serializeXml
participant OpDisp as applyOp
participant Validator as validateInvariants
Client->>Server: call write_docx { inputPath?, outputPath, ops/content }
Server->>Handler: invoke handleWriteDocx(args)
Handler->>Writer: writeDocxPatched(inputPath, outputPath, ops)
Writer->>Zip: loadDocxZip(inputPath)
Zip-->>Writer: PizZip
Writer->>Zip: getDocumentXml(zip)
Zip-->>Writer: document.xml
Writer->>Dom: parseXml(document.xml)
Dom-->>Writer: Document DOM
Writer->>Validator: captureSnapshot(body) (before)
loop for each op
Writer->>OpDisp: applyOp(body, op, zip?)
OpDisp-->>Writer: OpResult
end
Writer->>Validator: captureSnapshot(body) (after)
Validator-->>Writer: validateInvariants(before, after)
Writer->>Dom: serializeXml(doc)
Dom-->>Writer: modified XML
Writer->>Zip: saveDocxZip(zip, newXml, outputPath)
Zip-->>Writer: saved
Writer-->>Handler: WriteDocxResult
Handler-->>Server: JSON response
Server-->>Client: result + stats
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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
Nitpicks 🔍
|
|
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/filesystem-handlers.ts (1)
213-227:⚠️ Potential issue | 🟠 MajorPotential
TypeError—pagesandimagesaccessed without null guards.Line 215 calls
pdfPayload.pages.forEach(...)and line 216 callspage.images.forEach(...)without optional chaining. Ifresult.payloadis truthy butpagesis missing, or a page lacksimages, this throws at runtime. Compare withhandleReadFile(line 88) which uses?.flatMapand?? [].🐛 Proposed fix — add optional chaining / fallback
if (result.isPdf && result.payload) { const pdfPayload = result.payload as { pages: Array<{ text: string; images: Array<{ data: string; mimeType: string }> }> }; - pdfPayload.pages.forEach((page) => { - page.images.forEach((image) => { + (pdfPayload.pages ?? []).forEach((page) => { + (page.images ?? []).forEach((image) => { contentItems.push({ type: "image", data: image.data,
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 88: Update the package.json dependency "@xmldom/xmldom" from "^0.8.11" to
"^0.9.8" to pick up the latest stable release, then run your package manager
(npm install or yarn install) to update node_modules and the lockfile
(package-lock.json or yarn.lock); after updating, run the test suite and any
build/CI checks to ensure compatibility and commit the updated package.json and
lockfile.
In `@src/tools/docx/dom.ts`:
- Around line 17-19: The parseXml function currently calls new
DOMParser().parseFromString(...) which lets `@xmldom/xmldom` emit non-fatal parse
errors to console and return a partial DOM; update parseXml to instantiate
DOMParser with an errorHandler option (capture warnings/errors) and/or inspect
the returned Document for parsererror nodes, collect any non-fatal messages, and
throw a descriptive Error when any parse issues are detected so callers (e.g.,
getBody()) fail fast; refer to the parseXml function and the DOMParser from
`@xmldom/xmldom` when making this change.
In `@src/tools/docx/modify.ts`:
- Around line 270-284: The current branch that handles idx === paragraphs.length
uses body.appendChild(newP) which can append after a w:sectPr and produce a
malformed DOCX; instead, detect if body contains a final w:sectPr element and
insert newP before that element (fall back to appendChild only if no w:sectPr
exists). Update the insertion logic in the block around body.appendChild/newP
(references: body, newP, paragraphs, nodeListToArray) to search body.childNodes
for an Element with nodeName 'w:sectPr' and call body.insertBefore(newP,
sectPrNode) when found.
In `@src/tools/filesystem.ts`:
- Around line 958-980: The code currently uses a fragile content-sniffing
heuristic (content.trim().startsWith('<w:body')) to decide between creating a
DOCX and calling replaceBodyXml, which can misinterpret plain text that begins
with "<w:body"; change the API to make intent explicit by adding an explicit
mode/flag (e.g., mode: 'create' | 'replaceBody') to the function that accepts
content and use that flag to choose between calling replaceBodyXml(validPath,
targetPath, trimmedContent) and writeDocxImpl(targetPath, content) (maintaining
validatePath usage), or alternatively perform robust XML validation (parse
trimmedContent and verify it is well-formed XML with a w:body root) before
taking the replaceBodyXml path; update all callers to pass the new mode or
ensure content is validated accordingly.
In `@src/tools/schemas.ts`:
- Around line 118-122: The ReplaceParagraphAtBodyIndexOpSchema currently uses
bodyChildIndex: z.number(), which permits fractional and negative values causing
runtime TypeError in replace-paragraph-at-body-index.ts; update the schema to
enforce a non-negative integer (e.g., use z.number().int().nonnegative() or
equivalent) for the bodyChildIndex field so invalid fractional/negative indices
are rejected at parse time; keep the existing runtime bounds check in
replace-paragraph-at-body-index.ts for upper-bound validation.
In `@src/utils/files/base.ts`:
- Line 233: DocxFileHandler.getInfo currently returns fileType: 'binary' which
contradicts the updated fileType union that added 'docx'; update the
DocxFileHandler.getInfo method to return fileType: 'docx' (instead of 'binary')
and ensure any related metadata or callers expecting 'binary' are adjusted if
needed so the new 'docx' variant is used consistently (reference:
DocxFileHandler.getInfo and the fileType union in base.ts).
In `@src/utils/files/docx.ts`:
- Around line 149-159: The returned FileInfo object in the docx processor
currently sets fileType to 'binary'; update it to 'docx' so consumers can
correctly detect DOCX files—locate the return object in the function in
src/utils/files/docx.ts (the block that returns
size/created/modified/accessed/isDirectory/isFile/permissions/fileType/metadata)
and change fileType: 'binary' to fileType: 'docx'.
- Around line 93-107: The parsed JSON result is not validated and may be a
non-array value assigned to modifications; update the logic in the function
handling `content` (the branch that calls JSON.parse) so that after parsing you
check Array.isArray(parsed) and only assign to `modifications` when
true—otherwise fall back to treating the input as a single replace operation (as
the existing string fallback does) or throw a clear error; reference the
`content` variable and the `modifications` array so you locate the JSON.parse
branch and ensure `modifyDocxContent` receives an actual array of
DocxModification objects.
🧹 Nitpick comments (13)
src/tools/docx/zip.ts (1)
35-43:saveDocxZipmutates the passedzipinstance in place.Line 40 modifies the caller's
zipobject before writing. If a caller ever needs to save multiple variants or retry with different XML, this side effect could surprise. Consider documenting this or cloning before mutation. Low risk given current usage.src/tools/filesystem.ts (1)
950-997:writeDocxlacks telemetrycapture()calls, unlikewritePdf.
writePdf(Lines 897–930) logs telemetry for both creation and modification modes.writeDocxhas nocapture()calls, creating an observability gap. Consider adding similar telemetry for consistency.Example telemetry addition
if (typeof content === 'string') { const trimmedContent = content.trim(); if (trimmedContent.startsWith('<w:body')) { + capture('server_write_docx', { + fileExtension: '.docx', + mode: 'replaceBody', + contentLength: trimmedContent.length + }); const targetPath = outputPath ? await validatePath(outputPath) : validPath;src/tools/docx/dom.ts (2)
148-169:createElementused for namespaced OOXML elements instead ofcreateElementNS.Lines 154, 164 (and similar patterns in
styleParagraphRuns) usedoc.createElement('w:rPr')rather thandoc.createElementNS(nsUri, 'w:rPr'). In@xmldom/xmldom, this works in practice because the prefixw:is already declared in the document and xmldom preserves it. However, it produces elements without a proper namespace binding, which could cause issues with stricter XML tooling downstream.This is low risk for DOCX files consumed by Word/LibreOffice (they're lenient), but worth being aware of.
126-137:setParagraphTextMinimalsilently no-ops on paragraphs with no<w:t>nodes.If a paragraph has runs but no text nodes (e.g., image-only or field-code paragraphs), this function returns without any indication that the replacement didn't happen. Callers relying on this for text replacement won't know it was skipped.
Consider returning a boolean to indicate success, or at minimum, documenting this behavior at the call sites.
src/tools/docx/ops/set-color-for-paragraph-exact.ts (1)
16-16: No validation onop.color— invalid hex values will produce invalid OOXML.If an invalid color string (e.g.,
"red"instead of"FF0000") is passed, it will be written asw:val="red"which is not a valid OOXML color value. This won't cause a runtime error here but could produce a document that Word flags as corrupted.Consider validating at the op dispatch level or in the schema.
src/tools/schemas.ts (1)
124-134: Consider validatingcoloras a hex string.Both color op schemas accept any string for
color. A simple regex constraint would catch obvious typos early and provide a better error message than silent DOM corruption.Proposed fix
+const hexColor = z.string().regex(/^[0-9A-Fa-f]{6}$/, 'Expected 6-digit hex color (e.g. "FF0000")'); + const SetColorForStyleOpSchema = z.object({ type: z.literal('set_color_for_style'), style: z.string(), - color: z.string(), + color: hexColor, }); const SetColorForParagraphExactOpSchema = z.object({ type: z.literal('set_color_for_paragraph_exact'), text: z.string(), - color: z.string(), + color: hexColor, });src/handlers/filesystem-handlers.ts (2)
414-426: Dynamic import is fine but lacks timeout protection.
handleReadDocxusesawait import(...)which is acceptable for code-splitting. However, unlikehandleReadFile(which wraps inwithTimeout), this handler has no timeout. For large DOCX files,readDocxOutlinecould be slow.
435-439:parsed.ops as anybypasses type safety.
WriteDocxArgsSchemavalidatesopsthrough Zod, but the inferred type may not align with theDocxOp[]expected bywriteDocxPatched. Consider narrowing the type explicitly instead of casting toany.♻️ Suggested improvement
- const result = await writeDocxPatched( - parsed.inputPath, - parsed.outputPath, - parsed.ops as any - ); + const result = await writeDocxPatched( + parsed.inputPath, + parsed.outputPath, + parsed.ops as DocxOp[] + );This requires importing
DocxOpfrom the types module, but preserves intent while being more explicit.src/tools/docx/modify.ts (2)
112-150: Unused temp files — dead code inreplaceBodyXml.
tempDocxPathandtempXmlPathare written (lines 118, 121) but never read back. They appear to be leftover backup/debug artifacts. They add unnecessary I/O and temp file management overhead.Also, line 128 (
documentElement.firstChild as Element) is fragile — ifnewBodyXmlhas leading whitespace,firstChildwould be a text node and the cast silently produces an element withnodeName === '#text', caught only by the subsequent check. Consider using a more robust child selection.♻️ Proposed cleanup
export async function replaceBodyXml( inputPath: string, outputPath: string, newBodyXml: string, ): Promise<void> { - const tempDir = os.tmpdir(); - const tempDocxPath = path.join(tempDir, `docx_temp_${Date.now()}_${Math.random().toString(36).substring(7)}.docx`); - const tempXmlPath = path.join(tempDir, `docx_dom_${Date.now()}_${Math.random().toString(36).substring(7)}.xml`); - - try { - const inputBuf = await fs.readFile(inputPath); - await fs.writeFile(tempDocxPath, inputBuf); - - const { zip, dom, body } = parseDocument(inputBuf); - await fs.writeFile(tempXmlPath, zip.file('word/document.xml')!.asText()); - - // Parse the new body XML - const newBodyDom = new DOMParser().parseFromString( - `<root>${newBodyXml}</root>`, - 'application/xml', - ); - const newBodyElement = newBodyDom.documentElement.firstChild as Element; - if (!newBodyElement || newBodyElement.nodeName !== 'w:body') { - throw new Error('Invalid body XML: must start with <w:body>'); - } + const inputBuf = await fs.readFile(inputPath); + const { zip, dom, body } = parseDocument(inputBuf); + + // Parse the new body XML + const newBodyDom = new DOMParser().parseFromString( + `<root>${newBodyXml}</root>`, + 'application/xml', + ); + const children = nodeListToArray(newBodyDom.documentElement.childNodes); + const newBodyElement = children.find( + (n) => n.nodeType === 1 && (n as Element).nodeName === 'w:body', + ) as Element | undefined; + if (!newBodyElement) { + throw new Error('Invalid body XML: must contain <w:body>'); + }This removes the unused temp files and makes the child lookup robust to whitespace text nodes.
161-225:writeDocxcreates a minimal DOCX — considerxml:space="preserve"on<w:t>.The generated
<w:t>elements (line 188) don't includexml:space="preserve", which means leading/trailing spaces in paragraph text will be stripped by conforming OOXML readers.♻️ Suggested fix
- (para) => ` <w:p> + (para) => ` <w:p> <w:r> - <w:t>${escaped(para)}</w:t> + <w:t xml:space="preserve">${escaped(para)}</w:t> </w:r> </w:p>`,src/utils/files/docx.ts (1)
9-9: Unused import:writeDocx.
writeDocxis imported but never used in this file — thewritemethod throws unconditionally. Remove the import to avoid confusion.♻️ Proposed fix
-import { readDocx, getDocxMetadata, modifyDocxContent, writeDocx } from '../../tools/docx/index.js'; +import { readDocx, getDocxMetadata, modifyDocxContent } from '../../tools/docx/index.js';src/tools/docx/read.ts (2)
183-224:readDocxreads the DOCX file from disk 3 times.
loadDocx(path)is called at line 192, thengetDocxMetadata(path)at line 220 internally calls bothloadDocx(path)andextractParagraphs(path)(which callsloadDocx(path)again). For a large DOCX this triples I/O. Additionally,extractParagraphsre-extracts the same paragraph list thatreadDocxalready computed asallParagraphs.Consider refactoring to pass the already-loaded zip and parsed paragraphs into a metadata builder rather than re-reading from disk.
86-132: Significant duplication acrossextractTextFromDocx,extractParagraphs, andreadDocx.All three functions repeat the same pattern: load zip → get
word/document.xml→ parse DOM → get body → iteratew:pchildren. Consider extracting a shared internal helper that returns the parsed body (or a loaded{ zip, body }tuple) to DRY up the code.
| "@opendocsg/pdf2md": "^0.2.2", | ||
| "@supabase/supabase-js": "^2.89.0", | ||
| "@vscode/ripgrep": "^1.15.9", | ||
| "@xmldom/xmldom": "^0.8.11", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
npm view `@xmldom/xmldom` versions --json | tail -20Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 357
🏁 Script executed:
npm view pizzip versions --json | tail -20Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 265
🏁 Script executed:
npm view `@xmldom/xmldom`@0.8.11 --json | jq '{version: .version, published: .time, license: .license}'Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 2200
🏁 Script executed:
npm view pizzip@3.2.0 --json | jq '{version: .version, published: .time, license: .license}'Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 938
🌐 Web query:
@xmldom/xmldom security vulnerabilities CVE
💡 Result:
Known @xmldom/xmldom security CVEs (with affected & fixed versions):
| CVE | Issue | Affected versions (@xmldom/xmldom) |
Patched / fixed in |
|---|---|---|---|
| CVE-2021-32796 | Improper escaping during XML serialization can cause unexpected syntactic changes | < 0.7.0 (xmldom ≤ 0.6.0) | 0.7.0 (and later) [1] |
| CVE-2022-37616 | Prototype pollution in copy() (maintainer disputes/considers report invalid, but CVE exists) |
< 0.7.6, >= 0.8.0 < 0.8.3, = 0.9.0-beta.1 | 0.7.6, 0.8.3, >= 0.9.0-beta.2 [2][3] |
| CVE-2022-39353 | Parser accepts XML with multiple top-level elements (multiple roots) without error | < 0.7.7, >= 0.8.0 < 0.8.4, >= 0.9.0-beta.1 < 0.9.0-beta.4 | 0.7.7, 0.8.4, >= 0.9.0-beta.4 [4][5] |
Practical guidance: upgrade to a version that is at least the “patched/fixed in” version for each CVE above (ideally a version that satisfies all three). [1][3][5]
Sources: NVD CVE records and GitHub Security Advisories for xmldom/xmldom. [1][2][3][4][5]
🌐 Web query:
pizzip security vulnerabilities CVE
💡 Result:
As of February 10, 2026, there are no publicly listed CVEs / direct security advisories for the npm package pizzip in major vulnerability databases (e.g., Snyk reports “No direct vulnerabilities” for the latest release). [1]
Notes:
pizzipis the synchronous ZIP library from open-xml-templating/pizzip. [2]- Even if
pizzipitself has no CVE, your app can still be affected by indirect vulnerabilities in its dependency chain (Snyk notes this distinction). [1]
Sources
- Snyk vulnerability page for
pizzip(no known direct issues): (security.snyk.io) pizzipupstream repository (project identity/ownership): (github.com)
Citations:
- 1: https://security.snyk.io/package/npm/pizzip?utm_source=openai
- 2: https://github.com/open-xml-templating/pizzip?utm_source=openai
Consider upgrading @xmldom/xmldom from 0.8.11 to the latest stable 0.9.8.
@xmldom/xmldom 0.8.11 is free of known vulnerabilities (all relevant CVEs are patched), but the latest stable release is 0.9.8 (Feb 2025). Since the caret constraint allows 0.9.x versions, upgrading is recommended for access to recent improvements and fixes. pizzip 3.2.0 is already the latest stable version and has no known vulnerabilities.
🤖 Prompt for AI Agents
In `@package.json` at line 88, Update the package.json dependency "@xmldom/xmldom"
from "^0.8.11" to "^0.9.8" to pick up the latest stable release, then run your
package manager (npm install or yarn install) to update node_modules and the
lockfile (package-lock.json or yarn.lock); after updating, run the test suite
and any build/CI checks to ensure compatibility and commit the updated
package.json and lockfile.
| export function parseXml(xmlStr: string): Document { | ||
| return new DOMParser().parseFromString(xmlStr, 'application/xml'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat package.json | grep -A 5 -B 5 xmldomRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 424
🌐 Web query:
@xmldom/xmldom DOMParser errorHandler parse errors behavior
💡 Result:
What DOMParser “parse errors” do in @xmldom/xmldom
1) Legacy behavior (errorHandler) — mostly reporting, not stopping
Historically (and still supported for compatibility), DOMParser accepts a non‑standard errorHandler option that can be either:
- an object with
warning,error,fatalErrorcallbacks, or - a function
(level, msg) => ...(callback model).
The parser invokes these callbacks when it encounters issues, but parsing often continues and aDocumentmay still be returned, depending on what happened. (npmjs.com)
2) Current behavior (0.9.x) — onError + consistent ParseError throwing
In @xmldom/xmldom 0.9.0+, error handling was redesigned to be more consistent. The preferred option is now onError(level, message, context), where level is one of warning | error | fatalError. (tessl.io)
Key change: fatal parsing problems now throw a ParseError (and stop parsing), instead of quietly producing a broken/incomplete Document. (dev.to)
Also: errorHandler still exists but is deprecated in favor of onError. (tessl.io)
3) How to make parsing “strict” (fail fast)
@xmldom/xmldom provides predefined handlers you can plug into onError:
onErrorStopParsing: throw onerrorandfatalErroronWarningStopParsing: throw onwarningtoo (strictest)
(tessl.io)
import { DOMParser, onErrorStopParsing, onWarningStopParsing } from '@xmldom/xmldom';
// fail on any error (error or fatalError)
const strict = new DOMParser({ onError: onErrorStopParsing });
// fail even on warnings
const veryStrict = new DOMParser({ onError: onWarningStopParsing });If you tell me which @xmldom/xmldom version you’re on and what input you’re parsing (text/xml vs text/html), I can describe the exact outcome you should expect for your specific parse error.
Citations:
- 1: https://www.npmjs.com/package/%40xmldom/xmldom?utm_source=openai
- 2: https://tessl.io/registry/tessl/npm-xmldom--xmldom/0.9.0/docs/parsing.md?utm_source=openai
- 3: https://dev.to/karfau/release-090-of-xmldomxmldom-4106?utm_source=openai
- 4: https://tessl.io/registry/tessl/npm-xmldom--xmldom/0.9.0/docs/parsing.md?utm_source=openai
- 5: https://tessl.io/registry/tessl/npm-xmldom--xmldom/0.9.0/docs/constants-utilities.md?utm_source=openai
🏁 Script executed:
rg "parseXml" src/ -A 3 -B 1Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1016
🏁 Script executed:
# Check if there's any error handling or validation downstream
rg "parseError|ParseError|Error.*parse|parse.*error" src/tools/docx/ -i
# Check if document is validated after parsing
rg "getBody|getDocumentXml" src/tools/docx/ -A 5 -B 2 | head -60
# Look at the full write.ts usage context
sed -n '1,50p' src/tools/docx/write.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 4765
🌐 Web query:
@xmldom/xmldom 0.8.11 DOMParser error handling behavior default
💡 Result:
In @xmldom/xmldom 0.8.11, if you call:
new DOMParser().parseFromString(xml, 'text/xml')and you don’t pass an options.errorHandler, the parser uses the built-in DOMHandler as the default error handler (because the default domBuilder is a DOMHandler). Default behavior is: warnings go to console.warn, errors go to console.error, and fatalError throws a ParseError (which stops parsing). The default options also include a locator:{} so messages include line/column info. [1]
parseXml does not handle non-fatal XML parse errors from @xmldom/xmldom.
In @xmldom/xmldom 0.8.11, non-fatal parse errors are logged to console and parsing continues, potentially producing an incomplete DOM. While fatal errors do throw, recoverable parse errors would silently slip through and cause cryptic failures downstream (e.g., getBody() throwing unexpectedly if the body element was not fully parsed).
Consider adding error detection to catch parse issues early:
Proposed fix
export function parseXml(xmlStr: string): Document {
- return new DOMParser().parseFromString(xmlStr, 'application/xml');
+ const errors: string[] = [];
+ const doc = new DOMParser({
+ errorHandler: {
+ warning: () => {},
+ error: (msg: string) => errors.push(msg),
+ fatalError: (msg: string) => errors.push(msg),
+ },
+ }).parseFromString(xmlStr, 'application/xml');
+ if (errors.length > 0) {
+ throw new Error(`XML parse error: ${errors[0]}`);
+ }
+ return doc;
}🤖 Prompt for AI Agents
In `@src/tools/docx/dom.ts` around lines 17 - 19, The parseXml function currently
calls new DOMParser().parseFromString(...) which lets `@xmldom/xmldom` emit
non-fatal parse errors to console and return a partial DOM; update parseXml to
instantiate DOMParser with an errorHandler option (capture warnings/errors)
and/or inspect the returned Document for parsererror nodes, collect any
non-fatal messages, and throw a descriptive Error when any parse issues are
detected so callers (e.g., getBody()) fail fast; refer to the parseXml function
and the DOMParser from `@xmldom/xmldom` when making this change.
| if (idx < 0 || idx > paragraphs.length) return; | ||
|
|
||
| if (idx === paragraphs.length) { | ||
| body.appendChild(newP); | ||
| } else { | ||
| let current = 0; | ||
| for (const child of nodeListToArray(body.childNodes)) { | ||
| if (child.nodeType !== 1 || (child as Element).nodeName !== 'w:p') continue; | ||
| if (current === idx) { | ||
| body.insertBefore(newP, child); | ||
| break; | ||
| } | ||
| current++; | ||
| } | ||
| } |
There was a problem hiding this comment.
Inserting at end may place paragraph after w:sectPr.
Line 273 uses body.appendChild(newP) when idx === paragraphs.length. In standard DOCX, w:body's last child is typically w:sectPr (section properties). Appending after it can produce a malformed document. The paragraph should be inserted before w:sectPr if it exists.
🐛 Proposed fix
if (idx === paragraphs.length) {
- body.appendChild(newP);
+ // Insert before w:sectPr if present (must remain last)
+ const lastChild = body.lastChild;
+ if (lastChild && (lastChild as Element).nodeName === 'w:sectPr') {
+ body.insertBefore(newP, lastChild);
+ } else {
+ body.appendChild(newP);
+ }
} else {📝 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 (idx < 0 || idx > paragraphs.length) return; | |
| if (idx === paragraphs.length) { | |
| body.appendChild(newP); | |
| } else { | |
| let current = 0; | |
| for (const child of nodeListToArray(body.childNodes)) { | |
| if (child.nodeType !== 1 || (child as Element).nodeName !== 'w:p') continue; | |
| if (current === idx) { | |
| body.insertBefore(newP, child); | |
| break; | |
| } | |
| current++; | |
| } | |
| } | |
| if (idx < 0 || idx > paragraphs.length) return; | |
| if (idx === paragraphs.length) { | |
| // Insert before w:sectPr if present (must remain last) | |
| const lastChild = body.lastChild; | |
| if (lastChild && (lastChild as Element).nodeName === 'w:sectPr') { | |
| body.insertBefore(newP, lastChild); | |
| } else { | |
| body.appendChild(newP); | |
| } | |
| } else { | |
| let current = 0; | |
| for (const child of nodeListToArray(body.childNodes)) { | |
| if (child.nodeType !== 1 || (child as Element).nodeName !== 'w:p') continue; | |
| if (current === idx) { | |
| body.insertBefore(newP, child); | |
| break; | |
| } | |
| current++; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/tools/docx/modify.ts` around lines 270 - 284, The current branch that
handles idx === paragraphs.length uses body.appendChild(newP) which can append
after a w:sectPr and produce a malformed DOCX; instead, detect if body contains
a final w:sectPr element and insert newP before that element (fall back to
appendChild only if no w:sectPr exists). Update the insertion logic in the block
around body.appendChild/newP (references: body, newP, paragraphs,
nodeListToArray) to search body.childNodes for an Element with nodeName
'w:sectPr' and call body.insertBefore(newP, sectPrNode) when found.
| if (typeof content === 'string') { | ||
| // Check if content is body XML (starts with <w:body) | ||
| const trimmedContent = content.trim(); | ||
| if (trimmedContent.startsWith('<w:body')) { | ||
| // --- BODY XML REPLACEMENT MODE --- | ||
| // Replace body XML in existing DOCX (preserves all styles and other files) | ||
| const targetPath = outputPath ? await validatePath(outputPath) : validPath; | ||
|
|
||
| // Check if source file exists | ||
| try { | ||
| await fs.access(validPath); | ||
| } catch { | ||
| throw new Error(`Source DOCX file does not exist: ${validPath}. Cannot replace body XML in non-existent file.`); | ||
| } | ||
|
|
||
| // Replace body XML (preserves all styles and formatting) | ||
| await replaceBodyXml(validPath, targetPath, trimmedContent); | ||
| } else { | ||
| // --- DOCX CREATION MODE --- | ||
| // Create new DOCX from text (minimal structure, no styles) | ||
| const targetPath = outputPath ? await validatePath(outputPath) : validPath; | ||
| await writeDocxImpl(targetPath, content); | ||
| } |
There was a problem hiding this comment.
Content-sniffing heuristic (<w:body) is fragile and undocumented at the API boundary.
The startsWith('<w:body') check on Line 961 overloads the content: string parameter with two distinct semantics (plain text vs. body XML). If a user ever passes a plain-text string that happens to begin with <w:body, it would silently take the XML replacement path and likely corrupt the document.
Consider either:
- Using a separate parameter/flag (e.g.,
mode: 'create' | 'replaceBody' | 'modify') to make intent explicit, or - Documenting this convention prominently in the schema/tool description so callers are aware.
🤖 Prompt for AI Agents
In `@src/tools/filesystem.ts` around lines 958 - 980, The code currently uses a
fragile content-sniffing heuristic (content.trim().startsWith('<w:body')) to
decide between creating a DOCX and calling replaceBodyXml, which can
misinterpret plain text that begins with "<w:body"; change the API to make
intent explicit by adding an explicit mode/flag (e.g., mode: 'create' |
'replaceBody') to the function that accepts content and use that flag to choose
between calling replaceBodyXml(validPath, targetPath, trimmedContent) and
writeDocxImpl(targetPath, content) (maintaining validatePath usage), or
alternatively perform robust XML validation (parse trimmedContent and verify it
is well-formed XML with a w:body root) before taking the replaceBodyXml path;
update all callers to pass the new mode or ensure content is validated
accordingly.
| const ReplaceParagraphAtBodyIndexOpSchema = z.object({ | ||
| type: z.literal('replace_paragraph_at_body_index'), | ||
| bodyChildIndex: z.number(), | ||
| to: z.string(), | ||
| }); |
There was a problem hiding this comment.
bodyChildIndex should be constrained to a non-negative integer.
z.number() allows fractional values (e.g., 1.5). The runtime bounds check in replace-paragraph-at-body-index.ts (Line 18) would pass for a fractional index within range, but children[1.5] returns undefined in JS, causing a TypeError on child.nodeName.
Proposed fix
const ReplaceParagraphAtBodyIndexOpSchema = z.object({
type: z.literal('replace_paragraph_at_body_index'),
- bodyChildIndex: z.number(),
+ bodyChildIndex: z.number().int().min(0),
to: z.string(),
});🤖 Prompt for AI Agents
In `@src/tools/schemas.ts` around lines 118 - 122, The
ReplaceParagraphAtBodyIndexOpSchema currently uses bodyChildIndex: z.number(),
which permits fractional and negative values causing runtime TypeError in
replace-paragraph-at-body-index.ts; update the schema to enforce a non-negative
integer (e.g., use z.number().int().nonnegative() or equivalent) for the
bodyChildIndex field so invalid fractional/negative indices are rejected at
parse time; keep the existing runtime bounds check in
replace-paragraph-at-body-index.ts for upper-bound validation.
|
|
||
| /** File type classification */ | ||
| fileType: 'text' | 'excel' | 'image' | 'binary'; | ||
| fileType: 'text' | 'excel' | 'image' | 'binary' | 'docx'; |
There was a problem hiding this comment.
'docx' added to fileType union but DocxFileHandler.getInfo() returns 'binary'.
The fileType union now includes 'docx', but looking at src/utils/files/docx.ts (Line 155 in the relevant snippet), DocxFileHandler.getInfo() returns fileType: 'binary' instead of fileType: 'docx'. This means the new union variant is never actually used.
Fix in src/utils/files/docx.ts
return {
size: stats.size,
created: stats.birthtime,
modified: stats.mtime,
accessed: stats.atime,
isDirectory: false,
isFile: true,
permissions: (stats.mode & 0o777).toString(8),
- fileType: 'binary',
+ fileType: 'docx',
metadata
};📝 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.
| fileType: 'text' | 'excel' | 'image' | 'binary' | 'docx'; | |
| return { | |
| size: stats.size, | |
| created: stats.birthtime, | |
| modified: stats.mtime, | |
| accessed: stats.atime, | |
| isDirectory: false, | |
| isFile: true, | |
| permissions: (stats.mode & 0o777).toString(8), | |
| fileType: 'docx', | |
| metadata | |
| }; |
🤖 Prompt for AI Agents
In `@src/utils/files/base.ts` at line 233, DocxFileHandler.getInfo currently
returns fileType: 'binary' which contradicts the updated fileType union that
added 'docx'; update the DocxFileHandler.getInfo method to return fileType:
'docx' (instead of 'binary') and ensure any related metadata or callers
expecting 'binary' are adjusted if needed so the new 'docx' variant is used
consistently (reference: DocxFileHandler.getInfo and the fileType union in
base.ts).
| if (Array.isArray(content)) { | ||
| modifications = content; | ||
| } else if (typeof content === 'string') { | ||
| // Try to parse as JSON | ||
| try { | ||
| modifications = JSON.parse(content); | ||
| } catch { | ||
| // If not JSON, treat as single replace operation | ||
| modifications = [{ | ||
| type: 'replace', | ||
| findText: range, | ||
| replaceText: content | ||
| }]; | ||
| } | ||
| } |
There was a problem hiding this comment.
JSON.parse result not validated as an array.
If a user passes a JSON string that parses to a non-array (e.g., "{}" or "42"), it's assigned directly to modifications: DocxModification[] without validation. This would cause modifyDocxContent to fail in unexpected ways or silently do nothing.
🛡️ Proposed fix — validate parsed JSON is an array
try {
- modifications = JSON.parse(content);
+ const parsed = JSON.parse(content);
+ if (!Array.isArray(parsed)) {
+ throw new Error('not an array');
+ }
+ modifications = parsed;
} 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 (Array.isArray(content)) { | |
| modifications = content; | |
| } else if (typeof content === 'string') { | |
| // Try to parse as JSON | |
| try { | |
| modifications = JSON.parse(content); | |
| } catch { | |
| // If not JSON, treat as single replace operation | |
| modifications = [{ | |
| type: 'replace', | |
| findText: range, | |
| replaceText: content | |
| }]; | |
| } | |
| } | |
| if (Array.isArray(content)) { | |
| modifications = content; | |
| } else if (typeof content === 'string') { | |
| // Try to parse as JSON | |
| try { | |
| const parsed = JSON.parse(content); | |
| if (!Array.isArray(parsed)) { | |
| throw new Error('not an array'); | |
| } | |
| modifications = parsed; | |
| } catch { | |
| // If not JSON, treat as single replace operation | |
| modifications = [{ | |
| type: 'replace', | |
| findText: range, | |
| replaceText: content | |
| }]; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/utils/files/docx.ts` around lines 93 - 107, The parsed JSON result is not
validated and may be a non-array value assigned to modifications; update the
logic in the function handling `content` (the branch that calls JSON.parse) so
that after parsing you check Array.isArray(parsed) and only assign to
`modifications` when true—otherwise fall back to treating the input as a single
replace operation (as the existing string fallback does) or throw a clear error;
reference the `content` variable and the `modifications` array so you locate the
JSON.parse branch and ensure `modifyDocxContent` receives an actual array of
DocxModification objects.
| return { | ||
| size: stats.size, | ||
| created: stats.birthtime, | ||
| modified: stats.mtime, | ||
| accessed: stats.atime, | ||
| isDirectory: false, | ||
| isFile: true, | ||
| permissions: (stats.mode & 0o777).toString(8), | ||
| fileType: 'binary', | ||
| metadata | ||
| }; |
There was a problem hiding this comment.
fileType should be 'docx' instead of 'binary'.
The FileInfo interface (from base.ts) supports 'docx' as a fileType value. Returning 'binary' misclassifies the file and may affect downstream consumers that check fileType.
🐛 Proposed fix
- fileType: 'binary',
+ fileType: 'docx',📝 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.
| return { | |
| size: stats.size, | |
| created: stats.birthtime, | |
| modified: stats.mtime, | |
| accessed: stats.atime, | |
| isDirectory: false, | |
| isFile: true, | |
| permissions: (stats.mode & 0o777).toString(8), | |
| fileType: 'binary', | |
| metadata | |
| }; | |
| return { | |
| size: stats.size, | |
| created: stats.birthtime, | |
| modified: stats.mtime, | |
| accessed: stats.atime, | |
| isDirectory: false, | |
| isFile: true, | |
| permissions: (stats.mode & 0o777).toString(8), | |
| fileType: 'docx', | |
| metadata | |
| }; |
🤖 Prompt for AI Agents
In `@src/utils/files/docx.ts` around lines 149 - 159, The returned FileInfo object
in the docx processor currently sets fileType to 'binary'; update it to 'docx'
so consumers can correctly detect DOCX files—locate the return object in the
function in src/utils/files/docx.ts (the block that returns
size/created/modified/accessed/isDirectory/isFile/permissions/fileType/metadata)
and change fileType: 'binary' to fileType: 'docx'.
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@src/tools/docx/builders/image.ts`:
- Around line 85-112: The buildImageElement function currently hardcodes
wp:docPr id="1" and pic:cNvPr id="0", causing duplicate IDs when multiple images
are inserted; update buildImageElement to accept or compute unique numeric IDs
(e.g., add an optional parameter imageId or pull from a shared monotonically
incrementing counter stored on the document builder) and use that ID for both
the wp:docPr/@id and pic:cNvPr/@id attributes (ensure any callers that create
images supply or obtain the next ID so IDs are globally unique across the
document).
- Around line 78-112: The buildDrawingXml function is duplicated; extract it to
a shared module (e.g., create a new module exporting buildDrawingXml) and update
both callers to import that single export; specifically, move the
buildDrawingXml implementation (the function signature and its helpers like
escapeXmlAttr and NAMESPACES usage) into the new module and replace the local
definitions in src/tools/docx/builders/image.ts and
src/tools/docx/ops/insert-image-after-text.ts with imports of buildDrawingXml so
the hardcoded-ID fix and XML generation logic are maintained in one place.
In `@src/tools/docx/builders/table.ts`:
- Around line 87-95: The header row generation in the table builder can create a
different number of header cells than the table's colCount (which may be derived
from spec.rows[0].length), causing grid/row mismatches; inside the block that
builds headers (referencing spec.headers, buildCell, tbl, tr, and spec.rows)
validate and normalize header length to match the computed colCount: compute
colCount consistently (preferably derive once from spec.colCount ||
spec.headers?.length || spec.rows?.[0]?.length), then when spec.headers exists,
trim or pad the header array to colCount (pad with empty strings) and only
iterate up to colCount when calling buildCell; ensure colWidths indexing also
uses that same colCount to avoid out-of-bounds lookups.
- Around line 97-105: The data row loop in src/tools/docx/builders/table.ts may
produce ragged rows because it iterates by row.length rather than the table's
expected column count; update the block that builds rows (the loop referencing
spec.rows, buildCell, spec.colWidths and tbl) to ensure every w:tr has exactly
the same number of w:tc as the table grid by computing the expected colCount
(from headers or first row) and either pad short rows with empty cells via
buildCell('', false, width) for missing columns (using spec.colWidths[i] where
available) or throw/validate when a row is longer than colCount so the table
grid remains consistent.
In `@src/tools/docx/create.ts`:
- Around line 418-427: The stats calculation uses bodyChildCount =
content.items.length which can be wrong if buildImageElement or other builders
insert multiple nodes or errors occur; instead compute bodyChildrenAfter from
the assembled document body itself (e.g., call getBodyChildren or similar
utility) and set stats.bodyChildrenAfter to that real count; update the stats
object creation near the WriteDocxStats assignment (replace bodyChildCount
usage) and ensure any references to buildImageElement or content.items remain
unchanged except for relying on the actual DOM children count.
- Around line 58-64: The created document.xml.rels currently writes an empty
<Relationships> element; update the code that writes 'document.xml.rels' (the
zip.folder('word')?.folder('_rels')?.file(...) call that produces
document.xml.rels) to include Relationship entries for the parts you add to the
package: add relationships for styles.xml, settings.xml, webSettings.xml, and
fontTable.xml with unique Ids (e.g., rIdStyles, rIdSettings, rIdWebSettings,
rIdFontTable), the correct Type URIs per OOXML (styles, settings, webSettings,
fonts) and Target attributes pointing to their respective files (e.g.,
'styles.xml', 'settings.xml', 'webSettings.xml', 'fontTable.xml') so Word can
discover and apply those parts.
In `@src/tools/docx/ops/header-replace-text-exact.ts`:
- Around line 26-29: Remove the dead guard and unused variable around
zip.folder('word'): the call zip.folder('word') always returns a PizZip instance
so the check if (!files) and the early return ('no_word_folder') must be
deleted; instead rely on the existing discovery logic that iterates
Object.keys(zip.files) and filters by /^word\/header[0-9]+\.xml$/ in the
headerReplaceTextExact flow, and remove the unused files binding to keep the
function concise.
In `@src/tools/docx/ops/insert-image-after-text.ts`:
- Around line 40-46: The current anchor validation in insert-image-after-text.ts
silently prefers `before` when both `op.before` and `op.after` are set; update
the validation to detect the ambiguous case and return a skipped result instead
of proceeding. Specifically, in the block that computes `anchorText` and
`position` (using `op.before ?? op.after` and `op.before ? 'before' : 'after'`),
first check if both `op.before` and `op.after` are truthy and if so return `{
op, status: 'skipped', matched: 0, reason: 'ambiguous_anchor: provide exactly
one of "after" or "before"' }`; otherwise continue to set `anchorText` and
`position` as before. Ensure you reference `op.before`, `op.after`,
`anchorText`, and `position` when making the change.
In `@src/tools/docx/ops/insert-paragraph-after-text.ts`:
- Around line 22-43: The empty-document branch in insert-paragraph-after-text.ts
currently returns { op, status: 'applied', matched: 0 } which conflicts with the
convention that matched: 0 indicates skipped; update the return to { op, status:
'applied', matched: 1, reason: 'empty_document_append' } (or otherwise set
matched to a non-zero value) so the creation of the paragraph is unambiguously
reported by the function that uses op, body, children and newP/newR/newT.
In `@src/tools/schemas.ts`:
- Around line 241-257: The refinement on WriteDocxArgsSchema currently only
enforces presence of ops for updates but doesn't forbid content when inputPath
is present; update the refine predicate for WriteDocxArgsSchema so that: when
inputPath is absent require content exists and ops must be undefined/absent, and
when inputPath is present require ops exists and content must be
undefined/absent; also adjust the refinement error message to reflect both
presence and mutual exclusion rules (mention inputPath, content, and ops) to
make the contract explicit.
- Around line 174-192: The schemas InsertTableOpSchema and InsertImageOpSchema
currently allow both after and before to be provided, which contradicts the
JSDoc; update each schema to enforce mutual exclusivity by adding a Zod
refinement (e.g., .refine(obj => !(obj.after && obj.before), { message: 'Provide
either "after" or "before", not both', path: ['after','before'] })) to validate
that both fields are not present at the same time for InsertTableOpSchema and
InsertImageOpSchema.
🧹 Nitpick comments (9)
src/tools/docx/relationships.ts (1)
11-12: Nit: Consolidate the two imports from./constants.jsinto one.Suggested fix
-import { getMimeType } from './constants.js'; -import { DOCX_PATHS, NAMESPACES } from './constants.js'; +import { getMimeType, DOCX_PATHS, NAMESPACES } from './constants.js';src/tools/docx/ops/insert-image-after-text.ts (1)
35-39: Synchronousfsusage blocks the event loop and is inconsistent withbuilders/image.ts.This file uses
fs.existsSync(Line 50) andfs.readFileSync(Line 67) while the correspondingbuildImageElementinbuilders/image.tsusesfs/promises(async). If this function is ever called from an async context, the sync reads will block. Consider makingapplyInsertImageasync (returningPromise<OpResult>) to align with the rest of the image-handling code. This would also require updating the op dispatcher.Also applies to: 23-23
src/tools/docx/ops/insert-paragraph-after-text.ts (1)
27-40: Paragraph-building logic is duplicated between the empty-doc and normal-match branches.The
w:p+ optionalw:pPr/w:pStyle+w:r/w:tconstruction at lines 27–40 is nearly identical to lines 55–71. Extracting a small helper (e.g.,buildParagraph(doc, text, style?)) would reduce duplication and make future changes less error-prone.Also applies to: 55-71
src/tools/docx/ops/insert-table-after-text.ts (1)
30-31: Mutual exclusivity ofbefore/afteris not enforced in this function.The type contract says exactly one of
beforeoraftermust be provided. If both are supplied,anchorTextsilently usesop.before(via??) whilepositionuses the truthiness ofop.before. This works by accident but relies entirely on upstream schema validation. Consider adding an explicit guard:Proposed defensive guard
+ if (op.before && op.after) { + return { op, status: 'skipped', matched: 0, reason: 'provide_only_one_of_before_or_after' }; + } + const anchorText = op.before ?? op.after; const position: 'before' | 'after' = op.before ? 'before' : 'after';src/tools/docx/index.ts (1)
10-36: Two separate type export blocks from the same module with identical// Typescomments.Both blocks export from
./types.jswith the same// Typescomment. Consider either merging them into a single block or differentiating the comments (e.g.,// Content typesvs// Operational types).src/tools/docx/create.ts (2)
103-275: Heading styles 4–9 are nearly identical — consider generating them programmatically.Headings 4–9 share the same font size (22), color (
1F3763), and formatting — differing only inoutlineLvl. This is ~170 lines of repetitive XML. Generating them in a loop would reduce maintenance burden and risk of copy-paste drift.That said, since this is a static template and correctness is straightforward, this is purely a maintainability suggestion.
396-410: Content items with unrecognizedtypevalues are silently ignored.If
content.itemscontains an item with atypeother than'paragraph','table', or'image', it's skipped without any warning. This could lead to confusing results where the user provides content that doesn't appear in the output.Consider adding a warning or throwing for unknown item types:
Proposed enhancement
} else if (item.type === 'image') { const imgP = await buildImageElement(doc, zip, item); body.appendChild(imgP); + } else { + // Type system should prevent this, but guard against runtime surprises + throw new Error(`Unknown content item type: ${(item as any).type}`); }src/handlers/filesystem-handlers.ts (2)
442-455:as anycasts bypass type safety onopsandcontent.Lines 445 and 454 cast the Zod-parsed output to
any, losing the type contract between the schema and the downstream functions (writeDocxPatched,createDocxNew). If the schema drifts from theDocxOp[]/DocxContentStructureinterfaces, the mismatch won't be caught at compile time.Consider using Zod's inferred types or explicit type assertions to maintain safety:
Suggested approach
- result = await writeDocxPatched( - parsed.inputPath, - parsed.outputPath, - parsed.ops as any - ); + result = await writeDocxPatched( + parsed.inputPath, + parsed.outputPath, + parsed.ops as DocxOp[] + );- result = await createDocxNew( - parsed.outputPath, - parsed.content as any - ); + result = await createDocxNew( + parsed.outputPath, + parsed.content as DocxContentStructure + );This requires importing
DocxOpandDocxContentStructuretypes, but preserves compile-time checking.
111-133: DOCX handling inread_fileis functional but the response text is a single very long template literal.Line 129 builds the entire response (XML + metadata + instructions) in one string. This works but is hard to read and maintain. Consider composing it from smaller parts.
Also, the guidance at the end ("To modify this DOCX: 1. Edit the body XML...") references
write_docxbut the tool description forread_file(in server.ts, line 309) already tells users to preferread_docxfor editing workflows. This creates slightly conflicting guidance — one path says "use read_docx + write_docx" while this response says "edit the body XML and use write_docx". This may confuse the LLM.
| function buildDrawingXml( | ||
| rId: string, | ||
| widthEmu: number, | ||
| heightEmu: number, | ||
| altText: string, | ||
| fileName: string, | ||
| ): string { | ||
| return ( | ||
| `<w:drawing xmlns:w="${NAMESPACES.W}">` + | ||
| `<wp:inline distT="0" distB="0" distL="0" distR="0" ` + | ||
| `xmlns:wp="${NAMESPACES.WP}">` + | ||
| `<wp:extent cx="${widthEmu}" cy="${heightEmu}"/>` + | ||
| `<wp:docPr id="1" name="${fileName}" descr="${escapeXmlAttr(altText)}"/>` + | ||
| `<a:graphic xmlns:a="${NAMESPACES.A}">` + | ||
| `<a:graphicData uri="http://schemas.openxmlformats.org/drawingml/2006/picture">` + | ||
| `<pic:pic xmlns:pic="${NAMESPACES.PIC}">` + | ||
| `<pic:nvPicPr>` + | ||
| `<pic:cNvPr id="0" name="${fileName}" descr="${escapeXmlAttr(altText)}"/>` + | ||
| `<pic:cNvPicPr/>` + | ||
| `</pic:nvPicPr>` + | ||
| `<pic:blipFill>` + | ||
| `<a:blip r:embed="${rId}" xmlns:r="${NAMESPACES.R}"/>` + | ||
| `<a:stretch><a:fillRect/></a:stretch>` + | ||
| `</pic:blipFill>` + | ||
| `<pic:spPr>` + | ||
| `<a:xfrm><a:off x="0" y="0"/><a:ext cx="${widthEmu}" cy="${heightEmu}"/></a:xfrm>` + | ||
| `<a:prstGeom prst="rect"><a:avLst/></a:prstGeom>` + | ||
| `</pic:spPr>` + | ||
| `</pic:pic>` + | ||
| `</a:graphicData>` + | ||
| `</a:graphic>` + | ||
| `</wp:inline>` + | ||
| `</w:drawing>` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all occurrences of buildDrawingXml
rg -n 'buildDrawingXml' --type=ts -A 2Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1038
🏁 Script executed:
#!/bin/bash
# Read the buildDrawingXml function from builders/image.ts
echo "=== src/tools/docx/builders/image.ts (lines 78-115) ==="
sed -n '78,115p' src/tools/docx/builders/image.ts
echo ""
echo "=== src/tools/docx/ops/insert-image-after-text.ts (lines 129-165) ==="
sed -n '129,165p' src/tools/docx/ops/insert-image-after-text.ts
# Check for comments around the second function mentioning duplication
echo ""
echo "=== Checking context around line 129 in insert-image-after-text.ts ==="
sed -n '120,130p' src/tools/docx/ops/insert-image-after-text.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 3177
Extract buildDrawingXml to a shared module to eliminate duplication.
The function is duplicated verbatim in both src/tools/docx/builders/image.ts (lines 78–112) and src/tools/docx/ops/insert-image-after-text.ts (lines 129–163). The comment in the latter file already acknowledges this duplication ("shared with builders/image.ts"). Create a shared module (e.g., builders/drawing.ts or builders/utils.ts) to define this function once and import it from both locations. This centralizes the hardcoded-ID fix in one place and reduces maintenance overhead.
🤖 Prompt for AI Agents
In `@src/tools/docx/builders/image.ts` around lines 78 - 112, The buildDrawingXml
function is duplicated; extract it to a shared module (e.g., create a new module
exporting buildDrawingXml) and update both callers to import that single export;
specifically, move the buildDrawingXml implementation (the function signature
and its helpers like escapeXmlAttr and NAMESPACES usage) into the new module and
replace the local definitions in src/tools/docx/builders/image.ts and
src/tools/docx/ops/insert-image-after-text.ts with imports of buildDrawingXml so
the hardcoded-ID fix and XML generation logic are maintained in one place.
| return ( | ||
| `<w:drawing xmlns:w="${NAMESPACES.W}">` + | ||
| `<wp:inline distT="0" distB="0" distL="0" distR="0" ` + | ||
| `xmlns:wp="${NAMESPACES.WP}">` + | ||
| `<wp:extent cx="${widthEmu}" cy="${heightEmu}"/>` + | ||
| `<wp:docPr id="1" name="${fileName}" descr="${escapeXmlAttr(altText)}"/>` + | ||
| `<a:graphic xmlns:a="${NAMESPACES.A}">` + | ||
| `<a:graphicData uri="http://schemas.openxmlformats.org/drawingml/2006/picture">` + | ||
| `<pic:pic xmlns:pic="${NAMESPACES.PIC}">` + | ||
| `<pic:nvPicPr>` + | ||
| `<pic:cNvPr id="0" name="${fileName}" descr="${escapeXmlAttr(altText)}"/>` + | ||
| `<pic:cNvPicPr/>` + | ||
| `</pic:nvPicPr>` + | ||
| `<pic:blipFill>` + | ||
| `<a:blip r:embed="${rId}" xmlns:r="${NAMESPACES.R}"/>` + | ||
| `<a:stretch><a:fillRect/></a:stretch>` + | ||
| `</pic:blipFill>` + | ||
| `<pic:spPr>` + | ||
| `<a:xfrm><a:off x="0" y="0"/><a:ext cx="${widthEmu}" cy="${heightEmu}"/></a:xfrm>` + | ||
| `<a:prstGeom prst="rect"><a:avLst/></a:prstGeom>` + | ||
| `</pic:spPr>` + | ||
| `</pic:pic>` + | ||
| `</a:graphicData>` + | ||
| `</a:graphic>` + | ||
| `</wp:inline>` + | ||
| `</w:drawing>` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Hardcoded wp:docPr id="1" will produce duplicate IDs when multiple images are inserted.
The OOXML spec requires unique id values for wp:docPr across the document. When buildImageElement is called multiple times, every image gets id="1" (Line 90) and pic:cNvPr id="0" (Line 95). While Word is often tolerant, strict validators and other consumers (LibreOffice, accessibility tools) may reject or misrender the document.
Consider accepting or computing a unique ID (e.g., scan existing docPr IDs in the document, or use a monotonically incrementing counter).
🤖 Prompt for AI Agents
In `@src/tools/docx/builders/image.ts` around lines 85 - 112, The
buildImageElement function currently hardcodes wp:docPr id="1" and pic:cNvPr
id="0", causing duplicate IDs when multiple images are inserted; update
buildImageElement to accept or compute unique numeric IDs (e.g., add an optional
parameter imageId or pull from a shared monotonically incrementing counter
stored on the document builder) and use that ID for both the wp:docPr/@id and
pic:cNvPr/@id attributes (ensure any callers that create images supply or obtain
the next ID so IDs are globally unique across the document).
| // Header row | ||
| if (spec.headers && spec.headers.length > 0) { | ||
| const tr = doc.createElement('w:tr'); | ||
| for (let i = 0; i < spec.headers.length; i++) { | ||
| const width = spec.colWidths?.[i]; | ||
| tr.appendChild(buildCell(spec.headers[i], true, width)); | ||
| } | ||
| tbl.appendChild(tr); | ||
| } |
There was a problem hiding this comment.
Header row cell count could also mismatch colCount when derived from data rows.
If spec.headers is absent, colCount comes from spec.rows[0].length. But if spec.headers is present and its length differs from data row lengths, the grid and rows will be inconsistent. The schema doesn't enforce this alignment.
🤖 Prompt for AI Agents
In `@src/tools/docx/builders/table.ts` around lines 87 - 95, The header row
generation in the table builder can create a different number of header cells
than the table's colCount (which may be derived from spec.rows[0].length),
causing grid/row mismatches; inside the block that builds headers (referencing
spec.headers, buildCell, tbl, tr, and spec.rows) validate and normalize header
length to match the computed colCount: compute colCount consistently (preferably
derive once from spec.colCount || spec.headers?.length ||
spec.rows?.[0]?.length), then when spec.headers exists, trim or pad the header
array to colCount (pad with empty strings) and only iterate up to colCount when
calling buildCell; ensure colWidths indexing also uses that same colCount to
avoid out-of-bounds lookups.
| // Data rows | ||
| for (const row of spec.rows) { | ||
| const tr = doc.createElement('w:tr'); | ||
| for (let i = 0; i < row.length; i++) { | ||
| const width = spec.colWidths?.[i]; | ||
| tr.appendChild(buildCell(row[i], false, width)); | ||
| } | ||
| tbl.appendChild(tr); | ||
| } |
There was a problem hiding this comment.
Ragged rows could produce an inconsistent table grid.
Data rows are iterated by their own row.length (line 100), which may differ from colCount (derived from headers or the first row). OOXML expects each w:tr to have the same number of w:tc elements as defined in w:tblGrid. A row with fewer cells could cause rendering issues or corruption in some Word processors.
Consider either padding short rows with empty cells or validating that all rows match the expected column count.
🤖 Prompt for AI Agents
In `@src/tools/docx/builders/table.ts` around lines 97 - 105, The data row loop in
src/tools/docx/builders/table.ts may produce ragged rows because it iterates by
row.length rather than the table's expected column count; update the block that
builds rows (the loop referencing spec.rows, buildCell, spec.colWidths and tbl)
to ensure every w:tr has exactly the same number of w:tc as the table grid by
computing the expected colCount (from headers or first row) and either pad short
rows with empty cells via buildCell('', false, width) for missing columns (using
spec.colWidths[i] where available) or throw/validate when a row is longer than
colCount so the table grid remains consistent.
| // ─── word/_rels/document.xml.rels ──────────────────────────────── | ||
| zip.folder('word')?.folder('_rels')?.file( | ||
| 'document.xml.rels', | ||
| `<?xml version="1.0" encoding="UTF-8" standalone="yes"?> | ||
| <Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships"> | ||
| </Relationships>`, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/tools/docx/create.ts | head -150Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 6323
🌐 Web query:
OOXML Office Open XML document.xml.rels relationships styles.xml required specification
💡 Result:
For a WordprocessingML (.docx) package, the specification treats /word/styles.xml (the Style Definitions part) as the target of an implicit relationship from the Main Document part (/word/document.xml). This relationship is stored in the part-relationship item:
- Relationship part location:
/word/_rels/document.xml.rels(relationships for/word/document.xml) (OPC/OOXML packaging model) [4] - Relationship Type (Transitional):
http://schemas.openxmlformats.org/officeDocument/2006/relationships/styles[1] - Relationship Type (Strict):
http://purl.oclc.org/ooxml/officeDocument/relationships/styles[2] - Typical Target:
styles.xml(i.e.,/word/styles.xml) [1]
Example document.xml.rels entry (as shown in the spec-derived references):
<Relationship Id="rId3"
Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/styles"
Target="styles.xml"/>[1]
Is styles.xml required?
A minimal conformant WordprocessingML package requires only:
/[Content_Types].xml/_rels/.rels(package-level relationship)- a Main Document part (e.g.,
/word/document.xml) targeted by that package-level relationship [3]
So, styles.xml is not required for the minimal package, but if a Style Definitions part is present, it is (by definition) the target of an implicit relationship from the Main Document (and/or Glossary Document) part [2]. Also, the Main Document part is explicitly listed as being permitted to have an implicit relationship to the Style Definitions part [5].
Sources: Style Definitions part + relationship type [1][2], minimal required package structure [3], relationship-part location concept [4], Main Document implicit relationships list [5].
🏁 Script executed:
sed -n '65,250p' src/tools/docx/create.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 4828
🏁 Script executed:
sed -n '250,370p' src/tools/docx/create.ts | head -150Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 4008
🏁 Script executed:
sed -n '290,370p' src/tools/docx/create.ts | grep -A 20 "word/_rels/document.xml.rels" || echo "Already viewed"Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 90
Add missing relationship entries in word/_rels/document.xml.rels for created document parts.
The <Relationships> element is empty, but the OOXML specification requires explicit relationship entries for styles.xml, settings.xml, webSettings.xml, and fontTable.xml since these parts are present in the package. Without these entries, Word may not properly recognize and apply the styles, settings, and font definitions defined in the scaffold.
Proposed fix: add required relationship entries
zip.folder('word')?.folder('_rels')?.file(
'document.xml.rels',
`<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships">
+ <Relationship Id="rId1" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/styles" Target="styles.xml"/>
+ <Relationship Id="rId2" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/settings" Target="settings.xml"/>
+ <Relationship Id="rId3" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/webSettings" Target="webSettings.xml"/>
+ <Relationship Id="rId4" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/fontTable" Target="fontTable.xml"/>
</Relationships>`,
);📝 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.
| // ─── word/_rels/document.xml.rels ──────────────────────────────── | |
| zip.folder('word')?.folder('_rels')?.file( | |
| 'document.xml.rels', | |
| `<?xml version="1.0" encoding="UTF-8" standalone="yes"?> | |
| <Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships"> | |
| </Relationships>`, | |
| ); | |
| // ─── word/_rels/document.xml.rels ──────────────────────────────── | |
| zip.folder('word')?.folder('_rels')?.file( | |
| 'document.xml.rels', | |
| `<?xml version="1.0" encoding="UTF-8" standalone="yes"?> | |
| <Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships"> | |
| <Relationship Id="rId1" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/styles" Target="styles.xml"/> | |
| <Relationship Id="rId2" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/settings" Target="settings.xml"/> | |
| <Relationship Id="rId3" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/webSettings" Target="webSettings.xml"/> | |
| <Relationship Id="rId4" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/fontTable" Target="fontTable.xml"/> | |
| </Relationships>`, | |
| ); |
🤖 Prompt for AI Agents
In `@src/tools/docx/create.ts` around lines 58 - 64, The created document.xml.rels
currently writes an empty <Relationships> element; update the code that writes
'document.xml.rels' (the zip.folder('word')?.folder('_rels')?.file(...) call
that produces document.xml.rels) to include Relationship entries for the parts
you add to the package: add relationships for styles.xml, settings.xml,
webSettings.xml, and fontTable.xml with unique Ids (e.g., rIdStyles,
rIdSettings, rIdWebSettings, rIdFontTable), the correct Type URIs per OOXML
(styles, settings, webSettings, fonts) and Target attributes pointing to their
respective files (e.g., 'styles.xml', 'settings.xml', 'webSettings.xml',
'fontTable.xml') so Word can discover and apply those parts.
| const files = zip.folder('word'); | ||
| if (!files) { | ||
| return { op, status: 'skipped', matched: 0, reason: 'no_word_folder' }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PizZip folder() method return value documentation API
💡 Result:
PizZip#folder(name: string)returns a newPizZipinstance (for chaining) with the created/selected folder as the root. [1]PizZip#folder(regex: RegExp)returns an array of matching folders (empty if none), where each match is aZipObject. [2]
Sources: [1] [2]
🏁 Script executed:
# Check the file and surrounding context
cat -n src/tools/docx/ops/header-replace-text-exact.ts | head -80Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 2971
zip.folder('word') never returns a falsy value — this guard is dead code.
PizZip's folder() method always returns a new PizZip instance regardless of whether the folder exists. The !files check will never be true, making the 'no_word_folder' path unreachable. The files variable itself is also unused afterward — actual discovery happens via Object.keys(zip.files) on line 33. This is harmless since the regex filter on line 34 handles the absence of header files correctly.
Suggested cleanup
- // Iterate over all files in word/ looking for header*.xml
- const files = zip.folder('word');
- if (!files) {
- return { op, status: 'skipped', matched: 0, reason: 'no_word_folder' };
- }
-
// PizZip file listing🤖 Prompt for AI Agents
In `@src/tools/docx/ops/header-replace-text-exact.ts` around lines 26 - 29, Remove
the dead guard and unused variable around zip.folder('word'): the call
zip.folder('word') always returns a PizZip instance so the check if (!files) and
the early return ('no_word_folder') must be deleted; instead rely on the
existing discovery logic that iterates Object.keys(zip.files) and filters by
/^word\/header[0-9]+\.xml$/ in the headerReplaceTextExact flow, and remove the
unused files binding to keep the function concise.
| // ── Validate anchor ───────────────────────────────────────────── | ||
| const anchorText = op.before ?? op.after; | ||
| const position: 'before' | 'after' = op.before ? 'before' : 'after'; | ||
|
|
||
| if (!anchorText) { | ||
| return { op, status: 'skipped', matched: 0, reason: 'no_anchor: provide "after" or "before"' }; | ||
| } |
There was a problem hiding this comment.
No validation when both after and before are provided simultaneously.
The docstring (Line 10) states "Exactly one of after or before must be provided," but the code silently gives before precedence via the ?? operator when both are set. Consider returning a 'skipped' result with a descriptive reason when both anchors are present.
Suggested fix
// ── Validate anchor ─────────────────────────────────────────────
+ if (op.before && op.after) {
+ return { op, status: 'skipped', matched: 0, reason: 'provide only one of "after" or "before", not both' };
+ }
+
const anchorText = op.before ?? op.after;
const position: 'before' | 'after' = op.before ? 'before' : 'after';📝 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.
| // ── Validate anchor ───────────────────────────────────────────── | |
| const anchorText = op.before ?? op.after; | |
| const position: 'before' | 'after' = op.before ? 'before' : 'after'; | |
| if (!anchorText) { | |
| return { op, status: 'skipped', matched: 0, reason: 'no_anchor: provide "after" or "before"' }; | |
| } | |
| // ── Validate anchor ───────────────────────────────────────────── | |
| if (op.before && op.after) { | |
| return { op, status: 'skipped', matched: 0, reason: 'provide only one of "after" or "before", not both' }; | |
| } | |
| const anchorText = op.before ?? op.after; | |
| const position: 'before' | 'after' = op.before ? 'before' : 'after'; | |
| if (!anchorText) { | |
| return { op, status: 'skipped', matched: 0, reason: 'no_anchor: provide "after" or "before"' }; | |
| } |
🤖 Prompt for AI Agents
In `@src/tools/docx/ops/insert-image-after-text.ts` around lines 40 - 46, The
current anchor validation in insert-image-after-text.ts silently prefers
`before` when both `op.before` and `op.after` are set; update the validation to
detect the ambiguous case and return a skipped result instead of proceeding.
Specifically, in the block that computes `anchorText` and `position` (using
`op.before ?? op.after` and `op.before ? 'before' : 'after'`), first check if
both `op.before` and `op.after` are truthy and if so return `{ op, status:
'skipped', matched: 0, reason: 'ambiguous_anchor: provide exactly one of "after"
or "before"' }`; otherwise continue to set `anchorText` and `position` as
before. Ensure you reference `op.before`, `op.after`, `anchorText`, and
`position` when making the change.
| // Special case: if document is empty, append the paragraph | ||
| if (children.length === 0) { | ||
| const doc = body.ownerDocument; | ||
| if (!doc) return { op, status: 'skipped', matched: 0, reason: 'no_owner_document' }; | ||
|
|
||
| const newP = doc.createElement('w:p'); | ||
| if (op.style) { | ||
| const pPr = doc.createElement('w:pPr'); | ||
| const pStyle = doc.createElement('w:pStyle'); | ||
| pStyle.setAttribute('w:val', op.style); | ||
| pPr.appendChild(pStyle); | ||
| newP.appendChild(pPr); | ||
| } | ||
| const newR = doc.createElement('w:r'); | ||
| const newT = doc.createElement('w:t'); | ||
| newT.setAttribute('xml:space', 'preserve'); | ||
| newT.textContent = op.text; | ||
| newR.appendChild(newT); | ||
| newP.appendChild(newR); | ||
| body.appendChild(newP); | ||
| return { op, status: 'applied', matched: 0, reason: 'empty_document_append' }; | ||
| } |
There was a problem hiding this comment.
status: 'applied' with matched: 0 is semantically ambiguous.
In the empty-document case, the op returns { status: 'applied', matched: 0 }. Other ops use matched: 0 exclusively with status: 'skipped'. This inconsistency could confuse the orchestrator or validation logic — e.g., if it interprets matched: 0 as "nothing happened."
Consider using matched: 1 here (since the paragraph was created), or documenting this as an intentional special case in the OpResult contract.
🤖 Prompt for AI Agents
In `@src/tools/docx/ops/insert-paragraph-after-text.ts` around lines 22 - 43, The
empty-document branch in insert-paragraph-after-text.ts currently returns { op,
status: 'applied', matched: 0 } which conflicts with the convention that
matched: 0 indicates skipped; update the return to { op, status: 'applied',
matched: 1, reason: 'empty_document_append' } (or otherwise set matched to a
non-zero value) so the creation of the paragraph is unambiguously reported by
the function that uses op, body, children and newP/newR/newT.
| const InsertTableOpSchema = z.object({ | ||
| type: z.literal('insert_table'), | ||
| after: z.string().optional(), | ||
| before: z.string().optional(), | ||
| headers: z.array(z.string()).optional(), | ||
| rows: z.array(z.array(z.string())), | ||
| colWidths: z.array(z.number()).optional(), | ||
| style: z.string().optional(), | ||
| }); | ||
|
|
||
| const InsertImageOpSchema = z.object({ | ||
| type: z.literal('insert_image'), | ||
| after: z.string().optional(), | ||
| before: z.string().optional(), | ||
| imagePath: z.string(), | ||
| width: z.number().optional(), | ||
| height: z.number().optional(), | ||
| altText: z.string().optional(), | ||
| }); |
There was a problem hiding this comment.
InsertTableOp and InsertImageOp don't enforce mutual exclusivity of after/before.
The type JSDoc states these are mutually exclusive, but the schemas accept both simultaneously. If a caller provides both after and before, the runtime behavior is undefined — whichever the op handler checks first wins silently.
Proposed fix (InsertTableOpSchema example — same for InsertImageOpSchema)
const InsertTableOpSchema = z.object({
type: z.literal('insert_table'),
after: z.string().optional(),
before: z.string().optional(),
headers: z.array(z.string()).optional(),
rows: z.array(z.array(z.string())),
colWidths: z.array(z.number()).optional(),
style: z.string().optional(),
-});
+}).refine(
+ (data) => !(data.after && data.before),
+ { message: '"after" and "before" are mutually exclusive' },
+);📝 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 InsertTableOpSchema = z.object({ | |
| type: z.literal('insert_table'), | |
| after: z.string().optional(), | |
| before: z.string().optional(), | |
| headers: z.array(z.string()).optional(), | |
| rows: z.array(z.array(z.string())), | |
| colWidths: z.array(z.number()).optional(), | |
| style: z.string().optional(), | |
| }); | |
| const InsertImageOpSchema = z.object({ | |
| type: z.literal('insert_image'), | |
| after: z.string().optional(), | |
| before: z.string().optional(), | |
| imagePath: z.string(), | |
| width: z.number().optional(), | |
| height: z.number().optional(), | |
| altText: z.string().optional(), | |
| }); | |
| const InsertTableOpSchema = z.object({ | |
| type: z.literal('insert_table'), | |
| after: z.string().optional(), | |
| before: z.string().optional(), | |
| headers: z.array(z.string()).optional(), | |
| rows: z.array(z.array(z.string())), | |
| colWidths: z.array(z.number()).optional(), | |
| style: z.string().optional(), | |
| }).refine( | |
| (data) => !(data.after && data.before), | |
| { message: '"after" and "before" are mutually exclusive' }, | |
| ); | |
| const InsertImageOpSchema = z.object({ | |
| type: z.literal('insert_image'), | |
| after: z.string().optional(), | |
| before: z.string().optional(), | |
| imagePath: z.string(), | |
| width: z.number().optional(), | |
| height: z.number().optional(), | |
| altText: z.string().optional(), | |
| }).refine( | |
| (data) => !(data.after && data.before), | |
| { message: '"after" and "before" are mutually exclusive' }, | |
| ); |
🤖 Prompt for AI Agents
In `@src/tools/schemas.ts` around lines 174 - 192, The schemas InsertTableOpSchema
and InsertImageOpSchema currently allow both after and before to be provided,
which contradicts the JSDoc; update each schema to enforce mutual exclusivity by
adding a Zod refinement (e.g., .refine(obj => !(obj.after && obj.before), {
message: 'Provide either "after" or "before", not both', path:
['after','before'] })) to validate that both fields are not present at the same
time for InsertTableOpSchema and InsertImageOpSchema.
| export const WriteDocxArgsSchema = z.object({ | ||
| inputPath: z.string().optional(), | ||
| outputPath: z.string(), | ||
| ops: z.array(DocxOpSchema).optional(), | ||
| content: DocxContentStructureSchema.optional(), | ||
| }).refine( | ||
| (data) => { | ||
| // For new files (no inputPath): require content, ops optional | ||
| // For updates (has inputPath): require ops, content not allowed | ||
| if (!data.inputPath) { | ||
| return !!data.content; // New file must have content | ||
| } else { | ||
| return !!data.ops; // Update must have ops | ||
| } | ||
| }, | ||
| { message: 'For new files (no inputPath): provide "content". For updates (has inputPath): provide "ops".' } | ||
| ); |
There was a problem hiding this comment.
WriteDocxArgsSchema refinement doesn't reject content on update path.
The comment says "For updates (has inputPath): require ops, content not allowed", but the refinement only checks !!data.ops — it doesn't reject data.content being present. A caller could pass both inputPath + content + ops and the schema would accept it, potentially leading to confusion about which is used.
Proposed fix
(data) => {
if (!data.inputPath) {
return !!data.content;
} else {
- return !!data.ops;
+ return !!data.ops && !data.content;
}
},🤖 Prompt for AI Agents
In `@src/tools/schemas.ts` around lines 241 - 257, The refinement on
WriteDocxArgsSchema currently only enforces presence of ops for updates but
doesn't forbid content when inputPath is present; update the refine predicate
for WriteDocxArgsSchema so that: when inputPath is absent require content exists
and ops must be undefined/absent, and when inputPath is present require ops
exists and content must be undefined/absent; also adjust the refinement error
message to reflect both presence and mutual exclusion rules (mention inputPath,
content, and ops) to make the contract explicit.
PR Review — Issues #4 through #13Thanks for the work on this — the architecture (op pattern, validation snapshots, builders separation) is solid. However there are several issues that need to be addressed before merge. 🔴 #4 — Sync I/O blocks event loop (
|
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src/tools/docx/dom.ts`:
- Around line 227-238: setParagraphTextMinimal currently silently no-ops when
there are no <w:t> nodes; change its signature to return a boolean (true if text
was actually set, false otherwise), have it return false when tNodes.length ===
0 and true after successfully updating the first t node and clearing the rest,
and then update callers such as replace_paragraph_at_body_index to check the
boolean and set matched: 0 (or status accordingly) when false; alternatively you
may choose to throw an error instead of returning false, but ensure
replace_paragraph_at_body_index is updated to handle the new behavior.
- Around line 142-181: getTableContent currently always treats the first row as
headers which mislabels data tables; change getTableContent to either accept an
optional flag (e.g., treatFirstRowAsHeader?: boolean) or implement a quick
bold-detection heuristic before promoting the first row to headers: add a helper
like hasBoldInCell(cell: Element): boolean that inspects cell child runs (w:r ->
w:rPr -> w:b or w:strong) and use it on every cell of the first row (or require
the flag) so headers is only set when the flag is true or at least one/most
first-row cells have bold styling; update the function signature and calls
accordingly (reference getTableContent and getCellText to locate where to add
the flag/helper).
In `@src/tools/docx/ops/index.ts`:
- Around line 32-70: The dispatcher applyOp is synchronous and blocks the event
loop because applyInsertImage uses sync FS calls; change applyOp's signature to
async (return Promise<OpResult>) and convert the switch to await async handlers;
make applyInsertImage async and replace fs.readFileSync/fs.existsSync with
fs.promises APIs (or other non-blocking reads) and propagate async changes back
to callers (e.g., the loop in write.ts should await applyOp). Update any other
ops you change to return Promise<OpResult> so the switch can uniformly await
each handler.
In `@src/tools/docx/read.ts`:
- Around line 6-8: This file currently constructs new DOMParser/XMLSerializer
instances and calls new DOMParser().parseFromString(...) in several places
(mentions of DOMParser, XMLSerializer); replace those usages with the shared
utilities parseXml and serializeXml (the exported functions from dom.ts) so
parsing/serialization goes through the centralized helpers; update all
occurrences (the parseFromString call sites and any direct XMLSerializer usage)
to call parseXml(...) and serializeXml(...) respectively, remove the direct
imports of DOMParser/XMLSerializer, and ensure any code depending on the return
types still works with the DOM Document/Node returned by parseXml.
- Around line 328-333: The slice logic for paragraphs mishandles negative
options.offset (options.offset || 0 then slice(offset, offset + length)) causing
empty results; update the computation to support negative-offset tail behavior:
compute a normalized start index like const start = options.offset !== undefined
? (options.offset >= 0 ? options.offset : Math.max(allParagraphs.length +
options.offset, 0)) : 0, compute length as before (const length = options.length
!== undefined ? options.length : allParagraphs.length), then set paragraphs =
allParagraphs.slice(start, Math.min(start + length, allParagraphs.length));
adjust references to options.offset, options.length, allParagraphs and
paragraphs accordingly so negative offsets return the tail segment.
- Around line 271-280: getDocxMetadata currently reloads the same file by
calling loadDocx(path) and extractParagraphs(path), causing redundant reads;
change getDocxMetadata to accept an already-loaded zip (returned from loadDocx)
and the parsed paragraphs (from extractParagraphs) instead of a path, update its
signature and internal logic to use those inputs, and modify readDocx to call
getDocxMetadata(zip, paragraphs) after it has loaded the zip and parsed
paragraphs; to preserve compatibility you can add an overload or optional
parameters so callers that still pass a path fall back to the existing behavior,
and update any references to loadDocx/extractParagraphs inside getDocxMetadata
accordingly.
In `@src/tools/schemas.ts`:
- Around line 190-198: InsertImageOpSchema and DocxContentImageSchema allow
zero/negative image dimensions because width and height are declared as
z.number().optional(); update both schemas so the width and height validators
include .min(1) (e.g., z.number().min(1).optional()) to enforce positive
dimensions and prevent invalid DOCX drawing XML; modify the width and height
entries in InsertImageOpSchema and the corresponding fields in
DocxContentImageSchema accordingly.
🧹 Nitpick comments (3)
src/tools/docx/ops/set-color-for-paragraph-exact.ts (1)
21-30: Inner loop variableishadows outer loop variablei.Line 26 declares
let iwhich shadows the outerlet ion Line 21. Whileletis block-scoped so this works correctly at runtime, it's a readability hazard. Additionally, the inner loop reimplements whatcolorParagraphRuns(p, op.color)fromdom.tsalready does.Proposed fix
- const runs = p.getElementsByTagName('w:r'); - for (let i = 0; i < runs.length; i++) { - ensureRunColor(runs.item(i) as Element, op.color); - } + colorParagraphRuns(p, op.color);With import update:
-import { getParagraphText, ensureRunColor } from '../dom.js'; +import { getParagraphText, colorParagraphRuns } from '../dom.js';src/server.ts (2)
25-34:DC_PRIORITY_NOTEis injected only into theget_configtool description.This routing instruction is critical for correct tool selection, but it's embedded in
get_config's description (Line 227), which is semantically unrelated to file routing. If a client doesn't callget_configearly in a session, the LLM may never see this guidance.Consider whether this should also appear in
read_fileor as a top-level server instruction/system prompt, depending on how the MCP client surfaces tool descriptions.
497-608:write_docxtool description is comprehensive but very long.The description is ~110 lines with detailed operation examples. While thorough for LLM guidance, this consumes significant tokens in every
list_toolsresponse. Consider whether some of this detail (especially per-op examples on Lines 542-588) could be shortened or moved to a linked reference, keeping only the workflow and key constraints inline.That said, this is a tradeoff — more detail helps the LLM use the tool correctly.
| export function getTableContent(tbl: Element): { headers?: string[]; rows: string[][] } { | ||
| const rows: Element[] = []; | ||
| for (const child of nodeListToArray(tbl.childNodes)) { | ||
| if (child.nodeType === 1 && (child as Element).nodeName === 'w:tr') { | ||
| rows.push(child as Element); | ||
| } | ||
| } | ||
|
|
||
| if (rows.length === 0) { | ||
| return { rows: [] }; | ||
| } | ||
|
|
||
| // Extract cells from each row | ||
| const tableRows: string[][] = []; | ||
| for (const row of rows) { | ||
| const cells: string[] = []; | ||
| for (const child of nodeListToArray(row.childNodes)) { | ||
| if (child.nodeType === 1 && (child as Element).nodeName === 'w:tc') { | ||
| cells.push(getCellText(child as Element)); | ||
| } | ||
| } | ||
| if (cells.length > 0) { | ||
| tableRows.push(cells); | ||
| } | ||
| } | ||
|
|
||
| // First row might be header - check if it has bold formatting | ||
| // For simplicity, we'll treat first row as potential header | ||
| // User can determine this based on style or content | ||
| if (tableRows.length > 0) { | ||
| const firstRow = tableRows[0]; | ||
| const restRows = tableRows.slice(1); | ||
| return { | ||
| headers: firstRow.length > 0 ? firstRow : undefined, | ||
| rows: restRows.length > 0 ? restRows : [], | ||
| }; | ||
| } | ||
|
|
||
| return { rows: tableRows }; | ||
| } |
There was a problem hiding this comment.
getTableContent unconditionally treats the first row as headers.
Every table's first row is split out as headers, even when the table has no actual header row. This produces misleading outlines for data-only tables. Since this feeds into ReadDocxResult.tables[].headers, downstream consumers (and LLM callers) will assume bold/header semantics.
Consider making this heuristic opt-in or checking for bold formatting on the first row's cells (the comment on Line 168 already hints at this).
🤖 Prompt for AI Agents
In `@src/tools/docx/dom.ts` around lines 142 - 181, getTableContent currently
always treats the first row as headers which mislabels data tables; change
getTableContent to either accept an optional flag (e.g., treatFirstRowAsHeader?:
boolean) or implement a quick bold-detection heuristic before promoting the
first row to headers: add a helper like hasBoldInCell(cell: Element): boolean
that inspects cell child runs (w:r -> w:rPr -> w:b or w:strong) and use it on
every cell of the first row (or require the flag) so headers is only set when
the flag is true or at least one/most first-row cells have bold styling; update
the function signature and calls accordingly (reference getTableContent and
getCellText to locate where to add the flag/helper).
| export function setParagraphTextMinimal(p: Element, text: string): void { | ||
| const tNodes = p.getElementsByTagName('w:t'); | ||
| if (tNodes.length === 0) return; | ||
|
|
||
| const first = tNodes.item(0)!; | ||
| first.textContent = text; | ||
| first.setAttribute('xml:space', 'preserve'); | ||
|
|
||
| for (let i = 1; i < tNodes.length; i++) { | ||
| tNodes.item(i)!.textContent = ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
setParagraphTextMinimal silently no-ops when the paragraph has no <w:t> nodes.
If a paragraph contains no runs/text nodes (e.g., an empty paragraph or one with only drawing elements), this function returns without modifying anything and without signaling to callers. Operations like replace_paragraph_at_body_index that rely on this will report status: 'applied' even though no text was actually set.
Consider returning a boolean or throwing so callers can set matched: 0 appropriately.
🤖 Prompt for AI Agents
In `@src/tools/docx/dom.ts` around lines 227 - 238, setParagraphTextMinimal
currently silently no-ops when there are no <w:t> nodes; change its signature to
return a boolean (true if text was actually set, false otherwise), have it
return false when tNodes.length === 0 and true after successfully updating the
first t node and clearing the rest, and then update callers such as
replace_paragraph_at_body_index to check the boolean and set matched: 0 (or
status accordingly) when false; alternatively you may choose to throw an error
instead of returning false, but ensure replace_paragraph_at_body_index is
updated to handle the new behavior.
| export function applyOp(body: Element, op: DocxOp, zip?: PizZip): OpResult { | ||
| switch (op.type) { | ||
| case 'replace_paragraph_text_exact': | ||
| return applyReplaceParagraphTextExact(body, op); | ||
| case 'replace_paragraph_at_body_index': | ||
| return applyReplaceParagraphAtBodyIndex(body, op); | ||
| case 'set_color_for_style': | ||
| return applySetColorForStyle(body, op); | ||
| case 'set_color_for_paragraph_exact': | ||
| return applySetColorForParagraphExact(body, op); | ||
| case 'set_paragraph_style_at_body_index': | ||
| return applySetParagraphStyleAtBodyIndex(body, op); | ||
| case 'insert_paragraph_after_text': | ||
| return applyInsertParagraphAfterText(body, op); | ||
| case 'delete_paragraph_at_body_index': | ||
| return applyDeleteParagraphAtBodyIndex(body, op); | ||
| case 'table_set_cell_text': | ||
| return applyTableSetCellText(body, op); | ||
| case 'replace_table_cell_text': | ||
| return applyReplaceTableCellText(body, op); | ||
| case 'replace_hyperlink_url': | ||
| if (!zip) return { op, status: 'skipped', matched: 0, reason: 'zip_required_for_hyperlink_op' }; | ||
| return applyReplaceHyperlinkUrl(body, op, zip); | ||
| case 'header_replace_text_exact': | ||
| if (!zip) return { op, status: 'skipped', matched: 0, reason: 'zip_required_for_header_op' }; | ||
| return applyHeaderReplaceTextExact(body, op, zip); | ||
| case 'insert_table': | ||
| return applyInsertTable(body, op); | ||
| case 'insert_image': | ||
| if (!zip) return { op, status: 'skipped', matched: 0, reason: 'zip_required_for_image_op' }; | ||
| return applyInsertImage(body, op, zip); | ||
| default: | ||
| return { | ||
| op, | ||
| status: 'skipped', | ||
| matched: 0, | ||
| reason: `unknown_op_type: ${(op as any).type}`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Synchronous dispatcher will block the event loop for insert_image ops.
applyOp is synchronous (→ OpResult), but applyInsertImage (per PR review blocker #4) uses fs.readFileSync / fs.existsSync internally to read image files from disk. This blocks the Node.js event loop during image insertion.
Make applyOp async (returning Promise<OpResult>) and convert image ops to use fs.promises. Since the caller in write.ts likely iterates ops in a loop, switching to async/await here is straightforward.
Proposed signature change
-export function applyOp(body: Element, op: DocxOp, zip?: PizZip): OpResult {
+export async function applyOp(body: Element, op: DocxOp, zip?: PizZip): Promise<OpResult> {Then convert applyInsertImage to async as well, and await it at the call site (Line 62).
🤖 Prompt for AI Agents
In `@src/tools/docx/ops/index.ts` around lines 32 - 70, The dispatcher applyOp is
synchronous and blocks the event loop because applyInsertImage uses sync FS
calls; change applyOp's signature to async (return Promise<OpResult>) and
convert the switch to await async handlers; make applyInsertImage async and
replace fs.readFileSync/fs.existsSync with fs.promises APIs (or other
non-blocking reads) and propagate async changes back to callers (e.g., the loop
in write.ts should await applyOp). Update any other ops you change to return
Promise<OpResult> so the switch can uniformly await each handler.
| import fs from 'fs/promises'; | ||
| import PizZip from 'pizzip'; | ||
| import { DOMParser, XMLSerializer } from '@xmldom/xmldom'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent XML parsing — bypasses shared parseXml utility from dom.ts.
The file imports DOMParser and XMLSerializer directly from @xmldom/xmldom (Line 8) and calls new DOMParser().parseFromString(...) in multiple places (Lines 53, 110, 208, 230, 255, 290, 312) instead of using the shared parseXml/serializeXml from dom.ts.
This means any future error-handling improvements to parseXml (already flagged in a prior review) won't apply here. Consolidate to use the shared utilities.
Also applies to: 53-53, 110-110, 208-208, 230-230, 255-255, 290-290, 312-312
🤖 Prompt for AI Agents
In `@src/tools/docx/read.ts` around lines 6 - 8, This file currently constructs
new DOMParser/XMLSerializer instances and calls new
DOMParser().parseFromString(...) in several places (mentions of DOMParser,
XMLSerializer); replace those usages with the shared utilities parseXml and
serializeXml (the exported functions from dom.ts) so parsing/serialization goes
through the centralized helpers; update all occurrences (the parseFromString
call sites and any direct XMLSerializer usage) to call parseXml(...) and
serializeXml(...) respectively, remove the direct imports of
DOMParser/XMLSerializer, and ensure any code depending on the return types still
works with the DOM Document/Node returned by parseXml.
| /** Get comprehensive metadata. */ | ||
| export async function getDocxMetadata(path: string): Promise<DocxMetadata> { | ||
| const zip = await loadDocx(path); | ||
| const paragraphs = await extractParagraphs(path); | ||
| const coreProps = await getCoreProperties(zip); | ||
| const fullText = paragraphs.map((p) => p.text).join(' '); | ||
| const wordCount = fullText.split(/\s+/).filter((w) => w.length > 0).length; | ||
|
|
||
| return { ...coreProps, paragraphCount: paragraphs.length, wordCount }; | ||
| } |
There was a problem hiding this comment.
getDocxMetadata re-reads the file and re-parses paragraphs redundantly.
getDocxMetadata calls loadDocx(path) (Line 273) and extractParagraphs(path) (Line 274), which internally calls loadDocx(path) again. When called from readDocx (Line 335), the same file has already been loaded on Line 307 — totaling 3 reads of the same file.
Refactor to accept the already-loaded zip and parsed paragraphs:
Proposed fix
-export async function getDocxMetadata(path: string): Promise<DocxMetadata> {
- const zip = await loadDocx(path);
- const paragraphs = await extractParagraphs(path);
+export async function getDocxMetadata(
+ path: string,
+ zip?: PizZip,
+ paragraphs?: DocxParagraph[],
+): Promise<DocxMetadata> {
+ zip = zip ?? await loadDocx(path);
+ paragraphs = paragraphs ?? await extractParagraphs(path);
const coreProps = await getCoreProperties(zip);
const fullText = paragraphs.map((p) => p.text).join(' ');
const wordCount = fullText.split(/\s+/).filter((w) => w.length > 0).length;
return { ...coreProps, paragraphCount: paragraphs.length, wordCount };
}🤖 Prompt for AI Agents
In `@src/tools/docx/read.ts` around lines 271 - 280, getDocxMetadata currently
reloads the same file by calling loadDocx(path) and extractParagraphs(path),
causing redundant reads; change getDocxMetadata to accept an already-loaded zip
(returned from loadDocx) and the parsed paragraphs (from extractParagraphs)
instead of a path, update its signature and internal logic to use those inputs,
and modify readDocx to call getDocxMetadata(zip, paragraphs) after it has loaded
the zip and parsed paragraphs; to preserve compatibility you can add an overload
or optional parameters so callers that still pass a path fall back to the
existing behavior, and update any references to loadDocx/extractParagraphs
inside getDocxMetadata accordingly.
| let paragraphs = allParagraphs; | ||
| if (options?.offset !== undefined || options?.length !== undefined) { | ||
| const offset = options.offset || 0; | ||
| const length = options.length !== undefined ? options.length : allParagraphs.length; | ||
| paragraphs = allParagraphs.slice(offset, offset + length); | ||
| } |
There was a problem hiding this comment.
Negative offset values produce incorrect/empty results.
options.offset || 0 is falsy-coalescing (fine), but allParagraphs.slice(offset, offset + length) with a negative offset breaks: e.g., slice(-5, -5 + 10) → slice(-5, 5) — on a 20-element array this is slice(15, 5) which returns [].
The read_file tool description advertises negative-offset tail behavior for DOCX, but this implementation doesn't support it. Either implement tail behavior (as done for text files) or document the limitation.
Proposed fix for negative offset support
let paragraphs = allParagraphs;
if (options?.offset !== undefined || options?.length !== undefined) {
- const offset = options.offset || 0;
- const length = options.length !== undefined ? options.length : allParagraphs.length;
- paragraphs = allParagraphs.slice(offset, offset + length);
+ const offset = options.offset ?? 0;
+ const length = options.length ?? allParagraphs.length;
+ if (offset < 0) {
+ paragraphs = allParagraphs.slice(offset); // tail behavior
+ } else {
+ paragraphs = allParagraphs.slice(offset, offset + length);
+ }
}📝 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.
| let paragraphs = allParagraphs; | |
| if (options?.offset !== undefined || options?.length !== undefined) { | |
| const offset = options.offset || 0; | |
| const length = options.length !== undefined ? options.length : allParagraphs.length; | |
| paragraphs = allParagraphs.slice(offset, offset + length); | |
| } | |
| let paragraphs = allParagraphs; | |
| if (options?.offset !== undefined || options?.length !== undefined) { | |
| const offset = options.offset ?? 0; | |
| const length = options.length ?? allParagraphs.length; | |
| if (offset < 0) { | |
| paragraphs = allParagraphs.slice(offset); // tail behavior | |
| } else { | |
| paragraphs = allParagraphs.slice(offset, offset + length); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/tools/docx/read.ts` around lines 328 - 333, The slice logic for
paragraphs mishandles negative options.offset (options.offset || 0 then
slice(offset, offset + length)) causing empty results; update the computation to
support negative-offset tail behavior: compute a normalized start index like
const start = options.offset !== undefined ? (options.offset >= 0 ?
options.offset : Math.max(allParagraphs.length + options.offset, 0)) : 0,
compute length as before (const length = options.length !== undefined ?
options.length : allParagraphs.length), then set paragraphs =
allParagraphs.slice(start, Math.min(start + length, allParagraphs.length));
adjust references to options.offset, options.length, allParagraphs and
paragraphs accordingly so negative offsets return the tail segment.
| const InsertImageOpSchema = z.object({ | ||
| type: z.literal('insert_image'), | ||
| after: z.string().optional(), | ||
| before: z.string().optional(), | ||
| imagePath: z.string(), | ||
| width: z.number().optional(), | ||
| height: z.number().optional(), | ||
| altText: z.string().optional(), | ||
| }); |
There was a problem hiding this comment.
Image width/height lack minimum value constraints.
z.number().optional() allows zero or negative dimensions, which would produce invalid drawing XML in the DOCX. Add .min(1) to prevent nonsensical image sizes.
Proposed fix (apply to both InsertImageOpSchema and DocxContentImageSchema)
- width: z.number().optional(),
- height: z.number().optional(),
+ width: z.number().min(1).optional(),
+ height: z.number().min(1).optional(),Also applies to: 230-236
🤖 Prompt for AI Agents
In `@src/tools/schemas.ts` around lines 190 - 198, InsertImageOpSchema and
DocxContentImageSchema allow zero/negative image dimensions because width and
height are declared as z.number().optional(); update both schemas so the width
and height validators include .min(1) (e.g., z.number().min(1).optional()) to
enforce positive dimensions and prevent invalid DOCX drawing XML; modify the
width and height entries in InsertImageOpSchema and the corresponding fields in
DocxContentImageSchema accordingly.
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
|
CodeAnt AI Incremental review completed. |
2 participants
User description
Summary by CodeRabbit
New Features
Quality
CodeAnt-AI Description
Add first-class DOCX read/write tools and local Desktop Commander routing
What Changed
Impact
✅ Precise DOCX editing workflows (single read_docx → one write_docx call)✅ Preserve formatting when creating or modifying DOCX✅ Avoid accidental cloud-based edits of local DOCX/PDF files💡 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.