diff --git a/src/handlers/filesystem-handlers.ts b/src/handlers/filesystem-handlers.ts index fadd05e79..e79ddaa72 100644 --- a/src/handlers/filesystem-handlers.ts +++ b/src/handlers/filesystem-handlers.ts @@ -243,9 +243,9 @@ export async function handleReadFile(args: unknown): Promise { /** * Handle read_multiple_files command */ -export async function handleReadMultipleFiles(args: unknown): Promise { +export async function handleReadMultipleFiles(args: unknown, stillLoadingError?: (filePath: string) => string | undefined): Promise { const parsed = ReadMultipleFilesArgsSchema.parse(args); - const fileResults = await readMultipleFiles(parsed.paths); + const fileResults = await readMultipleFiles(parsed.paths, stillLoadingError); // Create a text summary of all files const textSummary = fileResults.map(result => { diff --git a/src/index.ts b/src/index.ts index 60d22f57b..1cd207c4a 100644 --- a/src/index.ts +++ b/src/index.ts @@ -15,6 +15,7 @@ import { logToStderr, logger } from './utils/logger.js'; import { exitProcess } from './utils/exit-process.js'; import { runRemote } from './npm-scripts/remote.js'; import { ensureChromeAvailable } from './tools/pdf/markdown.js'; +import { preloadFileSupport } from './utils/files/index.js'; // Store messages to defer until after initialization const deferredMessages: Array<{ level: string, message: string }> = []; @@ -136,6 +137,10 @@ async function runServer() { // Preemptively check/download Chrome for PDF generation (runs in background) ensureChromeAvailable(); + + // Load the Excel, DOCX and PDF packages, one at a time, now that initialize + // has been answered; until then the calls that need them answer at once + preloadFileSupport(); }; await server.connect(transport); diff --git a/src/search-manager.ts b/src/search-manager.ts index 8b111cf36..858f6feba 100644 --- a/src/search-manager.ts +++ b/src/search-manager.ts @@ -7,7 +7,8 @@ import { capture } from './utils/capture.js'; import { logger } from './utils/logger.js'; import { getRipgrepPath } from './utils/ripgrep-resolver.js'; import { isExcelFile } from './utils/files/index.js'; -import PizZip from 'pizzip'; +import { exceljsPackage } from './utils/files/excel.js'; +import { pizzipPackage } from './utils/files/docx.js'; export interface SearchResult { context?: boolean; // A line around a match (contextLines), not a match @@ -576,14 +577,16 @@ function characterClassEnd(glob: string, start: number): number { excelFiles = this.filterOfficeFiles(excelFiles, filePattern, rootPath); } - // Dynamically import ExcelJS to search all sheets - const ExcelJS = await import('exceljs'); + if (excelFiles.length === 0) return; + // ExcelJS, to search all sheets: loaded here if the server hasn't yet + // (a search runs in the background, so it isn't refused while it loads) + const ExcelJS = exceljsPackage.load(); for (const filePath of excelFiles) { if (sink.isStopped()) break; try { - const workbook = new ExcelJS.default.Workbook(); + const workbook = new ExcelJS.Workbook(); await workbook.xlsx.readFile(filePath); // Search ALL sheets in the workbook (row-wise for speed and cross-column matching) @@ -773,6 +776,10 @@ function characterClassEnd(glob: string, start: number): number { docxFiles = this.filterOfficeFiles(docxFiles, filePattern, rootPath); } + if (docxFiles.length === 0) return; + // PizZip, to open the DOCX files: loaded here if the server hasn't yet + const PizZip = pizzipPackage.load(); + for (const filePath of docxFiles) { if (sink.isStopped()) break; diff --git a/src/server.ts b/src/server.ts index bb035af02..8e8157dc2 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1202,6 +1202,19 @@ server.setRequestHandler(ListToolsRequestSchema, async () => { import * as handlers from './handlers/index.js'; import { ServerResult } from './types.js'; import { withoutInternalFacts } from './utils/internal-facts.js'; +import { stillLoadingError, type FileAction } from './utils/files/index.js'; + +/** + * A tool call on an Excel, DOCX or PDF file whose package is still loading + * after initialize answers at once with this error (utils/files/factory.ts) + * instead of loading it inside the call. Not sent to telemetry, unlike other + * errors: it names the file. read_multiple_files answers per file; + * get_file_info waits for the load, or loads it itself, and so does a search. + */ +function stillLoading(filePath: unknown, action: FileAction, options?: { isPdf?: boolean; isUrl?: boolean }): ServerResult | undefined { + const error = typeof filePath === 'string' ? stillLoadingError(filePath, action, options) : undefined; + return error === undefined ? undefined : { content: [{ type: 'text', text: `Error: ${error}` }], isError: true }; +} server.setRequestHandler(CallToolRequestSchema, async (request: CallToolRequest): Promise => { const args = request.params.arguments; @@ -1445,20 +1458,25 @@ async function handleCallToolRequest(request: CallToolRequest): Promise stillLoadingError(filePath, 'read')); break; case "write_file": - result = await handlers.handleWriteFile(args); + result = stillLoading(args?.path, 'write') ?? await handlers.handleWriteFile(args); break; - case "write_pdf": - result = await handlers.handleWritePdf(args); + case "write_pdf": { + // Markdown (a new PDF) or page edits, read as the tool reads them; + // arguments the tool refuses get its own error + const parsed = WritePdfArgsSchema.safeParse(args); + const action = parsed.success && typeof parsed.data.content === 'string' ? 'write' : 'edit'; + result = (parsed.success ? stillLoading(parsed.data.path, action, { isPdf: true }) : undefined) ?? await handlers.handleWritePdf(args); break; + } case "create_directory": result = await handlers.handleCreateDirectory(args); @@ -1493,7 +1511,7 @@ async function handleCallToolRequest(request: CallToolRequest): Promise { +/** + * Reads each file; a failure is that file's `error`. `stillLoadingError` (the + * server's) gives the error for a file whose package is still loading, which + * is then not read (utils/files/factory.ts). + */ +export async function readMultipleFiles(paths: string[], stillLoadingError?: (filePath: string) => string | undefined): Promise { return Promise.all( paths.map(async (filePath: string) => { + const stillLoading = stillLoadingError?.(filePath); + if (stillLoading) { + return { path: filePath, error: stillLoading }; + } try { const validPath = await validatePath(filePath); const fileResult = await readFile(validPath); diff --git a/src/tools/pdf/extract-images.ts b/src/tools/pdf/extract-images.ts index 472585b8e..e7b54af86 100644 --- a/src/tools/pdf/extract-images.ts +++ b/src/tools/pdf/extract-images.ts @@ -1,4 +1,4 @@ -import { getDocumentProxy, extractImages } from 'unpdf'; +import { LazyPackage } from '../../utils/lazy-package.js'; export interface ImageInfo { /** Object ID within PDF */ @@ -29,6 +29,9 @@ export interface ImageCompressionOptions { maxDimension?: number; } +// import(): unpdf is an ES module only. If it fails, Node keeps it failed until a restart. +export const unpdfPackage = new LazyPackage('unpdf', 'PDF reading support', () => import('unpdf')); + /** * Optimized image extraction from PDF using unpdf's built-in extractImages method * @param pdfBuffer PDF file as Uint8Array @@ -41,6 +44,7 @@ export async function extractImagesFromPdf( pageNumbers?: number[], compressionOptions: ImageCompressionOptions = {} ): Promise> { + const { getDocumentProxy, extractImages } = await unpdfPackage.load(); const pdfDocument = await getDocumentProxy(pdfBuffer); const pagesToProcess = pageNumbers || Array.from({ length: pdfDocument.numPages }, (_, i) => i + 1); diff --git a/src/tools/pdf/index.ts b/src/tools/pdf/index.ts index f0a807df6..7693477fa 100644 --- a/src/tools/pdf/index.ts +++ b/src/tools/pdf/index.ts @@ -1,8 +1,9 @@ -export { editPdf, insertRenderOptions } from './manipulations.js'; +export { editPdf, insertRenderOptions, pdfLibPackage } from './manipulations.js'; export type { PdfOperations, PdfInsertOperation, PdfDeleteOperation } from './manipulations.js'; -export { parsePdfToMarkdown, parseMarkdownToPdf, resolveRender } from './markdown.js'; +export { parsePdfToMarkdown, parseMarkdownToPdf, resolveRender, mdToPdfPackage } from './markdown.js'; export type { IgnoredRenderOption } from './markdown.js'; +export { pdf2mdPackage } from './lib/pdf2md.js'; export type { PdfMetadata, PdfPageItem } from './lib/pdf2md.js'; -export { extractImagesFromPdf } from './extract-images.js'; +export { extractImagesFromPdf, unpdfPackage } from './extract-images.js'; export type { ImageInfo, PageImages } from './extract-images.js'; diff --git a/src/tools/pdf/lib/pdf2md.ts b/src/tools/pdf/lib/pdf2md.ts index 0b5b3b176..b7dbdfa6d 100644 --- a/src/tools/pdf/lib/pdf2md.ts +++ b/src/tools/pdf/lib/pdf2md.ts @@ -1,13 +1,18 @@ import { createRequire } from 'module'; +import { LazyPackage } from '../../../utils/lazy-package.js'; import { generatePageNumbers } from '../utils.js'; import { extractImagesFromPdf, ImageInfo } from '../extract-images.js'; const require = createRequire(import.meta.url); -const { parse } = require('@opendocsg/pdf2md/lib/util/pdf'); -const { makeTransformations, transform } = require('@opendocsg/pdf2md/lib/util/transformations'); +/** What @opendocsg/pdf2md's parse() returns: its modules are loaded untyped, with require() */ +type ParseResult = any; -type ParseResult = ReturnType; +export const pdf2mdPackage = new LazyPackage('@opendocsg/pdf2md', 'PDF reading support', () => { + const { parse } = require('@opendocsg/pdf2md/lib/util/pdf'); + const { makeTransformations, transform } = require('@opendocsg/pdf2md/lib/util/transformations'); + return { parse, makeTransformations, transform }; +}); /** @@ -69,6 +74,7 @@ export type PageRange = { * @returns A Promise that resolves to a PdfParseResult object containing the parsed data. */ export async function pdf2md(pdfBuffer: Uint8Array, pageNumbers: number[] | PageRange = []): Promise { + const { parse, makeTransformations, transform } = pdf2mdPackage.load(); const result = await parse(pdfBuffer); const { fonts, pages, pdfDocument } = result; diff --git a/src/tools/pdf/manipulations.ts b/src/tools/pdf/manipulations.ts index ad8209b36..90e7027a9 100644 --- a/src/tools/pdf/manipulations.ts +++ b/src/tools/pdf/manipulations.ts @@ -1,6 +1,7 @@ import fs from 'fs/promises'; import { createRequire } from 'module'; import type { PDFDocument as PDFDocumentType, PDFPage } from 'pdf-lib'; +import { LazyPackage } from '../../utils/lazy-package.js'; import { normalizePageIndexes } from './utils.js'; import { parseMarkdownToPdf } from './markdown.js'; import type { PdfInsertOperationSchema, PdfDeleteOperationSchema, PdfOperationSchema } from '../schemas.js'; @@ -8,7 +9,6 @@ import { z } from 'zod'; // Use createRequire to load pdf-lib as CJS (works around Node 25 ESM resolution issues) const require = createRequire(import.meta.url); -const { PDFDocument } = require('pdf-lib') as { PDFDocument: typeof PDFDocumentType }; // Infer TypeScript types from Zod schemas for consistency type PdfInsertOperation = z.infer; @@ -17,7 +17,10 @@ type PdfOperations = z.infer; export type { PdfOperations, PdfInsertOperation, PdfDeleteOperation }; +export const pdfLibPackage = new LazyPackage('pdf-lib', 'PDF editing support', (): { PDFDocument: typeof PDFDocumentType } => require('pdf-lib')); + async function loadPdfDocumentFromBuffer(filePathOrBuffer: string | Buffer | Uint8Array): Promise { + const { PDFDocument } = pdfLibPackage.load(); const buffer = typeof filePathOrBuffer === 'string' ? await fs.readFile(filePathOrBuffer) : filePathOrBuffer; const pdfBytes = new Uint8Array(buffer); return await PDFDocument.load(pdfBytes); diff --git a/src/tools/pdf/markdown.ts b/src/tools/pdf/markdown.ts index aa490ffab..62e5ce9c2 100644 --- a/src/tools/pdf/markdown.ts +++ b/src/tools/pdf/markdown.ts @@ -8,11 +8,11 @@ import { createRequire } from 'module'; import { tmpdir, userInfo } from 'os'; import { basename, dirname, isAbsolute, join, relative, resolve, sep } from 'path'; import type { Browser, LaunchOptions, PuppeteerNode } from 'puppeteer'; -import { convertMdToPdf } from 'md-to-pdf/dist/lib/md-to-pdf.js'; -import { defaultConfig, type Config as MdToPdfConfig } from 'md-to-pdf/dist/lib/config.js'; +import type { Config as MdToPdfConfig } from 'md-to-pdf/dist/lib/config.js'; import type { PageRange } from './lib/pdf2md.js'; import { PdfParseResult, pdf2md } from './lib/pdf2md.js'; import { CONFIG_FILE } from '../../config.js'; +import { LazyPackage } from '../../utils/lazy-package.js'; const isUrl = (source: string): boolean => source.startsWith('http://') || source.startsWith('https://'); @@ -34,13 +34,20 @@ const CHROME_EXIT_WAIT_MS = 10_000; /** Cookie a render's Chrome sends to its web server; nothing else gets in */ const RENDER_COOKIE_NAME = 'desktop-commander-pdf-render'; -// md-to-pdf's own Puppeteer, file server and front matter parser, loaded the +const require = createRequire(import.meta.url); + +// md-to-pdf, with its own Puppeteer, file server and front matter parser, loaded the // way md-to-pdf loads them (they are its dependencies, not Desktop Commander's) -const requireFromMdToPdf = createRequire(createRequire(import.meta.url).resolve('md-to-pdf')); -const puppeteer: PuppeteerNode = requireFromMdToPdf('puppeteer'); -const serveHandler: (request: IncomingMessage, response: ServerResponse, config: { public: string; directoryListing: boolean; cleanUrls: boolean }) => Promise = - requireFromMdToPdf('serve-handler'); -const grayMatter: (input: string, options: unknown) => { content: string; data: unknown } = requireFromMdToPdf('gray-matter'); +export const mdToPdfPackage = new LazyPackage('md-to-pdf', 'PDF writing support', () => { + const requireFromMdToPdf = createRequire(require.resolve('md-to-pdf')); + const { convertMdToPdf }: typeof import('md-to-pdf/dist/lib/md-to-pdf.js') = require('md-to-pdf/dist/lib/md-to-pdf.js'); + const { defaultConfig }: typeof import('md-to-pdf/dist/lib/config.js') = require('md-to-pdf/dist/lib/config.js'); + const puppeteer: PuppeteerNode = requireFromMdToPdf('puppeteer'); + const serveHandler: (request: IncomingMessage, response: ServerResponse, config: { public: string; directoryListing: boolean; cleanUrls: boolean }) => Promise = + requireFromMdToPdf('serve-handler'); + const grayMatter: (input: string, options: unknown) => { content: string; data: unknown } = requireFromMdToPdf('gray-matter'); + return { convertMdToPdf, defaultConfig, puppeteer, serveHandler, grayMatter }; +}); const isPlainObject = (value: unknown): value is Record => typeof value === 'object' && value !== null && !Array.isArray(value); @@ -100,9 +107,6 @@ interface ResolvedRender { ignoredOptions: IgnoredRenderOption[]; } -/** md-to-pdf's switch-off of gray-matter's JavaScript engine, which evaluates the header's code */ -const DISABLED_JS_ENGINE = (defaultConfig.gray_matter_options as { engines: Record }).engines.javascript; - /** * The caller's gray_matter_options over md-to-pdf's defaults, with the * JavaScript engine off whatever the caller says: a caller's settings (even @@ -112,12 +116,15 @@ const DISABLED_JS_ENGINE = (defaultConfig.gray_matter_options as { engines: Reco * both names are switched off. */ function safeGrayMatterOptions(callerOptions: unknown): Record { + const { defaultConfig } = mdToPdfPackage.load(); + // md-to-pdf's switch-off of gray-matter's JavaScript engine, which evaluates the header's code + const disabledJsEngine = (defaultConfig.gray_matter_options as { engines: Record }).engines.javascript; const caller = isPlainObject(callerOptions) ? callerOptions : {}; const callerEngines = isPlainObject(caller.engines) ? caller.engines : {}; return { ...defaultConfig.gray_matter_options, ...caller, - engines: { ...callerEngines, js: DISABLED_JS_ENGINE, javascript: DISABLED_JS_ENGINE }, + engines: { ...callerEngines, js: disabledJsEngine, javascript: disabledJsEngine }, }; } @@ -130,6 +137,7 @@ function safeGrayMatterOptions(callerOptions: unknown): Record * md-to-pdf merging the front matter a second time. */ export function resolveRender(markdown: string, options: unknown = {}): ResolvedRender { + const { grayMatter } = mdToPdfPackage.load(); const fromOptions = isPlainObject(options) ? options : {}; // Parse the front matter the way md-to-pdf would, with the caller's // gray_matter_options, but never with gray-matter's JavaScript engine @@ -578,7 +586,7 @@ async function serveAllowedFile(request: IncomingMessage, response: ServerRespon // serve-handler serves exactly that file (no .html redirects, which would name it from another folder). // It reads only the request's url and headers; the request itself keeps its URL. const forFile = { url: `/${encodeURIComponent(basename(file))}`, headers: request.headers } as IncomingMessage; - await serveHandler(forFile, response, { public: dirname(file), directoryListing: false, cleanUrls: false }); + await mdToPdfPackage.load().serveHandler(forFile, response, { public: dirname(file), directoryListing: false, cleanUrls: false }); } /** @@ -767,6 +775,7 @@ export async function parseMarkdownToPdf(markdown: string, options: any = {}): P // The render files as the caller gave them, by the checked paths the render reads let givenPaths = new Map(); try { + const { convertMdToPdf, defaultConfig, puppeteer } = mdToPdfPackage.load(); // The folder the markdown's files are served from must be inside the allowed folders const { validatePath } = await import('../filesystem.js'); const basedir: string = options.basedir ? await validatePath(options.basedir) : process.cwd(); diff --git a/src/utils/files/docx.ts b/src/utils/files/docx.ts index 11b029402..e38de23ae 100644 --- a/src/utils/files/docx.ts +++ b/src/utils/files/docx.ts @@ -17,7 +17,9 @@ */ import fs from 'fs/promises'; -import PizZip from 'pizzip'; +import { createRequire } from 'module'; +import type PizZip from 'pizzip'; +import { LazyPackage } from '../lazy-package.js'; import { FileHandler, FileResult, FileInfo, ReadOptions, EditResult } from './base.js'; // ════════════════════════════════════════════════════════════════ @@ -71,8 +73,18 @@ interface DocxZipContents { xmlParts: Map; } +const require = createRequire(import.meta.url); + +export const pizzipPackage = new LazyPackage('pizzip', 'DOCX support', (): typeof PizZip => require('pizzip')); + +/** Opens a zip from its bytes, or a new, empty one */ +function openZip(data?: Buffer): PizZip { + const PizZipClass = pizzipPackage.load(); + return data === undefined ? new PizZipClass() : new PizZipClass(data); +} + function loadDocxZip(buf: Buffer): DocxZipContents { - const zip = new PizZip(buf); + const zip = openZip(buf); const docFile = zip.file('word/document.xml'); if (!docFile) throw new Error('Invalid DOCX: missing word/document.xml'); @@ -448,7 +460,7 @@ function escapeXml(text: string): string { } function createMinimalDocxZip(documentXml: string): PizZip { - const zip = new PizZip(); + const zip = openZip(); zip.file('[Content_Types].xml', `` + @@ -651,7 +663,7 @@ export class DocxFileHandler implements FileHandler { // Load and pretty-print const buf = await fs.readFile(path); - const zip = new PizZip(buf); + const zip = openZip(buf); const docFile = zip.file('word/document.xml'); if (!docFile) throw new Error('Invalid DOCX: missing word/document.xml'); diff --git a/src/utils/files/excel.ts b/src/utils/files/excel.ts index 4f5284588..ac10ecc3c 100644 --- a/src/utils/files/excel.ts +++ b/src/utils/files/excel.ts @@ -3,8 +3,10 @@ * Handles reading, writing, and editing Excel files (.xlsx, .xls, .xlsm) */ -import ExcelJS from 'exceljs'; +import type ExcelJS from 'exceljs'; import fs from 'fs/promises'; +import { createRequire } from 'module'; +import { LazyPackage } from '../lazy-package.js'; import { FileHandler, ReadOptions, @@ -14,6 +16,15 @@ import { ExcelSheet } from './base.js'; +const require = createRequire(import.meta.url); + +// require(), not import(): Node keeps a failed import() failed, so it couldn't load again +export const exceljsPackage = new LazyPackage('exceljs', 'Excel support', (): typeof ExcelJS => require('exceljs')); + +async function newWorkbook(): Promise { + return new (exceljsPackage.load().Workbook)(); +} + // File size limit: 10MB const FILE_SIZE_LIMIT = 10 * 1024 * 1024; @@ -40,7 +51,7 @@ export class ExcelFileHandler implements FileHandler { async read(path: string, options?: ReadOptions): Promise { await this.checkFileSize(path); - const workbook = new ExcelJS.Workbook(); + const workbook = await newWorkbook(); await workbook.xlsx.readFile(path); const metadata = await this.extractMetadata(workbook, path); @@ -104,7 +115,7 @@ ${JSON.stringify(data)}`; // Handle append mode by finding last row and writing after it if (mode === 'append') { try { - const workbook = new ExcelJS.Workbook(); + const workbook = await newWorkbook(); await workbook.xlsx.readFile(path); if (Array.isArray(parsedContent)) { @@ -141,7 +152,7 @@ ${JSON.stringify(data)}`; } // Rewrite mode (or append to non-existent file): create new workbook - const workbook = new ExcelJS.Workbook(); + const workbook = await newWorkbook(); if (Array.isArray(parsedContent)) { // Single sheet from 2D array @@ -180,7 +191,7 @@ ${JSON.stringify(data)}`; // Parse range: "Sheet1!A1:C10" or "Sheet1" const [sheetName, cellRange] = this.parseRange(range); - const workbook = new ExcelJS.Workbook(); + const workbook = await newWorkbook(); await workbook.xlsx.readFile(path); // Get or create sheet @@ -259,7 +270,7 @@ ${JSON.stringify(data)}`; const stats = await fs.stat(path); try { - const workbook = new ExcelJS.Workbook(); + const workbook = await newWorkbook(); await workbook.xlsx.readFile(path); const metadata = await this.extractMetadata(workbook, path); diff --git a/src/utils/files/factory.ts b/src/utils/files/factory.ts index 020280907..7facab073 100644 --- a/src/utils/files/factory.ts +++ b/src/utils/files/factory.ts @@ -6,13 +6,19 @@ * or async (content-based like BinaryFileHandler using isBinaryFile) */ +import path from 'path'; import { FileHandler } from './base.js'; import { TextFileHandler } from './text.js'; import { ImageFileHandler } from './image.js'; import { BinaryFileHandler } from './binary.js'; -import { ExcelFileHandler } from './excel.js'; +import { ExcelFileHandler, exceljsPackage } from './excel.js'; import { PdfFileHandler } from './pdf.js'; -import { DocxFileHandler } from './docx.js'; +import { DocxFileHandler, pizzipPackage } from './docx.js'; +import { pdfLibPackage } from '../../tools/pdf/manipulations.js'; +import { pdf2mdPackage } from '../../tools/pdf/lib/pdf2md.js'; +import { unpdfPackage } from '../../tools/pdf/extract-images.js'; +import { mdToPdfPackage } from '../../tools/pdf/markdown.js'; +import type { LazyPackage } from '../lazy-package.js'; // Singleton instances of each handler let excelHandler: ExcelFileHandler | null = null; @@ -123,3 +129,56 @@ export function isExcelFile(path: string): boolean { export function isImageFile(path: string): boolean { return getImageHandler().canHandle(path); } + +export type FileAction = 'read' | 'write' | 'edit'; + +/** + * Right after initialize (#777): the packages Excel, DOCX and PDF files need, + * loaded in the background one at a time, smaller first, so each pause of the + * main thread is one package. + */ +export function preloadFileSupport(): void { + void (async () => { + for (const pkg of [pizzipPackage, pdfLibPackage, pdf2mdPackage, unpdfPackage, exceljsPackage, mdToPdfPackage]) { + await pkg.preload(); + } + })(); +} + +/** The packages a file needs for `action`, by the handlers' canHandle() (its name, no disk access) */ +function packagesFor(filePath: string, action: FileAction, isPdf: boolean, isUrl: boolean): LazyPackage[] { + if (isPdf) { + // Editing a PDF can insert markdown pages, which are rendered + return action === 'read' ? [pdf2mdPackage, unpdfPackage] : action === 'write' ? [mdToPdfPackage] : [pdfLibPackage, mdToPdfPackage]; + } + // A URL is fetched, not opened by a file handler: only a PDF one is parsed + if (isUrl) return []; + if (getDocxHandler().canHandle(filePath)) return [pizzipPackage]; + if (getExcelHandler().canHandle(filePath)) return [exceljsPackage]; + return []; +} + +/** + * The answer for a tool call that would `action` `filePath` while a package it + * needs is still loading, naming the file; undefined once loaded. A package + * not preloading yet (before initialize, or after a failure) starts here, + * after the call has answered. `isPdf` for a PDF whatever its name (write_pdf), + * `isUrl` for a URL read. + */ +export function stillLoadingError( + filePath: string, + action: FileAction, + { isPdf = getPdfHandler().canHandle(filePath), isUrl = false }: { isPdf?: boolean; isUrl?: boolean } = {} +): string | undefined { + const pending = packagesFor(filePath, action, isPdf, isUrl).find((pkg) => !pkg.loaded); + if (!pending) return undefined; + const file = path.basename(filePath); + const { error, needsRestart, support } = pending; + if (!needsRestart) void pending.preload(); + if (!error) { + return `Can't ${action} ${file} yet: Desktop Commander is still loading its ${support} (it starts right after launch). Try again in a few seconds.`; + } + return needsRestart + ? `Can't ${action} ${file}: Desktop Commander couldn't load its ${support} (${error}). Restart Desktop Commander to load it again.` + : `Can't ${action} ${file}: Desktop Commander couldn't load its ${support} (${error}). It's loading it again; try again in a few seconds.`; +} diff --git a/src/utils/files/index.ts b/src/utils/files/index.ts index 992602309..037440811 100644 --- a/src/utils/files/index.ts +++ b/src/utils/files/index.ts @@ -7,10 +7,12 @@ export * from './base.js'; // Factory function -export { getFileHandler, isExcelFile, isImageFile } from './factory.js'; +export { getFileHandler, isExcelFile, isImageFile, preloadFileSupport, stillLoadingError } from './factory.js'; +export type { FileAction } from './factory.js'; // File handlers export { TextFileHandler } from './text.js'; export { ImageFileHandler, isImageAnswer } from './image.js'; export { BinaryFileHandler } from './binary.js'; -export { ExcelFileHandler } from './excel.js'; +export { ExcelFileHandler, exceljsPackage } from './excel.js'; +export { pizzipPackage } from './docx.js'; diff --git a/src/utils/lazy-package.ts b/src/utils/lazy-package.ts new file mode 100644 index 000000000..650d3766a --- /dev/null +++ b/src/utils/lazy-package.ts @@ -0,0 +1,41 @@ +import { logger } from './logger.js'; + +/** + * A package only some files need, declared next to the code that uses it, + * whose load() loads it on first use (#715). Loading them before initialize + * took 25-90 s on one reporter's machine and timed the client out, and loading + * one inside the tool call that first needs it can take longer than a client + * waits. So the server preloads them right after initialize, one at a time + * (preloadFileSupport() in utils/files/factory.ts), and until one is loaded the + * calls that need it answer at once that it is still loading. preload() loads + * it on a later turn of the event loop, so a call that starts it answers + * first. `error` says why the last preload failed. Node keeps a failed + * import() failed, so a package loaded with import() then `needsRestart`. + */ +export class LazyPackage { + loaded = false; + error?: string; + needsRestart = false; + private running?: Promise; + + constructor(readonly name: string, readonly support: string, readonly load: () => T) {} + + preload(): Promise { + this.running ??= new Promise((resolve) => setImmediate(resolve)).then(async () => { + let loading: unknown; + try { + loading = this.load(); + await loading; + this.loaded = true; + this.error = undefined; + } catch (error) { + this.error = error instanceof Error ? error.message : String(error); + this.needsRestart = loading instanceof Promise; + logger.error(`Loading ${this.name} failed: ${this.error}`); + } finally { + this.running = undefined; + } + }); + return this.running; + } +} diff --git a/test/fixtures/package-load-hooks.mjs b/test/fixtures/package-load-hooks.mjs new file mode 100644 index 000000000..dd6a9d57e --- /dev/null +++ b/test/fixtures/package-load-hooks.mjs @@ -0,0 +1,35 @@ +// Installed in the real server by test/helpers/server-modules.js with hookArgs() +// when a test holds a package back or makes it fail once: +// - DC_TEST_HOLD_PACKAGE: an import() of it doesn't resolve until the file +// DC_TEST_HOLD_RELEASE exists, so that package stays "still loading" +// (a require() can't be held: package-load-preload.mjs fails it instead) +// - DC_TEST_FAIL_PACKAGE_ONCE: the first ES module of the server's own copy of +// it (node_modules/ in its working folder) that loads fails, as a +// broken package does (DC_TEST_FAILED_MARKER records that it did); a +// CommonJS one fails in package-load-preload.mjs +import fs from 'node:fs'; +import path from 'node:path'; +import { pathToFileURL } from 'node:url'; + +const HELD = process.env.DC_TEST_HOLD_PACKAGE; +const RELEASE = process.env.DC_TEST_HOLD_RELEASE; +const FAIL_ONCE = process.env.DC_TEST_FAIL_PACKAGE_ONCE; +const FAILED_MARKER = process.env.DC_TEST_FAILED_MARKER; +// Its real path, as Node names its files (node_modules can be a link) +const PACKAGE_URL = FAIL_ONCE && `${pathToFileURL(fs.realpathSync(path.join(process.cwd(), 'node_modules', FAIL_ONCE))).href}/`; + +export async function resolve(specifier, context, nextResolve) { + if (specifier === HELD) { + while (!fs.existsSync(RELEASE)) await new Promise((done) => setTimeout(done, 25)); + } + return nextResolve(specifier, context); +} + +export async function load(url, context, nextLoad) { + const result = await nextLoad(url, context); + if (FAIL_ONCE && result.format === 'module' && url.startsWith(PACKAGE_URL) && !fs.existsSync(FAILED_MARKER)) { + fs.writeFileSync(FAILED_MARKER, ''); + throw new Error(`${FAIL_ONCE} failed to load (test)`); + } + return result; +} diff --git a/test/fixtures/package-load-preload.mjs b/test/fixtures/package-load-preload.mjs new file mode 100644 index 000000000..5fa030a85 --- /dev/null +++ b/test/fixtures/package-load-preload.mjs @@ -0,0 +1,24 @@ +// Preloaded into the real server (node --import) by test/helpers/server-modules.js +// when a test makes a package fail to load once: the first CommonJS file of +// DC_TEST_FAIL_PACKAGE_ONCE that runs throws, as a broken package does, whether +// require() or import() loads it (DC_TEST_FAILED_MARKER records that it did). +// Node keeps such an import() failed; a require() can try again. Only the +// server's own copy (node_modules/ in its working folder) fails, not +// one another package carries (@opendocsg/pdf2md has its own unpdf). +import fs from 'node:fs'; +import Module from 'node:module'; +import path from 'node:path'; + +const FAIL_ONCE = process.env.DC_TEST_FAIL_PACKAGE_ONCE; +const FAILED_MARKER = process.env.DC_TEST_FAILED_MARKER; +// Its real path, as Node names its files (node_modules can be a link) +const PACKAGE_DIR = fs.realpathSync(path.join(process.cwd(), 'node_modules', FAIL_ONCE)) + path.sep; + +const compileJs = Module._extensions['.js']; +Module._extensions['.js'] = function (module, filename) { + if (filename.startsWith(PACKAGE_DIR) && !fs.existsSync(FAILED_MARKER)) { + fs.writeFileSync(FAILED_MARKER, ''); + throw new Error(`${FAIL_ONCE} failed to load (test)`); + } + return compileJs.call(this, module, filename); +}; diff --git a/test/fixtures/record-modules-hooks.mjs b/test/fixtures/record-modules-hooks.mjs new file mode 100644 index 000000000..85222b552 --- /dev/null +++ b/test/fixtures/record-modules-hooks.mjs @@ -0,0 +1,12 @@ +// Installed in the real server by test/helpers/server-modules.js with hookArgs() +// (helpers/module-hooks.js): writes every module an import resolves to +// DC_TEST_MODULE_LOG, in record-modules-preload.mjs's format. +import fs from 'node:fs'; + +const log = fs.openSync(process.env.DC_TEST_MODULE_LOG, 'a'); + +export async function resolve(specifier, context, nextResolve) { + const resolved = await nextResolve(specifier, context); + fs.writeSync(log, `${Date.now()} ${resolved.url} ${context.parentURL ?? ''}\n`); + return resolved; +} diff --git a/test/fixtures/record-modules-preload.mjs b/test/fixtures/record-modules-preload.mjs new file mode 100644 index 000000000..965482865 --- /dev/null +++ b/test/fixtures/record-modules-preload.mjs @@ -0,0 +1,20 @@ +// Preloaded into the real server (node --import) by test/helpers/server-modules.js, +// which records the server's imports with record-modules-hooks.mjs: writes every +// module the server resolves through require to DC_TEST_MODULE_LOG as it happens, +// one " " line each, the hooks' format. (A +// require('node:...') of a built-in skips Module._resolveFilename; the tests +// leave built-ins out.) +import fs from 'node:fs'; +import Module from 'node:module'; +import { pathToFileURL } from 'node:url'; + +const log = fs.openSync(process.env.DC_TEST_MODULE_LOG, 'a'); +const record = (url, parentURL) => fs.writeSync(log, `${Date.now()} ${url} ${parentURL ?? ''}\n`); + +const resolveFilename = Module._resolveFilename; +Module._resolveFilename = function (request, parent, ...rest) { + const filename = resolveFilename.call(this, request, parent, ...rest); + const url = Module.isBuiltin(filename) ? `node:${filename.replace(/^node:/, '')}` : pathToFileURL(filename).href; + record(url, parent?.filename && pathToFileURL(parent.filename).href); + return filename; +}; diff --git a/test/helpers/heavy-packages.js b/test/helpers/heavy-packages.js new file mode 100644 index 000000000..031aa5c3c --- /dev/null +++ b/test/helpers/heavy-packages.js @@ -0,0 +1,22 @@ +/** + * Tool calls for Excel, DOCX and PDF files through the real server, the way a + * client gets past the server's background load: right after initialize, while + * the support for a file type is still loading, the server answers "Can't read + * report.xlsx yet: Desktop Commander is still loading its Excel support …". + * callToolOnceLoaded() tries again until it doesn't (read_multiple_files says + * so per file). + */ + +export const STILL_LOADING = /Desktop Commander is still loading its [A-Za-z ]+ support/; + +const textOf = (result) => (result.content ?? []).map((item) => item.text ?? '').join('\n'); + +/** client.callTool(params), again every 100 ms while the answer says the support is still loading, up to timeoutMs */ +export async function callToolOnceLoaded(client, params, { timeoutMs = 60_000, requestOptions } = {}) { + const deadline = Date.now() + timeoutMs; + for (;;) { + const result = await client.callTool(params, undefined, requestOptions); + if (!STILL_LOADING.test(textOf(result)) || Date.now() > deadline) return result; + await new Promise((resolve) => setTimeout(resolve, 100)); + } +} diff --git a/test/helpers/server-modules.js b/test/helpers/server-modules.js new file mode 100644 index 000000000..ef803cee8 --- /dev/null +++ b/test/helpers/server-modules.js @@ -0,0 +1,150 @@ +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import { fileURLToPath, pathToFileURL } from 'url'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; +import { LoggingMessageNotificationSchema } from '@modelcontextprotocol/sdk/types.js'; +import { isTestHome } from './test-env.js'; +import { closeClient } from './close-client.js'; +import { hookArgs } from './module-hooks.js'; + +const PROJECT_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); +const SERVER = path.join(PROJECT_ROOT, 'dist/index.js'); +const PRELOAD = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/record-modules-preload.mjs')).href; +/** Records the server's imports; the preload records its requires */ +const HOOKS = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/record-modules-hooks.mjs')).href; +/** Holds one package back, or makes it fail once (startServerRecordingModules({ holdPackage, failPackageOnce })) */ +const PACKAGE_LOAD_HOOKS = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/package-load-hooks.mjs')).href; +/** Makes a package's first CommonJS file fail (startServerRecordingModules({ failPackageOnce })) */ +const PACKAGE_LOAD_PRELOAD = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/package-load-preload.mjs')).href; + +/** Packages only reading, writing or rendering Excel, PDF and DOCX files need */ +export const HEAVY_PACKAGES = ['exceljs', 'pdf-lib', 'md-to-pdf', 'puppeteer', 'unpdf', '@opendocsg/pdf2md', 'pizzip']; + +/** The npm package a module URL belongs to ('@scope/name' or 'name'), or undefined for Node's and the server's own modules */ +export function packageOf(url) { + const marker = '/node_modules/'; + const at = url.lastIndexOf(marker); + if (at === -1) return undefined; + const [first, second] = url.slice(at + marker.length).split('/'); + return first.startsWith('@') ? `${first}/${second}` : first; +} + +function chromeExecutable(buildDir) { + if (process.platform === 'win32') return path.join(buildDir, 'chrome-win64', 'chrome.exe'); + if (process.platform === 'darwin') { + return path.join(buildDir, 'chrome-mac-arm64', 'Google Chrome for Testing.app', 'Contents', 'MacOS', 'Google Chrome for Testing'); + } + return path.join(buildDir, 'chrome-linux64', 'chrome'); +} + +/** + * Puts two Chrome for Testing builds (empty stand-ins) in Desktop Commander's + * Puppeteer cache. The server's Chrome warm-up picks the newer one and removes + * the older one, and never downloads Chrome. + */ +function seedChromeCache() { + const chromeDir = path.join(os.homedir(), '.claude-server-commander', 'puppeteer-cache', 'chrome'); + const [stale, current] = ['100.0.0.0', '101.0.0.0'].map((version) => path.join(chromeDir, `test-${version}`)); + for (const buildDir of [stale, current]) { + const executable = chromeExecutable(buildDir); + fs.mkdirSync(path.dirname(executable), { recursive: true }); + fs.writeFileSync(executable, ''); + } +} + +/** + * Starts the real server (dist/index.js) over MCP stdio, the way a client + * does, with a preload that records every module it resolves (import and + * require). Resolves once `initialize` is answered. + * + * With `holdPackage` (a package the server loads with import()), the server's + * imports of it don't resolve until release() is called: it stays "still + * loading". While it is held, keep other module loads out of the test: the + * held import can hold up the server's other imports. + * With `failPackageOnce`, the first file of that package the server runs fails, as + * a broken package does. + * + * Returns: + * - client: the connected MCP client + * - startedAt: Date.now() at spawn + * - initializedAt: Date.now() when the `initialize` answer arrived, before the + * client sent `notifications/initialized` (what the server does after that + * comes later) + * - modules(): every module resolved so far, first resolution each: [{ at, url, parent }] + * - logs(): what the server has logged so far (MCP log notifications), one string each + * - release(): lets a held package load + * - close() + * + * It writes to Desktop Commander's folder in the home, so it runs only in a test home. + */ +export async function startServerRecordingModules({ holdPackage, failPackageOnce } = {}) { + if (!isTestHome()) { + throw new Error('startServerRecordingModules writes to the home: run it through the test or repro runner'); + } + // The Chrome warm-up after the handshake finds a (stand-in) Chrome and never downloads one + seedChromeCache(); + const logFile = path.join(os.homedir(), `modules-${process.pid}-${Date.now()}.log`); + const releaseFile = path.join(os.homedir(), `release-${process.pid}-${Date.now()}`); + const failedMarker = path.join(os.homedir(), `failed-${process.pid}-${Date.now()}`); + fs.writeFileSync(logFile, ''); + + const transport = new StdioClientTransport({ + command: process.execPath, + args: [ + ...(holdPackage || failPackageOnce ? hookArgs(PACKAGE_LOAD_HOOKS) : []), + ...(failPackageOnce ? ['--import', PACKAGE_LOAD_PRELOAD] : []), + ...hookArgs(HOOKS), '--import', PRELOAD, SERVER, '--no-onboarding', + ], + cwd: PROJECT_ROOT, + env: { + ...process.env, + DC_TEST_MODULE_LOG: logFile, + ...(holdPackage ? { DC_TEST_HOLD_PACKAGE: holdPackage, DC_TEST_HOLD_RELEASE: releaseFile } : {}), + ...(failPackageOnce ? { DC_TEST_FAIL_PACKAGE_ONCE: failPackageOnce, DC_TEST_FAILED_MARKER: failedMarker } : {}), + }, + stderr: 'pipe', + }); + // The moment the initialize answer arrives, seen before the client acts on it + let initializedAt; + let onMessage; + Object.defineProperty(transport, 'onmessage', { + configurable: true, + get: () => onMessage, + set: (handler) => { + onMessage = handler && ((message, extra) => { + if (initializedAt === undefined && message?.result?.serverInfo) initializedAt = Date.now(); + return handler(message, extra); + }); + }, + }); + const client = new Client({ name: 'startup-modules-test', version: '1.0.0' }, { capabilities: {} }); + const logLines = []; + client.setNotificationHandler(LoggingMessageNotificationSchema, (notification) => { + logLines.push(String(notification.params.data)); + }); + const startedAt = Date.now(); + await client.connect(transport, { timeout: 120_000 }); + + const modules = () => { + const seen = new Map(); + for (const line of fs.readFileSync(logFile, 'utf8').split('\n')) { + const [at, url, parent] = line.split(' '); + if (url && !seen.has(url)) seen.set(url, { at: Number(at), url, parent }); + } + return [...seen.values()]; + }; + + const release = () => fs.writeFileSync(releaseFile, ''); + + const close = async () => { + release(); + await closeClient(client); + fs.rmSync(logFile, { force: true }); + fs.rmSync(releaseFile, { force: true }); + fs.rmSync(failedMarker, { force: true }); + }; + + return { client, startedAt, initializedAt, modules, logs: () => [...logLines], release, close }; +} diff --git a/test/integration/edit-block-performance.js b/test/integration/edit-block-performance.js index adde4a2c7..a9343d4ab 100644 --- a/test/integration/edit-block-performance.js +++ b/test/integration/edit-block-performance.js @@ -16,6 +16,7 @@ import { exitProcess } from '../../dist/utils/exit-process.js'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; +import { callToolOnceLoaded } from '../helpers/heavy-packages.js'; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); @@ -59,7 +60,7 @@ function assertToolSuccess(result, message) { } async function callTool(client, name, args) { - return client.callTool({ name, arguments: args }, undefined, { timeout: 120000 }); + return callToolOnceLoaded(client, { name, arguments: args }, { requestOptions: { timeout: 120000 } }); } async function sleep(ms) { diff --git a/test/repro/test-pdf-launch-failure-server.js b/test/repro/test-pdf-launch-failure-server.js index 41d80d063..6323e67d7 100644 --- a/test/repro/test-pdf-launch-failure-server.js +++ b/test/repro/test-pdf-launch-failure-server.js @@ -22,6 +22,7 @@ import os from 'os'; import path from 'path'; import { fileURLToPath } from 'url'; import { closeClient } from '../helpers/close-client.js'; +import { callToolOnceLoaded } from '../helpers/heavy-packages.js'; import { exitProcess } from '../../dist/utils/exit-process.js'; const PROJECT_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); @@ -67,7 +68,7 @@ const remainingProfiles = () => [...profiles].filter((name) => fs.existsSync(pat let failed = false; try { - const result = await client.callTool({ + const result = await callToolOnceLoaded(client, { name: 'write_pdf', arguments: { path: path.join(tempDir, 'never-written.pdf'), diff --git a/test/repro/test-startup-heavy-imports.js b/test/repro/test-startup-heavy-imports.js new file mode 100644 index 000000000..809bbcde1 --- /dev/null +++ b/test/repro/test-startup-heavy-imports.js @@ -0,0 +1,82 @@ +// Repro (#715): Desktop Commander takes 25-90 s to answer the MCP `initialize` +// request on the reporter's Windows 11 machine (Node 24.14, 0.2.50 local +// clone), one core busy for ~25 s, and the client gives up; loading the +// Excel, PDF and DOCX packages on first use brought it to 8-10 s there. +// +// Every launch loads the packages that only reading, writing and rendering +// Excel, PDF and DOCX files need (exceljs, pdf-lib, md-to-pdf with puppeteer, +// unpdf, @opendocsg/pdf2md, pizzip), before it answers `initialize`, whether +// or not the session ever opens such a file: the server's startup modules +// import them at the top of the file. +// +// Here the built server is started the way a client starts it (MCP over +// stdio), with a preload that records every module it resolves (import and +// require). Each run reports how long `initialize` took, counted to the +// moment its answer arrived (a measurement, not a verdict), how many modules +// were loaded before the answer and how many of them came in through those +// packages. The server loads them right after `initialize`, in the background; +// that is test-startup-imports.js's to check. +// +// Run: node test/repro/run-repro.js test-startup-heavy-imports.js +// (REPRO_RUNS=5 starts by default) +// Exit code: 1 if any of those packages is loaded before the answer. +import { HEAVY_PACKAGES, packageOf, startServerRecordingModules } from '../helpers/server-modules.js'; +import { exitProcess } from '../../dist/utils/exit-process.js'; + +const RUNS = Number(process.env.REPRO_RUNS || 5); + +/** For each module, the heavy package it was first loaded through, if any */ +function heavyPackageBehind(modules) { + const byUrl = new Map(modules.map((module) => [module.url, module])); + const behind = new Map(); + const find = (module, depth = 0) => { + if (behind.has(module.url)) return behind.get(module.url); + const pkg = packageOf(module.url); + let result; + if (pkg && HEAVY_PACKAGES.includes(pkg)) result = pkg; + else if (pkg && module.parent && byUrl.has(module.parent) && depth < 200) result = find(byUrl.get(module.parent), depth + 1); + behind.set(module.url, result); + return result; + }; + for (const module of modules) find(module); + return behind; +} + +function describe(modules) { + const own = modules.filter((module) => !module.url.startsWith('node:')); + const behind = heavyPackageBehind(own); + const counts = {}; + for (const module of own) { + const pkg = behind.get(module.url); + if (pkg) counts[pkg] = (counts[pkg] ?? 0) + 1; + } + const heavy = Object.values(counts).reduce((sum, count) => sum + count, 0); + const detail = Object.entries(counts).map(([pkg, count]) => `${pkg} ${count}`).join(', '); + return `${own.length} modules, ${heavy ? `${heavy} of them through ${detail}` : 'none of them through those packages'}`; +} + +const heavyLoaded = (modules) => HEAVY_PACKAGES.filter((pkg) => modules.some((module) => packageOf(module.url) === pkg)); + +let runsLoadingHeavy = 0; +const startupMs = []; +for (let run = 1; run <= RUNS; run++) { + const server = await startServerRecordingModules(); + try { + // Before the answer arrived: the server's background load starts only after it + const beforeAnswer = server.modules().filter((module) => module.at < server.initializedAt); + const loaded = heavyLoaded(beforeAnswer); + if (loaded.length > 0) runsLoadingHeavy++; + startupMs.push(server.initializedAt - server.startedAt); + console.log(`start ${run}: initialize answered after ${server.initializedAt - server.startedAt} ms; loaded before it: ${describe(beforeAnswer)}`); + } finally { + await server.close(); + } +} + +const sorted = [...startupMs].sort((a, b) => a - b); +const median = sorted[Math.floor(sorted.length / 2)]; +const times = `initialize answered after ${startupMs.join(', ')} ms (median ${median} ms)`; +console.log(runsLoadingHeavy > 0 + ? `REPRODUCED: ${runsLoadingHeavy} of ${RUNS} starts loaded Excel/PDF/DOCX packages before answering initialize; ${times}` + : `NOT REPRODUCED: ${RUNS} starts, none loaded Excel/PDF/DOCX packages before answering initialize; ${times}`); +exitProcess(runsLoadingHeavy > 0 ? 1 : 0); diff --git a/test/test-client-results.js b/test/test-client-results.js index 953b0f2d4..c57e7c050 100644 --- a/test/test-client-results.js +++ b/test/test-client-results.js @@ -15,6 +15,7 @@ import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; import { runIfMain, skip } from './helpers/run-if-main.js'; import { closeClient } from './helpers/close-client.js'; +import { callToolOnceLoaded } from './helpers/heavy-packages.js'; const PROJECT_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); @@ -23,7 +24,7 @@ const textOf = (result) => result.content?.find((block) => block.type === 'text' /** write_pdf with an option Desktop Commander ignores: the answer is the plain success line */ async function writePdfIgnoringAnOption(client, dir) { const target = path.join(dir, 'ignored-option.pdf'); - const result = await client.callTool({ + const result = await callToolOnceLoaded(client, { name: 'write_pdf', arguments: { path: target, content: '# Client results\n', options: { devtools: true } }, }); diff --git a/test/test-docx-header-footer-edit.js b/test/test-docx-header-footer-edit.js index 78892b17d..ae14ce3d1 100644 --- a/test/test-docx-header-footer-edit.js +++ b/test/test-docx-header-footer-edit.js @@ -19,6 +19,7 @@ import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; import { runIfMain } from './helpers/run-if-main.js'; import { closeClient } from './helpers/close-client.js'; +import { callToolOnceLoaded } from './helpers/heavy-packages.js'; const PROJECT_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); const PizZip = createRequire(import.meta.url)('pizzip'); @@ -77,7 +78,7 @@ export default async function runTests() { try { await client.connect(transport, { timeout: 30_000 }); const file = path.join(dir, 'sections.docx'); - await client.callTool({ name: 'write_file', arguments: { path: file, content: '# Title\n\nBody text' } }); + await callToolOnceLoaded(client, { name: 'write_file', arguments: { path: file, content: '# Title\n\nBody text' } }); const texts = addSections(file); const outline = textOf(await client.callTool({ name: 'read_file', arguments: { path: file } })); diff --git a/test/test-search-office-completion.js b/test/test-search-office-completion.js index 01b76cb1e..19b113018 100644 --- a/test/test-search-office-completion.js +++ b/test/test-search-office-completion.js @@ -17,7 +17,6 @@ import { configManager } from '../dist/config-manager.js'; import { startSearchAndWait } from './helpers/search.js'; import { runIfMain } from './helpers/run-if-main.js'; import { runNode } from './helpers/run-node.js'; -import { hookArgs } from './helpers/module-hooks.js'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); const TEST_DIR = path.join(__dirname, 'search-office-completion-test'); @@ -189,19 +188,20 @@ async function testTimeoutStopsOfficeSearches() { * An Office search that fails as a whole (here: ExcelJS can't be loaded) must * not vanish: the search answers that part of it failed, with the other sources' * matches, and the log says which part failed and why. Runs in a child process - * whose search-manager can't import exceljs. + * where exceljs can't be loaded (the search loads it with require(), through + * exceljsPackage in utils/files/excel.js). */ async function testFailedOfficeSearchIsLogged() { console.log('Testing that a failed Office search is logged...'); const REASON = 'exceljs is unavailable in this test'; - const hooks = ` - export async function resolve(specifier, context, nextResolve) { - if (specifier === 'exceljs' && context.parentURL?.endsWith('/search-manager.js')) { - throw new Error(${JSON.stringify(REASON)}); - } - return nextResolve(specifier, context); - }`; + const preload = ` + import Module from 'node:module'; + const resolveFilename = Module._resolveFilename; + Module._resolveFilename = function (request, ...rest) { + if (request === 'exceljs') throw new Error(${JSON.stringify(REASON)}); + return resolveFilename.call(this, request, ...rest); + };`; const dist = (file) => pathToFileURL(path.join(__dirname, '..', 'dist', file)).href; const script = ` import { handleGetMoreSearchResults } from ${JSON.stringify(dist('handlers/search-handlers.js'))}; @@ -212,7 +212,7 @@ async function testFailedOfficeSearchIsLogged() { searchManager.dispose(); console.log(JSON.stringify({ sessionId, isError: !!page.isError, text: page.content[0].text }));`; const child = await runNode([ - ...hookArgs(`data:text/javascript,${encodeURIComponent(hooks)}`), '--input-type=module', '-e', script, + '--import', `data:text/javascript,${encodeURIComponent(preload)}`, '--input-type=module', '-e', script, ], { timeoutMs: 60000 }); assert.strictEqual(child.status, 0, `The search process failed (${child.status}): ${child.stderr}`); diff --git a/test/test-startup-imports.js b/test/test-startup-imports.js new file mode 100644 index 000000000..9598f4987 --- /dev/null +++ b/test/test-startup-imports.js @@ -0,0 +1,259 @@ +/** + * Starting the server must not load the packages only Excel, PDF and DOCX + * files need before it answers `initialize` (#715). Every launch loaded them + * first, whether or not the session ever opened such a file: 1,183 of the + * 1,556 modules loaded before the answer, which one reporter saw take 25-90 s + * and time the client out. Nor may a tool call wait for one to load: a client + * gives a call a few seconds (review on #777). So right after `initialize` the + * server loads them in the background, one at a time; a call that needs one + * still loading answers at once that it is still loading, and works once it + * is loaded. A load that fails is not kept: the call says so, and a later call + * loads it again. + * + * Starts the real server (dist/index.js) over MCP stdio, the way a client + * does, recording every module it resolves (import and require). + */ +import assert from 'assert'; +import ExcelJS from 'exceljs'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import { fileURLToPath } from 'url'; +import { HEAVY_PACKAGES, packageOf, startServerRecordingModules } from './helpers/server-modules.js'; +import { callToolOnceLoaded } from './helpers/heavy-packages.js'; +import { isTestHome } from './helpers/test-env.js'; +import { runIfMain, skip } from './helpers/run-if-main.js'; + +const PROJECT_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); +const SAMPLE_PDF = path.join(PROJECT_ROOT, 'test/samples/01_sample_simple.pdf'); +/** The background load takes a few seconds at most; far above that */ +const LOAD_DEADLINE_MS = 30_000; +/** An answer "at once": a refused call takes milliseconds; far above that */ +const AT_ONCE_MS = 5_000; + +const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); +const heavyLoaded = (modules) => HEAVY_PACKAGES.filter((pkg) => modules.some((module) => packageOf(module.url) === pkg)); + +function text(result) { + return (result.content ?? []).map((item) => item.text ?? '').join('\n'); +} + +async function callTool(client, name, args) { + const result = await callToolOnceLoaded(client, { name, arguments: args }); + assert.notStrictEqual(result.isError, true, `${name} ${JSON.stringify(args)} failed: ${text(result)}`); + return text(result); +} + +/** Nothing for Excel, PDF or DOCX is loaded before the server answers `initialize` */ +async function testNothingLoadedBeforeInitialize(server) { + // Before the answer arrived: the server starts its background load only after it + const beforeAnswer = server.modules().filter((module) => module.at < server.initializedAt && !module.url.startsWith('node:')); + const loaded = heavyLoaded(beforeAnswer); + assert.deepStrictEqual(loaded, [], + `the server loaded ${loaded.join(', ')} before answering initialize ` + + `(${beforeAnswer.length} modules resolved before the answer)`); + console.log(`✓ Nothing for Excel, PDF or DOCX loaded before initialize was answered (${beforeAnswer.length} modules)`); +} + +/** Shortly after it, all of them are loaded, without any tool call */ +async function testLoadedSoonAfterInitialize(server) { + const deadline = Date.now() + LOAD_DEADLINE_MS; + let missing = HEAVY_PACKAGES; + while (missing.length > 0 && Date.now() < deadline) { + await sleep(100); + const loaded = heavyLoaded(server.modules()); + missing = HEAVY_PACKAGES.filter((pkg) => !loaded.includes(pkg)); + } + assert.deepStrictEqual(missing, [], + `${LOAD_DEADLINE_MS / 1000} s after initialize, with no tool call, the server had not loaded ${missing.join(', ')}: ` + + 'the first call that needs them loads them inside the call, which a client gives only a few seconds'); + const lastAt = Math.max(...server.modules().filter((module) => HEAVY_PACKAGES.includes(packageOf(module.url))).map((module) => module.at)); + console.log(`✓ All of them loaded in the background, the last ${lastAt - server.initializedAt} ms after initialize was answered`); +} + +/** Excel, DOCX and PDF files work once loaded */ +async function testFilesWorkOnceLoaded(server, dir) { + const { client } = server; + const xlsx = path.join(dir, 'budget.xlsx'); + await callTool(client, 'write_file', { path: xlsx, content: JSON.stringify([['Item', 'Note'], ['Alice', 'ZebraQuartz budget']]) }); + assert(/ZebraQuartz budget/.test(await callTool(client, 'read_file', { path: xlsx })), + 'reading back a spreadsheet written through the server should show its cells'); + + const docx = path.join(dir, 'memo.docx'); + await callTool(client, 'write_file', { path: docx, content: 'Memo title\nThe ZebraQuartz review is due' }); + await callTool(client, 'edit_block', { file_path: docx, old_string: 'ZebraQuartz', new_string: 'ZebraQuokka' }); + assert(/ZebraQuokka review/.test(await callTool(client, 'read_file', { path: docx })), + 'reading back a DOCX written and edited through the server should show the edited text'); + + assert((await callTool(client, 'read_file', { path: SAMPLE_PDF })).trim().length > 0, + 'reading a PDF through the server should return its text'); + console.log('✓ Excel, DOCX and PDF files work once their support is loaded'); +} + +/** A call that needs a package still loading answers at once that it is still loading */ +async function testCallWhileLoadingAnswersAtOnce(server, file) { + const started = Date.now(); + const call = server.client.callTool({ name: 'read_file', arguments: { path: file } }, undefined, { timeout: 120_000 }); + const answer = await Promise.race([call, sleep(AT_ONCE_MS).then(() => null)]); + try { + assert(answer, + `with PDF reading support still loading, read_file of a PDF gave no answer within ${AT_ONCE_MS / 1000} s: ` + + 'it waited for unpdf to load inside the call, and a client gives a call only a few seconds'); + assert(answer.isError === true && /Can't read held\.pdf yet: Desktop Commander is still loading its PDF reading support/.test(text(answer)), + `with PDF reading support still loading, read_file of held.pdf should answer at once that it can't read held.pdf yet; it answered: ${text(answer)}`); + console.log(`✓ A call while PDF reading support is loading answers in ${Date.now() - started} ms: ${text(answer)}`); + } finally { + server.release(); + await call.catch(() => {}); + } +} + +/** read_multiple_files says so for each file that needs it, by its name */ +async function testReadMultipleFilesWhileLoading(server, file, second) { + const answer = text(await server.client.callTool({ name: 'read_multiple_files', arguments: { paths: [file, second] } })); + for (const name of [path.basename(file), path.basename(second)]) { + const escaped = name.replace(/\./g, '\\.'); + assert(new RegExp(`${escaped}: Error - Can't read ${escaped} yet: Desktop Commander is still loading its PDF reading support`).test(answer), + `with PDF reading support still loading, read_multiple_files should say for ${name} that it can't read ${name} yet; it answered: ${answer}`); + } + console.log('✓ read_multiple_files says for each file, by name, that it can\'t be read yet'); +} + +/** write_pdf with markdown that starts with a link is writing a PDF, not editing one, as the tool reads it */ +async function testWritePdfMarkdownStartingWithLink(server, dir) { + // PDF writing (md-to-pdf) failed to load once; PDF editing (pdf-lib) is loaded + const deadline = Date.now() + LOAD_DEADLINE_MS; + while (!server.logs().some((line) => /Loading md-to-pdf failed/.test(line)) && Date.now() < deadline) await sleep(100); + const answer = await server.client.callTool({ name: 'write_pdf', arguments: { path: path.join(dir, 'links.pdf'), content: '[Home](https://example.com)\n\n# Links' } }); + assert(answer.isError === true && /Can't write links\.pdf: Desktop Commander couldn't load its PDF writing support/.test(text(answer)), + `write_pdf with markdown starting with a link, while PDF writing can't be loaded, should say it can't write links.pdf ` + + `(markdown, as the tool reads it, not page edits); it answered: ${text(answer)}`); + console.log(`✓ write_pdf with markdown starting with a link is writing a PDF: ${text(answer)}`); +} + +/** The same call works once the package is loaded */ +async function testSameCallWorksOnceLoaded(server, file) { + assert((await callTool(server.client, 'read_file', { path: file })).trim().length > 0, + 'once PDF reading support has loaded, read_file of the PDF should return its text'); + console.log('✓ The same call works once PDF reading support has loaded'); +} + +/** A failed load isn't kept: the call says so, with the reason, and a later call loads it again */ +async function testFailedLoadIsNotKept(server, file) { + // Calls only once the background load has tried exceljs and logged the failure + const deadline = Date.now() + LOAD_DEADLINE_MS; + while (!server.logs().some((line) => /Loading exceljs failed/.test(line)) && Date.now() < deadline) await sleep(100); + const answer = await server.client.callTool({ name: 'read_file', arguments: { path: file } }); + assert(answer.isError === true && /Can't read held\.xlsx: Desktop Commander couldn't load its Excel support \(.*exceljs failed to load \(test\)/.test(text(answer)), + `when exceljs failed to load, read_file of held.xlsx should say it can't read held.xlsx as Excel support couldn't be loaded, and why; it answered: ${text(answer)}`); + assert(/ZebraQuartz held/.test(await callTool(server.client, 'read_file', { path: file })), + 'after a failed load, a later read_file of the .xlsx should load Excel support again and show its cells ' + + '(Node keeps a failed import() failed, so it has to be loaded with require())'); + console.log(`✓ A failed load isn't kept: "${text(answer)}", and a later call loads it`); +} + +/** A package Node can't load again (an import() that failed) says to restart, not to try again */ +async function testFailedImportSaysRestart(server, file) { + const deadline = Date.now() + LOAD_DEADLINE_MS; + while (!server.logs().some((line) => /Loading unpdf failed/.test(line)) && Date.now() < deadline) await sleep(100); + const answer = await server.client.callTool({ name: 'read_file', arguments: { path: file } }); + assert(answer.isError === true && + /Can't read held\.pdf: Desktop Commander couldn't load its PDF reading support \(.*unpdf failed to load \(test\)\)\. Restart Desktop Commander/.test(text(answer)), + 'when unpdf failed to load, read_file of held.pdf should say to restart Desktop Commander: Node keeps a failed import() failed, ' + + `so trying again can't work; it answered: ${text(answer)}`); + console.log(`✓ A failed import() says to restart: ${text(answer)}`); +} + +async function runCases(failures, cases) { + for (const [check, ...args] of cases) { + try { + await check(...args); + } catch (error) { + failures.push(error); + console.error(`❌ ${check.name}: ${error.message}`); + } + } +} + +export default async function runTests() { + if (!isTestHome()) { + skip('test-startup-imports.js writes to the home: run it through node test/run-all-tests.js'); + return true; + } + const dir = fs.mkdtempSync(path.join(os.homedir(), 'startup-imports-')); + const failures = []; + try { + const server = await startServerRecordingModules(); + try { + await runCases(failures, [ + [testNothingLoadedBeforeInitialize, server], + [testLoadedSoonAfterInitialize, server], + [testFilesWorkOnceLoaded, server, dir], + ]); + } finally { + await server.close(); + } + + // PDF reading support held back: unpdf (loaded with import()) doesn't load + // until the server is released + const heldPdf = path.join(dir, 'held.pdf'); + const heldPdfToo = path.join(dir, 'held-too.pdf'); + fs.copyFileSync(SAMPLE_PDF, heldPdf); + fs.copyFileSync(SAMPLE_PDF, heldPdfToo); + const holding = await startServerRecordingModules({ holdPackage: 'unpdf' }); + try { + await runCases(failures, [ + [testCallWhileLoadingAnswersAtOnce, holding, heldPdf], + [testSameCallWorksOnceLoaded, holding, heldPdf], + ]); + } finally { + await holding.close(); + } + // A server of its own: after a call that succeeds, the server loads modules + // of its own, which a held import would hold up (see startServerRecordingModules) + const holdingToo = await startServerRecordingModules({ holdPackage: 'unpdf' }); + try { + await runCases(failures, [[testReadMultipleFilesWhileLoading, holdingToo, heldPdf, heldPdfToo]]); + } finally { + await holdingToo.close(); + } + + // Excel support failing to load once + const held = path.join(dir, 'held.xlsx'); + const workbook = new ExcelJS.Workbook(); + workbook.addWorksheet('Sheet1').addRow(['Item', 'ZebraQuartz held']); + await workbook.xlsx.writeFile(held); + const failing = await startServerRecordingModules({ failPackageOnce: 'exceljs' }); + try { + await runCases(failures, [[testFailedLoadIsNotKept, failing, held]]); + } finally { + await failing.close(); + } + + // PDF reading support (unpdf, loaded with import()) failing to load once + const failingImport = await startServerRecordingModules({ failPackageOnce: 'unpdf' }); + try { + await runCases(failures, [[testFailedImportSaysRestart, failingImport, heldPdf]]); + } finally { + await failingImport.close(); + } + + // PDF writing failing to load once + const failingPdf = await startServerRecordingModules({ failPackageOnce: 'md-to-pdf' }); + try { + await runCases(failures, [[testWritePdfMarkdownStartingWithLink, failingPdf, dir]]); + } finally { + await failingPdf.close(); + } + } finally { + fs.rmSync(dir, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); + } + if (failures.length > 0) { + console.error(`❌ ${failures.length} startup import test(s) failed`); + return false; + } + console.log('✅ Startup import tests passed'); + return true; +} + +runIfMain(import.meta.url, runTests); diff --git a/test/test-still-loading-answers.js b/test/test-still-loading-answers.js new file mode 100644 index 000000000..5b6a4bbed --- /dev/null +++ b/test/test-still-loading-answers.js @@ -0,0 +1,103 @@ +/** + * What a tool call answers while the Excel, DOCX or PDF support it needs is + * still loading (#777), on the server in this process, where nothing has + * loaded that support yet: + * - the answer names the file, but no telemetry event does: the answer was + * also sent as the call's error, file name included, on every retry + * - read_file of a URL ending in .docx fetches the URL and never uses DOCX + * support, so it isn't refused while that support loads + * + * Telemetry is caught the way test-telemetry-paths.js catches it. + */ +import assert from 'assert'; +import { EventEmitter } from 'events'; +import http from 'http'; +import https from 'https'; +import { syncBuiltinESMExports } from 'module'; +import os from 'os'; +import path from 'path'; +import { runIfMain, skip } from './helpers/run-if-main.js'; +import { isTestHome } from './helpers/test-env.js'; + +// Every telemetry payload this process would send +const sent = []; +https.request = (options, callback) => { + const chunks = []; + const req = new EventEmitter(); + req.write = (data) => { chunks.push(String(data)); return true; }; + req.setTimeout = () => req; + req.destroy = () => {}; + req.end = () => { + sent.push(JSON.parse(chunks.join(''))); + const res = new EventEmitter(); + res.statusCode = 204; + res.resume = () => res; + callback?.(res); + setImmediate(() => res.emit('end')); + }; + return req; +}; +syncBuiltinESMExports(); +delete process.env.DESKTOP_COMMANDER_DISABLE_TELEMETRY; + +const { server } = await import('../dist/server.js'); + +const text = (result) => (result.content ?? []).map((item) => item.text ?? '').join('\n'); +const callTool = (name, args) => + server._requestHandlers.get('tools/call')({ method: 'tools/call', params: { name, arguments: args } }, {}); + +/** Waits until telemetry has sent an event named `name` (or 5 s) */ +async function waitForEvent(name) { + for (let waited = 0; waited < 5000; waited += 50) { + if (sent.some((payload) => payload.events.some((event) => event.name === name))) return; + await new Promise((resolve) => setTimeout(resolve, 50)); + } +} + +async function testUrlIsNotRefused() { + const urlServer = http.createServer((request, response) => { + response.writeHead(200, { 'Content-Type': 'text/plain' }).end('ZebraQuartz from a URL'); + }); + await new Promise((resolve) => urlServer.listen(0, '127.0.0.1', resolve)); + try { + const url = `http://127.0.0.1:${urlServer.address().port}/report.docx`; + const answer = await callTool('read_file', { path: url, isUrl: true }); + assert(/ZebraQuartz from a URL/.test(text(answer)), + `read_file of a URL ending in .docx, while DOCX support is still loading, should fetch the URL (it never uses DOCX support); it answered: ${text(answer)}`); + console.log('✓ read_file of a URL ending in .docx fetches it while DOCX support is still loading'); + } finally { + urlServer.close(); + } +} + +async function testFileNameNotInTelemetry() { + const answer = await callTool('read_file', { path: path.join(os.tmpdir(), 'salary-2026.xlsx') }); + assert(answer.isError === true && /Can't read salary-2026\.xlsx yet/.test(text(answer)), + `setup: with Excel support still loading, read_file of salary-2026.xlsx should say it can't read it yet; it answered: ${text(answer)}`); + await waitForEvent('server_call_tool'); + const leaking = sent.flatMap((payload) => payload.events).filter((event) => JSON.stringify(event).includes('salary-2026')); + assert.deepStrictEqual(leaking, [], + 'the answer that a file can\'t be read yet names the file, and telemetry got that name: ' + JSON.stringify(leaking)); + console.log('✓ The answer names the file; no telemetry event does'); +} + +async function run() { + if (!isTestHome()) { + return skip('still-loading answers: the server in this process writes its config to the home; run through the test runner'); + } + const failures = []; + // The URL first: each case uses support nothing else here has loaded + for (const test of [testUrlIsNotRefused, testFileNameNotInTelemetry]) { + try { + await test(); + } catch (error) { + failures.push(test.name); + console.log(`✗ ${test.name}: ${error.message}`); + } + } + return failures.length === 0; +} + +runIfMain(import.meta.url, run); + +export default run;