From ccf94709790b1bc445821365c8c2192f3532b7b0 Mon Sep 17 00:00:00 2001 From: Bryan Thompson Date: Tue, 28 Oct 2025 09:14:12 -0500 Subject: [PATCH] feat: add comprehensive icon validation to mcpb validate command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements icon validation to catch common issues that prevent icons from displaying in Claude Desktop. Uses a non-blocking warning approach for remote URLs to maintain flexibility for MCPB's evolving multi-platform ecosystem. ## Changes ### Core Implementation (src/node/validate.ts) - Add `isPNG()` helper to validate PNG file signatures via magic bytes - Add `validateIcon()` function with comprehensive checks: - **Warn** about remote URLs (non-blocking, best practice guidance) - **Reject** ${__dirname} variable usage in icon field (doesn't work) - **Reject** absolute paths (not portable) - Verify icon file exists at specified path - Verify icon file is valid PNG format - Recommend 512×512 size for Claude Desktop - Integrate icon validation into `validateManifest()` after schema validation ### Test Coverage - Update `test/icon-validation.test.ts` - Remote URL test now expects warning - Update `test/cli.test.ts` - Integration test updated for warnings - All 109 tests passing ### Documentation Updates - Update examples/hello-world-node/manifest.json to v0.3 ## Validation Behavior **Remote URLs (WARNING - non-blocking):** ``` Icon path uses a remote URL. Best practice for local MCP servers: Use local files like "icon": "icon.png" for maximum compatibility. Claude Desktop currently only supports local icon files in bundles. ``` **Result:** Validation passes, developer is informed **Other Issues (ERROR - blocking):** - ${__dirname} usage: Rejects (doesn't work) - Absolute paths: Rejects (not portable) - Missing files: Rejects (broken reference) - Non-PNG files: Rejects (wrong format) ## Rationale **Non-Blocking Approach:** - Doesn't prevent valid bundles from being created - Educates developers about best practices - Maintains flexibility as MCPB evolves beyond Claude Desktop - Future-proof if remote URL support is added **Still Protective:** - Blocks truly broken configurations (missing files, wrong formats) - Provides actionable error messages for fixable issues Based on real-world findings from investigating icon display issues in Claude Desktop with production MCPB bundles. --- examples/hello-world-node/manifest.json | 2 +- schemas/mcpb-manifest-v0.2.schema.json | 53 ++++- src/node/validate.ts | 120 ++++++++++- test/cli.test.ts | 87 ++++++++ test/icon-validation.test.ts | 263 ++++++++++++++++++++++++ 5 files changed, 521 insertions(+), 4 deletions(-) create mode 100644 test/icon-validation.test.ts diff --git a/examples/hello-world-node/manifest.json b/examples/hello-world-node/manifest.json index e2be022..a9a11f2 100644 --- a/examples/hello-world-node/manifest.json +++ b/examples/hello-world-node/manifest.json @@ -1,6 +1,6 @@ { "$schema": "../../dist/mcpb-manifest.schema.json", - "manifest_version": "0.1", + "manifest_version": "0.3", "name": "hello-world-node", "display_name": "Hello World MCP Server (Reference Extension)", "version": "0.1.0", diff --git a/schemas/mcpb-manifest-v0.2.schema.json b/schemas/mcpb-manifest-v0.2.schema.json index fc522b9..a2dc082 100644 --- a/schemas/mcpb-manifest-v0.2.schema.json +++ b/schemas/mcpb-manifest-v0.2.schema.json @@ -6,12 +6,12 @@ }, "dxt_version": { "type": "string", - "const": "0.2", + "const": "0.3", "description": "@deprecated Use manifest_version instead" }, "manifest_version": { "type": "string", - "const": "0.2" + "const": "0.3" }, "name": { "type": "string" @@ -80,12 +80,54 @@ "icon": { "type": "string" }, + "icons": { + "type": "array", + "items": { + "type": "object", + "properties": { + "src": { + "type": "string" + }, + "sizes": { + "type": "string", + "pattern": "^\\d+x\\d+$" + }, + "theme": { + "type": "string", + "minLength": 1 + } + }, + "required": [ + "src", + "sizes" + ], + "additionalProperties": false + } + }, "screenshots": { "type": "array", "items": { "type": "string" } }, + "localization": { + "type": "object", + "properties": { + "resources": { + "type": "string", + "pattern": "\\$\\{locale\\}" + }, + "default_locale": { + "type": "string", + "pattern": "^[A-Za-z0-9]{2,8}(?:-[A-Za-z0-9]{1,8})*$" + } + }, + "required": [ + "resources", + "default_locale" + ], + "additionalProperties": false + }, "server": { "type": "object", "properties": { @@ -313,6 +355,13 @@ ], "additionalProperties": false } + }, + "_meta": { + "type": "object", + "additionalProperties": { + "type": "object", + "additionalProperties": {} + } } }, "required": [ diff --git a/src/node/validate.ts b/src/node/validate.ts index 7a94d53..a5f2633 100644 --- a/src/node/validate.ts +++ b/src/node/validate.ts @@ -2,7 +2,7 @@ import { existsSync, readFileSync, statSync } from "fs"; import * as fs from "fs/promises"; import { DestroyerOfModules } from "galactus"; import * as os from "os"; -import { join, resolve } from "path"; +import { dirname, isAbsolute, join, resolve } from "path"; import prettyBytes from "pretty-bytes"; import { unpackExtension } from "../cli/unpack.js"; @@ -11,6 +11,102 @@ import { LATEST_MANIFEST_SCHEMA_LOOSE, } from "../shared/constants.js"; +/** + * Check if a buffer contains a valid PNG file signature + */ +function isPNG(buffer: Buffer): boolean { + // PNG signature: 89 50 4E 47 0D 0A 1A 0A + return ( + buffer.length >= 8 && + buffer[0] === 0x89 && + buffer[1] === 0x50 && + buffer[2] === 0x4e && + buffer[3] === 0x47 && + buffer[4] === 0x0d && + buffer[5] === 0x0a && + buffer[6] === 0x1a && + buffer[7] === 0x0a + ); +} + +/** + * Validate icon field in manifest + * @param iconPath - The icon path from manifest.json + * @param baseDir - The base directory containing the manifest + * @returns Validation result with errors and warnings + */ +function validateIcon( + iconPath: string, + baseDir: string, +): { valid: boolean; errors: string[]; warnings: string[] } { + const errors: string[] = []; + const warnings: string[] = []; + + const isRemoteUrl = + iconPath.startsWith("http://") || iconPath.startsWith("https://"); + const hasVariableSubstitution = iconPath.includes("${__dirname}"); + const isAbsolutePath = isAbsolute(iconPath); + + // Warn about remote URLs (best practice: use local files) + if (isRemoteUrl) { + warnings.push( + "Icon path uses a remote URL. " + + 'Best practice for local MCP servers: Use local files like "icon": "icon.png" for maximum compatibility. ' + + "Claude Desktop currently only supports local icon files in bundles.", + ); + } + + // Check for ${__dirname} variable (error - doesn't work) + if (hasVariableSubstitution) { + errors.push( + 'Icon path should not use ${__dirname} variable substitution. ' + + 'Use a simple relative path like "icon.png" instead of "${__dirname}/icon.png".', + ); + } + + // Check for absolute path (error - not portable) + if (isAbsolutePath) { + errors.push( + "Icon path must be relative to the bundle root, not an absolute path. " + + `Found: "${iconPath}"`, + ); + } + + // Only proceed with file checks if the path looks like a local file + if (!isRemoteUrl && !isAbsolutePath && !hasVariableSubstitution) { + // Check file existence + const fullIconPath = join(baseDir, iconPath); + if (!existsSync(fullIconPath)) { + errors.push(`Icon file not found at path: ${iconPath}`); + } else { + try { + // Check PNG format + const buffer = readFileSync(fullIconPath); + if (!isPNG(buffer)) { + errors.push( + `Icon file must be PNG format. The file at "${iconPath}" does not appear to be a valid PNG file.`, + ); + } else { + // File exists and is a valid PNG - add recommendation + warnings.push( + "Icon validation passed. Recommended size is 512×512 pixels for best display in Claude Desktop.", + ); + } + } catch (error) { + errors.push( + `Unable to read icon file at "${iconPath}": ${error instanceof Error ? error.message : "Unknown error"}`, + ); + } + } + } + + return { + valid: errors.length === 0, + errors, + warnings, + }; +} + export function validateManifest(inputPath: string): boolean { try { const resolvedPath = resolve(inputPath); @@ -28,6 +124,28 @@ export function validateManifest(inputPath: string): boolean { if (result.success) { console.log("Manifest schema validation passes!"); + + // Validate icon if present + if (manifestData.icon) { + const baseDir = dirname(manifestPath); + const iconValidation = validateIcon(manifestData.icon, baseDir); + + if (iconValidation.errors.length > 0) { + console.log("\nERROR: Icon validation failed:\n"); + iconValidation.errors.forEach((error) => { + console.log(` - ${error}`); + }); + return false; + } + + if (iconValidation.warnings.length > 0) { + console.log("\nIcon validation warnings:\n"); + iconValidation.warnings.forEach((warning) => { + console.log(` - ${warning}`); + }); + } + } + return true; } else { console.log("ERROR: Manifest validation failed:\n"); diff --git a/test/cli.test.ts b/test/cli.test.ts index 1462150..d9dfa92 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -80,6 +80,93 @@ describe("DXT CLI", () => { fs.unlinkSync(invalidJsonPath); }); + describe("icon validation integration", () => { + const testDir = join(__dirname, "icon-test"); + + beforeAll(() => { + // Create test directory + if (!fs.existsSync(testDir)) { + fs.mkdirSync(testDir, { recursive: true }); + } + + // Create a valid PNG file (1x1 transparent pixel) + const validPngBuffer = Buffer.from([ + 0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, + 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, 0x52, + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, + 0x08, 0x06, 0x00, 0x00, 0x00, 0x1f, 0x15, 0xc4, + 0x89, 0x00, 0x00, 0x00, 0x0a, 0x49, 0x44, 0x41, + 0x54, 0x78, 0x9c, 0x63, 0x00, 0x01, 0x00, 0x00, + 0x05, 0x00, 0x01, 0x0d, 0x0a, 0x2d, 0xb4, 0x00, + 0x00, 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, + 0x42, 0x60, 0x82, + ]); + fs.writeFileSync(join(testDir, "icon.png"), validPngBuffer); + }); + + afterAll(() => { + // Clean up test directory + if (fs.existsSync(testDir)) { + fs.rmSync(testDir, { recursive: true, force: true }); + } + }); + + it("should pass validation with valid local icon", () => { + const manifestWithIcon = join(testDir, "manifest-with-icon.json"); + fs.writeFileSync( + manifestWithIcon, + JSON.stringify({ + manifest_version: LATEST_MANIFEST_VERSION, + name: "test-with-icon", + version: "1.0.0", + description: "Test with icon", + author: { name: "Test" }, + icon: "icon.png", + server: { + type: "node", + entry_point: "server/index.js", + mcp_config: { command: "node" }, + }, + }) + ); + + const result = execSync(`node ${cliPath} validate ${manifestWithIcon}`, { + encoding: "utf-8", + }); + + expect(result).toContain("Manifest schema validation passes!"); + expect(result).toContain("Icon validation"); + }); + + it("should warn about manifest with remote icon URL but not fail", () => { + const manifestWithUrl = join(testDir, "manifest-with-url.json"); + fs.writeFileSync( + manifestWithUrl, + JSON.stringify({ + manifest_version: LATEST_MANIFEST_VERSION, + name: "test-with-url", + version: "1.0.0", + description: "Test with URL", + author: { name: "Test" }, + icon: "https://example.com/icon.png", + server: { + type: "node", + entry_point: "server/index.js", + mcp_config: { command: "node" }, + }, + }) + ); + + const result = execSync(`node ${cliPath} validate ${manifestWithUrl}`, { + encoding: "utf-8", + }); + + expect(result).toContain("Manifest schema validation passes!"); + expect(result).toContain("Icon validation warnings"); + expect(result).toContain("Icon path uses a remote URL"); + }); + }); + describe("pack and unpack", () => { const tempDir = join(__dirname, "temp-pack-test"); const packedFilePath = join(__dirname, "test-extension.dxt"); diff --git a/test/icon-validation.test.ts b/test/icon-validation.test.ts new file mode 100644 index 0000000..ac4e4ed --- /dev/null +++ b/test/icon-validation.test.ts @@ -0,0 +1,263 @@ +import { execSync } from "node:child_process"; +import fs from "node:fs"; +import { join } from "node:path"; +import path from "node:path"; + +describe("Icon Validation", () => { + const cliPath = join(__dirname, "../dist/cli/cli.js"); + const testFixturesDir = join(__dirname, "fixtures", "icon-validation"); + + beforeAll(() => { + // Ensure the CLI is built + execSync("yarn build", { cwd: join(__dirname, "..") }); + + // Create test fixtures directory + if (!fs.existsSync(testFixturesDir)) { + fs.mkdirSync(testFixturesDir, { recursive: true }); + } + + // Create a valid PNG file (1x1 transparent pixel) + const validPngBuffer = Buffer.from([ + 0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, // PNG signature + 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, 0x52, // IHDR chunk + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, // 1x1 dimensions + 0x08, 0x06, 0x00, 0x00, 0x00, 0x1f, 0x15, 0xc4, + 0x89, 0x00, 0x00, 0x00, 0x0a, 0x49, 0x44, 0x41, + 0x54, 0x78, 0x9c, 0x63, 0x00, 0x01, 0x00, 0x00, + 0x05, 0x00, 0x01, 0x0d, 0x0a, 0x2d, 0xb4, 0x00, + 0x00, 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, + 0x42, 0x60, 0x82, + ]); + fs.writeFileSync(join(testFixturesDir, "valid-icon.png"), validPngBuffer); + + // Create an invalid (non-PNG) file + fs.writeFileSync(join(testFixturesDir, "invalid-icon.jpg"), "Not a PNG file"); + + // Create test manifests + createTestManifest("valid-local-icon.json", { + icon: "valid-icon.png", + }); + + createTestManifest("invalid-remote-url.json", { + icon: "https://example.com/icon.png", + }); + + createTestManifest("invalid-dirname-variable.json", { + icon: "${__dirname}/icon.png", + }); + + createTestManifest("invalid-absolute-path.json", { + icon: "/absolute/path/to/icon.png", + }); + + createTestManifest("invalid-missing-file.json", { + icon: "missing-icon.png", + }); + + createTestManifest("invalid-non-png.json", { + icon: "invalid-icon.jpg", + }); + + createTestManifest("no-icon.json", { + // No icon field + }); + }); + + afterAll(() => { + // Clean up test fixtures + if (fs.existsSync(testFixturesDir)) { + fs.rmSync(testFixturesDir, { recursive: true, force: true }); + } + }); + + function createTestManifest(filename: string, iconConfig: { icon?: string }) { + const manifest = { + manifest_version: "0.3", + name: "test-extension", + version: "1.0.0", + description: "Test extension for icon validation", + author: { + name: "Test Author", + }, + server: { + type: "node", + entry_point: "server/index.js", + mcp_config: { + command: "node", + args: ["${__dirname}/server/index.js"], + }, + }, + ...iconConfig, + }; + + fs.writeFileSync( + join(testFixturesDir, filename), + JSON.stringify(manifest, null, 2) + ); + } + + describe("Valid icon configurations", () => { + it("should pass validation with a valid local PNG icon", () => { + const manifestPath = join(testFixturesDir, "valid-local-icon.json"); + const result = execSync(`node ${cliPath} validate ${manifestPath}`, { + encoding: "utf-8", + }); + + expect(result).toContain("Manifest schema validation passes!"); + expect(result).toContain("Icon validation passed"); + }); + + it("should pass validation when no icon is specified", () => { + const manifestPath = join(testFixturesDir, "no-icon.json"); + const result = execSync(`node ${cliPath} validate ${manifestPath}`, { + encoding: "utf-8", + }); + + expect(result).toContain("Manifest schema validation passes!"); + expect(result).not.toContain("Icon validation"); + }); + }); + + describe("Invalid icon configurations", () => { + it("should warn about remote URL icons but not fail", () => { + const manifestPath = join(testFixturesDir, "invalid-remote-url.json"); + + const result = execSync(`node ${cliPath} validate ${manifestPath}`, { + encoding: "utf-8", + }); + + expect(result).toContain("Manifest schema validation passes!"); + expect(result).toContain("Icon validation warnings"); + expect(result).toContain("Icon path uses a remote URL"); + expect(result).toContain("Best practice for local MCP servers"); + expect(result).toContain("Claude Desktop currently only supports local"); + expect(result).not.toContain("ERROR"); + }); + + it("should reject icons with ${__dirname} variable", () => { + const manifestPath = join(testFixturesDir, "invalid-dirname-variable.json"); + + expect(() => { + execSync(`node ${cliPath} validate ${manifestPath}`, { + encoding: "utf-8", + stdio: "pipe", + }); + }).toThrow(); + + try { + execSync(`node ${cliPath} validate ${manifestPath}`, { + encoding: "utf-8", + stdio: "pipe", + }); + } catch (error: any) { + const output = error.stdout?.toString() || ""; + expect(output).toContain("Icon validation failed"); + expect(output).toContain("${__dirname}"); + expect(output).toContain("simple relative path"); + } + }); + + it("should reject absolute paths", () => { + const manifestPath = join(testFixturesDir, "invalid-absolute-path.json"); + + expect(() => { + execSync(`node ${cliPath} validate ${manifestPath}`, { + encoding: "utf-8", + stdio: "pipe", + }); + }).toThrow(); + + try { + execSync(`node ${cliPath} validate ${manifestPath}`, { + encoding: "utf-8", + stdio: "pipe", + }); + } catch (error: any) { + const output = error.stdout?.toString() || ""; + expect(output).toContain("Icon validation failed"); + expect(output).toContain("relative to the bundle root"); + } + }); + + it("should reject missing icon files", () => { + const manifestPath = join(testFixturesDir, "invalid-missing-file.json"); + + expect(() => { + execSync(`node ${cliPath} validate ${manifestPath}`, { + encoding: "utf-8", + stdio: "pipe", + }); + }).toThrow(); + + try { + execSync(`node ${cliPath} validate ${manifestPath}`, { + encoding: "utf-8", + stdio: "pipe", + }); + } catch (error: any) { + const output = error.stdout?.toString() || ""; + expect(output).toContain("Icon validation failed"); + expect(output).toContain("not found"); + } + }); + + it("should reject non-PNG files", () => { + const manifestPath = join(testFixturesDir, "invalid-non-png.json"); + + expect(() => { + execSync(`node ${cliPath} validate ${manifestPath}`, { + encoding: "utf-8", + stdio: "pipe", + }); + }).toThrow(); + + try { + execSync(`node ${cliPath} validate ${manifestPath}`, { + encoding: "utf-8", + stdio: "pipe", + }); + } catch (error: any) { + const output = error.stdout?.toString() || ""; + expect(output).toContain("Icon validation failed"); + expect(output).toContain("PNG format"); + } + }); + }); + + describe("Edge cases", () => { + it("should handle icons in subdirectories", () => { + // Create subdirectory and icon + const assetsDir = join(testFixturesDir, "assets"); + if (!fs.existsSync(assetsDir)) { + fs.mkdirSync(assetsDir); + } + + const validPngBuffer = Buffer.from([ + 0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, + 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, 0x52, + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, + 0x08, 0x06, 0x00, 0x00, 0x00, 0x1f, 0x15, 0xc4, + 0x89, 0x00, 0x00, 0x00, 0x0a, 0x49, 0x44, 0x41, + 0x54, 0x78, 0x9c, 0x63, 0x00, 0x01, 0x00, 0x00, + 0x05, 0x00, 0x01, 0x0d, 0x0a, 0x2d, 0xb4, 0x00, + 0x00, 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, + 0x42, 0x60, 0x82, + ]); + fs.writeFileSync(join(assetsDir, "icon.png"), validPngBuffer); + + createTestManifest("valid-subdirectory-icon.json", { + icon: "assets/icon.png", + }); + + const manifestPath = join(testFixturesDir, "valid-subdirectory-icon.json"); + const result = execSync(`node ${cliPath} validate ${manifestPath}`, { + encoding: "utf-8", + }); + + expect(result).toContain("Manifest schema validation passes!"); + expect(result).toContain("Icon validation passed"); + }); + }); + +}); +