diff --git a/src/__tests__/comfyui-path-trim.test.ts b/src/__tests__/comfyui-path-trim.test.ts new file mode 100644 index 000000000..dd6fc6800 --- /dev/null +++ b/src/__tests__/comfyui-path-trim.test.ts @@ -0,0 +1,268 @@ +// #1512 — COMFYUI_PATH was consumed exactly as given, so ONE trailing space made +// every install-root check miss and the connected ComfyUI was reported as +// undeterminable — 40 minutes after the bad value took effect, at the first write, +// with a message that echoed the path back but never pointed at the space. +// +// The value is trivially easy to produce. cmd.exe assigns everything up to the +// `&&`, INCLUDING the space before it: +// +// cmd /k "set COMFYUI_PATH=E:\...\ComfyUI && comfyui-mcp connect ..." +// +// so the launcher line people actually paste bakes one in. The panel pack already +// stripped it (`__init__.py`); the orchestrator did not — the two halves of one +// product disagreeing is the defect. +// +// THE TRAP THIS FILE GUARDS. The report calls `resolveComfyUIPath` "the single +// ingestion point". There are FIVE non-test readers, and a fix confined to the +// first is not just incomplete — it makes one case WORSE: extra-paths.ts compares +// the raw env against `config.comfyuiPath`, so normalizing only the latter turns +// an accidental match into a mismatch and silently reclassifies an explicitly +// named root as "inferred". The source rule below exists because four of the five +// sit where a unit test cannot cheaply reach, and every failure mode is silence. +// +// THE REPAIR IS A FALLBACK, NOT A CLEANUP. Trailing whitespace and quotes are +// legal POSIX filename characters, and a directory literally named `ComfyUI ` is +// creatable on Windows too (measured). So a value that RESOLVES as given is never +// touched. That still fixes the report: measured on win32, `existsSync(" ")` +// is false and `join(" ", "main.py")` does not resolve either — which is +// exactly why every install-root check missed. +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { dirname, join } from "node:path"; + +import { normalizeInstallPathEnv, __resetMalformedPathWarnings } from "../utils/install-path-env.js"; + +/** Normalize with the on-disk check stubbed OUT — the value names nothing, which + * is the only state the repair is allowed to act on. */ +const norm = (raw: string | undefined, warn = false) => + normalizeInstallPathEnv(raw, { exists: () => false, warn }); + +const HERE = dirname(fileURLToPath(import.meta.url)); +const SRC = join(HERE, ".."); + +beforeEach(() => { + __resetMalformedPathWarnings(); +}); + +describe("normalizeInstallPathEnv (#1512)", () => { + it("strips the trailing space cmd.exe bakes into `set VAR=v && cmd`", () => { + const out = norm("E:\\Ai_server\\ComfyUI_windows_portable\\ComfyUI "); + expect(out.path).toBe("E:\\Ai_server\\ComfyUI_windows_portable\\ComfyUI"); + expect(out.changed).toBe(true); + }); + + it("strips a MATCHED surrounding quote pair, the other paste artifact", () => { + expect(norm('"C:\\ComfyUI"').path).toBe("C:\\ComfyUI"); + expect(norm("'C:\\ComfyUI'").path).toBe("C:\\ComfyUI"); + // Quote OUTSIDE the space and space INSIDE the quote both normalize. + expect(norm(' "C:\\ComfyUI " ').path).toBe("C:\\ComfyUI"); + }); + + it("leaves a LONE trailing quote alone", () => { + // `"` is illegal in a Windows filename but LEGAL on POSIX. Stripping one + // unconditionally would corrupt a real path in order to fix a typo — the + // repair must not be able to do more damage than the bug. + expect(norm('/srv/weird"').path).toBe('/srv/weird"'); + expect(norm('/srv/weird"').changed).toBe(false); + expect(norm("'/srv/half").path).toBe("'/srv/half"); + }); + + it("treats a whitespace-only value as UNSET so detection still runs", () => { + // Adopting " " as a path would be worse than the bug: it defeats + // auto-detection AND cannot work. Both call sites truthy-check the result. + expect(norm(" ").path).toBeUndefined(); + expect(norm('" "').path).toBeUndefined(); + expect(norm("").path).toBeUndefined(); + expect(norm(undefined).path).toBeUndefined(); + }); + + it("reports changed:false for an already-clean value", () => { + const out = norm("/opt/ComfyUI"); + expect(out.path).toBe("/opt/ComfyUI"); + expect(out.changed).toBe(false); + }); + + it("does NO filesystem probe for a value it cannot change (codex P2)", () => { + // Five readers call this, some of them hot, and it replaced a plain env read. + // A stat per call would be new synchronous I/O on every one — and on a UNC or + // network root that call can block. The existence question only matters when + // the repair would change something, so it must not be asked otherwise. + // Counted, because "we only probe when needed" is the kind of claim that + // silently stops being true. + const probed: string[] = []; + const spy = (p: string) => { + probed.push(p); + return false; + }; + + normalizeInstallPathEnv("/opt/ComfyUI", { exists: spy, warn: false }); + normalizeInstallPathEnv("", { exists: spy, warn: false }); + normalizeInstallPathEnv(undefined, { exists: spy, warn: false }); + expect(probed).toEqual([]); + + // ...and exactly one probe when it WOULD change the value, since that is the + // only case where the answer decides anything. + normalizeInstallPathEnv("/opt/ComfyUI ", { exists: spy, warn: false }); + expect(probed).toEqual(["/opt/ComfyUI "]); + }); +}); + +describe("the malformed value is REPORTED, not silently repaired (#1512)", () => { + let errs: string[]; + let spy: ReturnType; + + beforeEach(() => { + errs = []; + spy = vi.spyOn(console, "error").mockImplementation((...a: unknown[]) => { + errs.push(a.join(" ")); + }); + }); + afterEach(() => spy.mockRestore()); + + it("names the variable, both values, and the launcher line that produced it", () => { + norm("C:\\ComfyUI ", true); + const msg = errs.join("\n"); + + expect(msg).toMatch(/COMFYUI_PATH/); + // JSON-quoted so the offending space is VISIBLE — the original error echoed + // the path bare, which is precisely why the space went unnoticed. + expect(msg).toMatch(/"C:\\\\ComfyUI "/); + expect(msg).toMatch(/&&/); + expect(msg).toMatch(/fix the launcher line/i); + }); + + it("warns ONCE per distinct value — retarget re-resolves on every switch", () => { + norm("C:\\ComfyUI ", true); + norm("C:\\ComfyUI ", true); + norm("D:\\Other ", true); + expect(errs.length).toBe(2); + }); + + it("says nothing when the value was already clean", () => { + norm("/opt/ComfyUI", true); + expect(errs).toHaveLength(0); + }); + + it("says nothing when the value RESOLVES as given — nothing was repaired", () => { + // The non-destructive guard's own half: a directory literally named with a + // trailing space is left untouched, so there is no repair to report either. + normalizeInstallPathEnv("/srv/ComfyUI ", { exists: () => true, warn: true }); + expect(errs).toHaveLength(0); + }); +}); + +/** Rebuild the config module against a specific COMFYUI_PATH. `config` is a + * module-level const evaluated at import time, so the env must be set BEFORE + * the import — which is exactly how the real process sees it. */ +async function comfyuiPathFor(raw: string | undefined): Promise { + const prev = process.env.COMFYUI_PATH; + const prevUrl = process.env.COMFYUI_URL; + vi.resetModules(); + if (raw === undefined) delete process.env.COMFYUI_PATH; + else process.env.COMFYUI_PATH = raw; + // Keep detection out of it: an unset URL is fine, but a stray remote URL from + // another test would send resolveComfyUIPath down its remote branch. + delete process.env.COMFYUI_URL; + try { + const mod = (await import("../config.js")) as { config: { comfyuiPath?: string } }; + return mod.config.comfyuiPath; + } finally { + if (prev === undefined) delete process.env.COMFYUI_PATH; + else process.env.COMFYUI_PATH = prev; + if (prevUrl === undefined) delete process.env.COMFYUI_URL; + else process.env.COMFYUI_URL = prevUrl; + vi.resetModules(); + } +} + +describe("the WIRING — a real config build normalizes the env (#1512)", () => { + it("the reporter's exact value no longer reaches config.comfyuiPath", async () => { + // Not the helper in isolation: this is the module-level `config` the whole + // server reads, built from process.env the way the real process builds it. + const dirty = "E:\\Ai_server\\ComfyUI_windows_portable\\ComfyUI "; + expect(await comfyuiPathFor(dirty)).toBe("E:\\Ai_server\\ComfyUI_windows_portable\\ComfyUI"); + }); + + it("a quoted value is unwrapped", async () => { + expect(await comfyuiPathFor('"C:\\ComfyUI"')).toBe("C:\\ComfyUI"); + }); +}); + +describe("NO reader of COMFYUI_PATH consumes it raw (#1512)", () => { + // The report calls resolveComfyUIPath "the single ingestion point". It is not. + // There are FIVE non-test readers, and the three past the obvious two are the + // reason this is a source-level rule rather than a couple of unit tests: + // + // - orchestrator/index.ts → feeds the spawn env builders, so a bad value + // reaches every agent this orchestrator starts + // - panel-tools.ts → joins it into the workflows dir, which then + // silently does not exist (library reads empty) + // - extra-paths.ts → compares it against config.comfyuiPath, which + // IS normalized; leaving this side raw makes a + // named root reclassify as "inferred" + // - workspace-env.ts → labels the workspace source + // + // Each sits somewhere a unit test cannot cheaply reach, and the failure mode is + // silence in every case. So the invariant is enforced where it can be seen. + const FILES = [ + ["orchestrator", "index.ts"], + ["orchestrator", "panel-tools.ts"], + ["services", "extra-paths.ts"], + ["services", "workspace-env.ts"], + ["config.ts"], + ].map((p) => join(SRC, ...p)); + + it("every raw read is normalized within 3 lines", () => { + let totalReads = 0; + const offenders: string[] = []; + + for (const file of FILES) { + const lines = readFileSync(file, "utf8").split(/\r?\n/); + lines.forEach((line, i) => { + // Skip COMMENTS: several of these files mention the variable by name in + // prose without reading it, and counting those makes the rule fire on + // documentation. Both forms matter — `//` line comments and the ` * ` + // continuation lines of a JSDoc block, which is what a first version of + // this scan missed (extra-paths.ts:279 explains the discriminator in + // exactly those words). + const trimmed = line.trim(); + if (trimmed.startsWith("*") || trimmed.startsWith("/*") || trimmed.startsWith("//")) return; + const code = line.replace(/\/\/.*$/, ""); + if (!/process\.env\.COMFYUI_PATH/.test(code)) return; + // EVERY occurrence on the line must be an argument — counted, not merely + // "the line contains a safe-looking call" (codex P2, twice). + // + // A proximity rule passes when the normalized result is discarded and the + // raw variable is forwarded anyway. A line-level rule passes on + // + // const p = normalizeInstallPathEnv(process.env.COMFYUI_PATH).path; f(process.env.COMFYUI_PATH); + // + // because the first occurrence exempts the second. Comparing counts is + // what actually encodes "no raw read survives": the guarded tally has to + // account for all of them. + const occurrences = (code.match(/process\.env\.COMFYUI_PATH/g) ?? []).length; + const guarded = ( + code.match(/(?:normalizeInstallPathEnv|resolveComfyUIPath)\(\s*process\.env\.COMFYUI_PATH/g) ?? + [] + ).length; + totalReads += occurrences; + if (guarded === occurrences) return; + offenders.push( + `${file.replace(SRC, "src")}:${i + 1} (${guarded}/${occurrences} consumed) ${line.trim()}`, + ); + }); + } + + // The premise. If the scan finds nothing the rule is vacuous and this test is + // a rubber stamp that would keep passing after someone renames the variable. + expect(totalReads).toBeGreaterThanOrEqual(5); + + expect( + offenders, + `These read process.env.COMFYUI_PATH without normalizing it. A trailing space ` + + `(Windows \`set VAR=v && cmd\`) then fails silently at each one (#1512):\n` + + offenders.join("\n"), + ).toEqual([]); + }); +}); diff --git a/src/config.ts b/src/config.ts index 6e49ccfd1..65227afd8 100644 --- a/src/config.ts +++ b/src/config.ts @@ -4,6 +4,7 @@ import { dirname, resolve, join } from "path"; import { chmodSync, copyFileSync, existsSync, mkdirSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { homedir } from "node:os"; import { isIP } from "node:net"; +import { normalizeInstallPathEnv } from "./utils/install-path-env.js"; import { parseComfyUIUrl, type ComfyUITarget } from "./transport/comfyui-url.js"; import { resetManagerApiCache } from "./services/manager-api-cache.js"; import { comfyuiEnvFilePath, freshSecretValue, loadEnvFileIntoProcess } from "./env-file.js"; @@ -249,9 +250,12 @@ export function isLoopbackHost(host: string | undefined): boolean { * COMFYUI_PATH env var still wins. */ function resolveComfyUIPath( - envPath: string | undefined, + rawEnvPath: string | undefined, opts: { remoteUrl: boolean; cloud: boolean; remoteHost?: string }, ): string | undefined { + // #1512 — normalize BEFORE the truthy check, so a whitespace-only value falls + // through to auto-detection instead of being adopted as a real (unusable) path. + const { path: envPath } = normalizeInstallPathEnv(rawEnvPath); if (envPath) { if (opts.remoteUrl) { console.error( diff --git a/src/orchestrator/index.ts b/src/orchestrator/index.ts index b016a7c8a..3a4af6103 100644 --- a/src/orchestrator/index.ts +++ b/src/orchestrator/index.ts @@ -138,6 +138,7 @@ import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; import { registerAllTools } from "../tools/index.js"; import { tryInstallRetiredNameRedirect } from "../tools/retired-redirect.js"; import { isForceRemoteFlagSet, isLoopbackHost, detectLocalComfyUIPath, setComfyuiTarget, onComfyuiTargetChanged, isTargetingLocal, isTargetingLocalOrLan, isTargetingPod, getComfyUIBaseUrl, getLocalComfyuiUrl, rescopeLocalTargetFile, getComfyUIAuthHeaders } from "../config.js"; +import { normalizeInstallPathEnv } from "../utils/install-path-env.js"; import { buildComfyuiMcpEnv, comfyuiSecretKeys, @@ -1308,9 +1309,18 @@ export async function runPanelOrchestrator(): Promise { // orchestrator previously read ONLY the env var, so a Desktop user without // COMFYUI_PATH always landed in "local install/pack tools limited" even with // a local install the MCP itself could find. - const envComfyuiPath = process.env.COMFYUI_PATH; + // #1512 — the SECOND ingestion point, and the one a fix confined to + // resolveComfyUIPath would have missed: this reads the env var directly, and + // what it produces is handed to the spawn env builders and to + // resolveComfyuiPathForTarget. A trailing space here does not merely fail a + // check locally — it is passed on to every agent this orchestrator starts. + // Same normalizer as config.ts so the two can never drift apart, which is the + // shape of the original bug (panel stripped it, orchestrator did not). + const envComfyuiPath = normalizeInstallPathEnv(process.env.COMFYUI_PATH).path; // `||` not `??`: a set-but-empty COMFYUI_PATH= means "unset" (the headless // MCP's config truthy-checks it the same way) — it must not block detection. + // normalizeInstallPathEnv already maps a whitespace-only value to undefined, + // so " " now reaches detection too instead of being adopted as a path. const localComfyuiPath = envComfyuiPath || detectLocalComfyUIPath(); const isLoopbackUrl = (u: string): boolean => { try { diff --git a/src/orchestrator/panel-tools.ts b/src/orchestrator/panel-tools.ts index 44b60497d..991123257 100644 --- a/src/orchestrator/panel-tools.ts +++ b/src/orchestrator/panel-tools.ts @@ -162,6 +162,7 @@ import { getComfyUIBaseUrl, getComfyuiTargetGeneration, } from "../config.js"; +import { normalizeInstallPathEnv } from "../utils/install-path-env.js"; import { sliceWorkflow } from "../services/workflow-slicer.js"; import { validateA2UISpecServer } from "../services/a2ui-spec.js"; import type { UiWorkflow } from "../comfyui/types.js"; @@ -5554,7 +5555,10 @@ function readPackWorkflow(packName: string): Record { * an absolute path. */ function comfyWorkflowsDirs(): string[] { - const base = process.env.COMFYUI_PATH; + // #1512 — a trailing space here does not fail loudly: it silently builds + // ` /user/default/workflows`, a directory that does not exist, so the + // workflow library simply appears empty and every lookup misses. + const base = normalizeInstallPathEnv(process.env.COMFYUI_PATH).path; if (!base) return []; return [ join(base, "user", "default", "workflows"), diff --git a/src/services/extra-paths.ts b/src/services/extra-paths.ts index b612a2b73..99233151d 100644 --- a/src/services/extra-paths.ts +++ b/src/services/extra-paths.ts @@ -4,6 +4,7 @@ import { homedir, platform } from "node:os"; import { dirname, join, isAbsolute, resolve } from "node:path"; import { parse as parseYaml, stringify as stringifyYaml } from "yaml"; import { config, isRemoteMode } from "../config.js"; +import { normalizeInstallPathEnv } from "../utils/install-path-env.js"; import { parseExtraModelPathsConfigsFromArgvRaw, type LiveServerSnapshot, @@ -404,7 +405,14 @@ function standaloneRoot(): { // an env var naming a Desktop-installer WRAPPER yields `/ComfyUI` — a path // this process INFERRED, which can vanish while the wrapper survives (codex round 4). // Anything inferred is gated exactly like the saved default workspace. - const envPath = process.env.COMFYUI_PATH; + // #1512 — normalized, and NOT optional here. This compares against + // `config.comfyuiPath`, which is normalized at ingestion; leaving this side raw + // would make a value with a trailing space fail `samePath` and silently + // reclassify an explicitly-named root as "comfyui-path-inferred" — a DIFFERENT, + // gated branch. Before the trim both sides were equally malformed and matched by + // accident, so normalizing only the other side would have introduced that + // divergence rather than fixed it. + const envPath = normalizeInstallPathEnv(process.env.COMFYUI_PATH).path; const source: StandaloneRootSource = !config.comfyuiPath ? "default-workspace" : envPath && samePath(config.comfyuiPath, envPath) diff --git a/src/services/workspace-env.ts b/src/services/workspace-env.ts index ce4eba35b..7516e19b6 100644 --- a/src/services/workspace-env.ts +++ b/src/services/workspace-env.ts @@ -5,6 +5,7 @@ import { homedir, platform } from "node:os"; import { basename, dirname, isAbsolute, join, resolve as pathResolve, sep } from "node:path"; import { promisify } from "node:util"; import { config, getComfyUIBaseUrl, isRemoteMode } from "../config.js"; +import { normalizeInstallPathEnv } from "../utils/install-path-env.js"; import { getSystemStats } from "../comfyui/client.js"; import { resolveLiveInterpreter } from "./live-interpreter.js"; import { logger } from "../utils/logger.js"; @@ -375,7 +376,9 @@ export async function getWorkspace(): Promise { let source: WorkspaceInfo["workspace_source"]; if (config.comfyuiPath) { // config.comfyuiPath is COMFYUI_PATH env or auto-detection - source = process.env.COMFYUI_PATH ? "env" : "auto-detected"; + // #1512 — normalized so a whitespace-only value is not reported as "env" + // while config.comfyuiPath actually came from auto-detection. + source = normalizeInstallPathEnv(process.env.COMFYUI_PATH).path ? "env" : "auto-detected"; } else if (cfg.defaultWorkspace) { source = "default-config"; } else { diff --git a/src/utils/install-path-env.ts b/src/utils/install-path-env.ts new file mode 100644 index 000000000..9865b3e01 --- /dev/null +++ b/src/utils/install-path-env.ts @@ -0,0 +1,138 @@ +// #1512 — install-root path normalization, kept OUT of config.ts on purpose. +// +// config.ts builds its module-level `config` at IMPORT time (env reads, install +// detection), which is why so many suites mock it wholesale. Putting a pure +// string helper behind that module forced every one of those partial mocks to +// grow a new export — 46 tests went red on the first attempt for exactly that +// reason, none of them a product defect. A leaf module with one node:fs import +// has no such gravity. +import { existsSync } from "node:fs"; +/** + * #1512 — normalize an install-root path arriving from the ENVIRONMENT. + * + * `COMFYUI_PATH` was consumed exactly as given, so one trailing space made every + * install-root check miss and the connected ComfyUI was reported as + * undeterminable — 40 minutes after the bad value took effect, at the first + * write, with a message that echoed the path but never pointed at the space. + * + * That value is trivially easy to produce on Windows. `cmd.exe` assigns + * everything up to the `&&`, INCLUDING the space before it: + * + * cmd /k "set COMFYUI_PATH=E:\...\ComfyUI && comfyui-mcp connect ..." + * + * so the launcher line people actually paste bakes in a trailing space. The + * panel pack already strips this (`__init__.py`); the orchestrator did not, and + * the two halves of one product disagreeing is the actual defect. + * + * Quote stripping is deliberately CONSERVATIVE: only a matched leading+trailing + * pair of the SAME character is removed, because that is the paste artifact + * (`set COMFYUI_PATH="C:\...\ComfyUI"`). A lone trailing quote is left alone — + * `"` is an illegal filename character on Windows but LEGAL on POSIX, so + * stripping one unconditionally would corrupt a real path to fix a typo. + * + * Returns `changed` so callers can SAY the value was malformed rather than + * silently repairing it: a launcher that produces a bad value here will produce + * one everywhere else too, and silent normalization hides that. + */ +export function normalizeInstallPathEnv( + raw: string | undefined, + opts: { + varName?: string; + /** Injected for tests; defaults to a throw-safe existsSync. */ + exists?: (p: string) => boolean; + /** Set false to normalize without emitting the ingestion warning. */ + warn?: boolean; + } = {}, +): { path: string | undefined; changed: boolean } { + if (typeof raw !== "string") return { path: undefined, changed: false }; + + let v = raw.trim(); + const first = v[0]; + if ((first === '"' || first === "'") && v.length >= 2 && v[v.length - 1] === first) { + v = v.slice(1, -1).trim(); + } + + // NOTHING TO REPAIR — return without touching the disk (codex P2). This is the + // overwhelmingly common case: a well-formed value, on every call, at five + // readers, some of them hot. Probing first would add synchronous I/O to what + // used to be a plain environment read, and on a UNC/network root that stat can + // block. The existence question only ever matters when the repair would + // actually change something, so it is asked only then. + if (v === raw) return { path: raw === "" ? undefined : raw, changed: false }; + + // THE GUARD THAT MAKES THIS NON-DESTRUCTIVE (codex P1). Trailing whitespace and + // quote characters are LEGAL in POSIX filenames, and a directory literally named + // `ComfyUI ` is creatable on Windows too (measured on win32). So a blanket trim + // can silently redirect a caller away from a real directory — a repair doing more + // damage than the bug it fixes. + // + // If the value as GIVEN resolves, it is not malformed: keep it untouched. The + // repair is therefore a strict FALLBACK, and can only ever act on a value that + // does not name anything. This still fixes the report: measured on win32, + // existsSync(" ") is false and join(" ", "main.py") does not resolve + // either, which is precisely why every install-root check missed. + // + // The TOCTOU here is inherent to any existence-based fallback and is benign in + // both directions: a path created after a failed probe gets normalized away (it + // did not exist when we were asked), and one deleted after a successful probe is + // returned as given and fails downstream exactly as it would have before. + const exists = + opts.exists ?? + ((p: string): boolean => { + try { + return existsSync(p); + } catch { + return false; + } + }); + if (raw !== "" && exists(raw)) return { path: raw, changed: false }; + + // An all-whitespace value normalizes to "" — treated as UNSET, matching the + // existing `||` truthy checks at the call sites (a set-but-empty COMFYUI_PATH= + // already means "unset" here). + const path = v === "" ? undefined : v; + // Warned HERE rather than by each caller, so a new ingestion point cannot get + // the normalization while silently forgetting to report it — and so every call + // site is the same single expression, which is what lets the source gate demand + // the raw read be consumed inline. + if (opts.warn !== false) warnInstallPathWasMalformed(raw, path, opts.varName); + return { path, changed: true }; +} + +/** Warn ONCE per distinct malformed value — the retarget path re-resolves on + * every switch, and a warning that repeats on a timer is one people learn to + * scroll past. */ +const warnedMalformedPaths = new Set(); + +/** + * #1512 — say that the value was malformed, at INGESTION, instead of letting it + * surface much later as "the models directory could not be determined". The + * reporter's own follow-up, and the half that turns a 40-minute-delayed mystery + * into an immediate, actionable line. + */ +function warnInstallPathWasMalformed( + raw: string, + normalized: string | undefined, + varName = "COMFYUI_PATH", +): void { + // No `raw === normalized` guard here: the sole caller already fires this only + // when it actually changed something. A second copy of that condition read as + // defensive but was undetectable — mutation testing kills nothing when it is + // removed, which is the signal that it decides nothing. + if (warnedMalformedPaths.has(raw)) return; + warnedMalformedPaths.add(raw); + console.error( + `[comfyui-mcp] WARNING: ${varName} had surrounding whitespace or quotes and was ` + + `normalized.\n` + + ` as given: ${JSON.stringify(raw)}\n` + + ` using: ${JSON.stringify(normalized ?? null)}\n` + + ` On Windows, \`set ${varName}= && \` captures the space BEFORE the \`&&\` — ` + + `fix the launcher line, or the same malformed value will reach anything else it starts.`, + ); +} + +/** Reset the warn-once ledger. Test-only — a module-level Set otherwise leaks + * across cases in the same worker and makes the second assertion vacuous. */ +export function __resetMalformedPathWarnings(): void { + warnedMalformedPaths.clear(); +}