From fa604157ffff0a28add589fe848a9da2da902291 Mon Sep 17 00:00:00 2001 From: Artokun Date: Thu, 13 Aug 2026 04:31:34 -0700 Subject: [PATCH 1/4] =?UTF-8?q?wip(1512):=20claim=20=E2=80=94=20COMFYUI=5F?= =?UTF-8?q?PATH=20is=20consumed=20untrimmed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From 6d7837a25f2ec801bb8a52492665fa238f2d924c Mon Sep 17 00:00:00 2001 From: Artokun Date: Thu, 13 Aug 2026 04:40:22 -0700 Subject: [PATCH 2/4] fix(1512): trim COMFYUI_PATH at BOTH ingestion points, and say it was malformed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. It cost a 12.3 GB download, stranded at 11.35 GB and finished by hand. cmd.exe assigns everything up to the `&&`, INCLUDING the space before it, so the launcher line people actually paste bakes one in: cmd /k "set COMFYUI_PATH=E:\...\ComfyUI && comfyui-mcp connect ..." The panel pack already stripped this; the orchestrator did not. Two halves of one product disagreeing is the defect. The report names resolveComfyUIPath as "the single ingestion point". It is not. orchestrator/index.ts reads process.env.COMFYUI_PATH DIRECTLY, and what that produces is handed to the spawn env builders — so a fix confined to config.ts would leave the bad value reaching every agent the orchestrator starts while looking fixed locally. Both now share one normalizer so they cannot drift. Narrower than the proposed patch in two places, on purpose: - quote stripping removes only a MATCHED leading+trailing pair. The proposed `replace(/^["']|["']$/g, "")` also strips a LONE trailing quote — illegal in a Windows filename but legal on POSIX, so it could corrupt a real path to fix a typo. The repair must not be able to do more damage than the bug. - a whitespace-only value normalizes to UNSET, so detection still runs instead of adopting " " as a path. Also builds the reporter's follow-up: the malformed value is REPORTED at ingestion, with both values JSON-quoted so the offending space is visible, naming the launcher line that produced it. Warn-once per distinct value — retarget re-resolves on every switch. 11 tests, 7/7 mutations killed. The orchestrator call site sits in a startup function no unit test can reach, so it is pinned by a source assertion: no raw read of that variable may survive un-normalized. Co-Authored-By: Claude Opus 5 (1M context) --- src/__tests__/comfyui-path-trim.test.ts | 189 ++++++++++++++++++++++++ src/config.ts | 84 ++++++++++- src/orchestrator/index.ts | 15 +- 3 files changed, 285 insertions(+), 3 deletions(-) create mode 100644 src/__tests__/comfyui-path-trim.test.ts diff --git a/src/__tests__/comfyui-path-trim.test.ts b/src/__tests__/comfyui-path-trim.test.ts new file mode 100644 index 000000000..497cd9ee4 --- /dev/null +++ b/src/__tests__/comfyui-path-trim.test.ts @@ -0,0 +1,189 @@ +// #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. `COMFYUI_PATH` has TWO ingestion points, and the +// patch proposed on the issue covers only the first: +// +// 1. `resolveComfyUIPath` in config.ts (boot + retarget) +// 2. a DIRECT `process.env.COMFYUI_PATH` read in orchestrator/index.ts, whose +// result is handed to the spawn env builders and resolveComfyuiPathForTarget +// +// Fixing only (1) leaves a trailing space reaching every agent the orchestrator +// starts. (2) sits inside a large startup function that a unit test cannot reach, +// so it is asserted against the SOURCE — the rule being that no raw read of that +// variable may survive un-normalized. +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, + warnIfInstallPathWasMalformed, + __resetMalformedPathWarnings, +} from "../config.js"; + +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 = normalizeInstallPathEnv("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(normalizeInstallPathEnv('"C:\\ComfyUI"').path).toBe("C:\\ComfyUI"); + expect(normalizeInstallPathEnv("'C:\\ComfyUI'").path).toBe("C:\\ComfyUI"); + // Quote OUTSIDE the space and space INSIDE the quote both normalize. + expect(normalizeInstallPathEnv(' "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(normalizeInstallPathEnv('/srv/weird"').path).toBe('/srv/weird"'); + expect(normalizeInstallPathEnv('/srv/weird"').changed).toBe(false); + expect(normalizeInstallPathEnv("'/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(normalizeInstallPathEnv(" ").path).toBeUndefined(); + expect(normalizeInstallPathEnv('" "').path).toBeUndefined(); + expect(normalizeInstallPathEnv("").path).toBeUndefined(); + expect(normalizeInstallPathEnv(undefined).path).toBeUndefined(); + }); + + it("reports changed:false for an already-clean value", () => { + const out = normalizeInstallPathEnv("/opt/ComfyUI"); + expect(out.path).toBe("/opt/ComfyUI"); + expect(out.changed).toBe(false); + }); +}); + +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", () => { + warnIfInstallPathWasMalformed("C:\\ComfyUI ", "C:\\ComfyUI"); + 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", () => { + warnIfInstallPathWasMalformed("C:\\ComfyUI ", "C:\\ComfyUI"); + warnIfInstallPathWasMalformed("C:\\ComfyUI ", "C:\\ComfyUI"); + warnIfInstallPathWasMalformed("D:\\Other ", "D:\\Other"); + expect(errs.length).toBe(2); + }); + + it("says nothing when the value was already clean", () => { + warnIfInstallPathWasMalformed("/opt/ComfyUI", "/opt/ComfyUI"); + 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("the SECOND ingestion point stays normalized (#1512)", () => { + // orchestrator/index.ts reads process.env.COMFYUI_PATH directly, inside a + // startup function no unit test can reach. What it produces is handed to the + // spawn env builders, so an un-normalized read there propagates the bad value + // to every agent the orchestrator starts. Asserted against the source, because + // the alternative is asserting nothing. + it("every raw read of COMFYUI_PATH in orchestrator/index.ts is normalized", () => { + const src = readFileSync(join(SRC, "orchestrator", "index.ts"), "utf8"); + const lines = src.split(/\r?\n/); + + const rawReads: number[] = []; + lines.forEach((line, i) => { + // Skip comments and the tool-description prose, which mention the variable + // by name without reading it. + const code = line.replace(/\/\/.*$/, ""); + if (/process\.env\.COMFYUI_PATH/.test(code)) rawReads.push(i); + }); + + // The premise: if this hits zero the rule is vacuous and the test is a + // rubber stamp, so fail loudly instead. + expect(rawReads.length).toBeGreaterThan(0); + + for (const i of rawReads) { + const window = lines.slice(i, i + 3).join("\n"); + expect( + /normalizeInstallPathEnv\(/.test(window), + `orchestrator/index.ts:${i + 1} reads process.env.COMFYUI_PATH without ` + + `normalizeInstallPathEnv() within 3 lines. A trailing space there is passed ` + + `on to every spawned agent (#1512).`, + ).toBe(true); + } + }); +}); diff --git a/src/config.ts b/src/config.ts index 6e49ccfd1..36811d75f 100644 --- a/src/config.ts +++ b/src/config.ts @@ -67,6 +67,84 @@ export function looksLikeComfyUIRoot(p: string): boolean { * (already a valid root, or no nested root found), so this is a strict no-op for * a correctly-installed (non-nested) ComfyUI. */ +/** + * #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): { + 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(); + } + // An all-whitespace value normalizes to "" — treated as UNSET, matching the + // existing `||` truthy checks at both call sites (a set-but-empty + // COMFYUI_PATH= already means "unset" here). + return { path: v === "" ? undefined : v, changed: v !== raw }; +} + +/** 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. + */ +export function warnIfInstallPathWasMalformed( + raw: string | undefined, + normalized: string | undefined, + varName = "COMFYUI_PATH", +): void { + if (typeof raw !== "string" || raw === normalized) return; + 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(); +} + export function descendToNestedRoot(p: string, label = "COMFYUI_PATH"): string { try { if (looksLikeComfyUIRoot(p)) return p; @@ -249,9 +327,13 @@ 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); + warnIfInstallPathWasMalformed(rawEnvPath, envPath); if (envPath) { if (opts.remoteUrl) { console.error( diff --git a/src/orchestrator/index.ts b/src/orchestrator/index.ts index b016a7c8a..bd932f04f 100644 --- a/src/orchestrator/index.ts +++ b/src/orchestrator/index.ts @@ -137,7 +137,7 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js"; 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 { isForceRemoteFlagSet, isLoopbackHost, detectLocalComfyUIPath, setComfyuiTarget, onComfyuiTargetChanged, isTargetingLocal, isTargetingLocalOrLan, isTargetingPod, getComfyUIBaseUrl, getLocalComfyuiUrl, rescopeLocalTargetFile, getComfyUIAuthHeaders, normalizeInstallPathEnv, warnIfInstallPathWasMalformed } from "../config.js"; import { buildComfyuiMcpEnv, comfyuiSecretKeys, @@ -1308,9 +1308,20 @@ 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 rawEnvComfyuiPath = process.env.COMFYUI_PATH; + const { path: envComfyuiPath } = normalizeInstallPathEnv(rawEnvComfyuiPath); + warnIfInstallPathWasMalformed(rawEnvComfyuiPath, envComfyuiPath); // `||` 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 { From 9707b752d831fc08e3b94eb595988739dada0ccc Mon Sep 17 00:00:00 2001 From: Artokun Date: Thu, 13 Aug 2026 04:57:40 -0700 Subject: [PATCH 3/4] =?UTF-8?q?fix(1512):=20close=20codex's=20three=20find?= =?UTF-8?q?ings=20=E2=80=94=20non-destructive=20repair,=20all=205=20reader?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 (destructive on valid POSIX paths). Trailing whitespace and quotes are LEGAL POSIX filename characters, so a blanket trim can redirect a caller away from a real directory — a repair doing more damage than the bug. The normalization is now a strict FALLBACK: a value that RESOLVES as given is never touched. Measured on win32 rather than assumed, because the obvious worry is that Windows tolerates trailing spaces and would make the guard a no-op there: existsSync("") -> true existsSync(" ") -> false existsSync(join(" ",...)) -> false mkdir "WithSpace " -> succeeds (so the POSIX case is real here too) The false result is exactly why every install-root check missed, so the guard keeps the fix while making it unable to touch a path that works. P1 (raw consumers beyond the asserted site). Found independently while auditing the same question; there are FIVE readers, not two. The extra-paths one is the sharp edge: it 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". Fixing one end alone would have introduced that. P2 (source assertion was a rubber stamp). It required a normalizer "within 3 lines", which passes when the result is discarded and the raw value forwarded anyway. It now requires the read to BE an argument, so the raw value has no name to be forwarded under — verified by building codex's counterexample and watching the gate reject it. The helper moved to src/utils/install-path-env.ts. config.ts builds its module-level config at IMPORT time, so many suites mock it wholesale; putting a pure string helper behind it turned 46 tests red for missing mock exports, none of them a product defect. A leaf module has no such gravity. One redundancy removed on its own evidence: a second `raw === normalized` guard inside the warn helper killed no mutation, because the only caller already gates on `changed`. Undetectable by construction is not defensive. 12 tests, 11/11 mutations killed. Suite 491 files / 9239 tests. Co-Authored-By: Claude Opus 5 (1M context) --- src/__tests__/comfyui-path-trim.test.ts | 169 +++++++++++++++--------- src/config.ts | 80 +---------- src/orchestrator/index.ts | 7 +- src/orchestrator/panel-tools.ts | 6 +- src/services/extra-paths.ts | 10 +- src/services/workspace-env.ts | 5 +- src/utils/install-path-env.ts | 125 ++++++++++++++++++ 7 files changed, 254 insertions(+), 148 deletions(-) create mode 100644 src/utils/install-path-env.ts diff --git a/src/__tests__/comfyui-path-trim.test.ts b/src/__tests__/comfyui-path-trim.test.ts index 497cd9ee4..ec6a5f8f1 100644 --- a/src/__tests__/comfyui-path-trim.test.ts +++ b/src/__tests__/comfyui-path-trim.test.ts @@ -12,27 +12,31 @@ // stripped it (`__init__.py`); the orchestrator did not — the two halves of one // product disagreeing is the defect. // -// THE TRAP THIS FILE GUARDS. `COMFYUI_PATH` has TWO ingestion points, and the -// patch proposed on the issue covers only the first: +// 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. // -// 1. `resolveComfyUIPath` in config.ts (boot + retarget) -// 2. a DIRECT `process.env.COMFYUI_PATH` read in orchestrator/index.ts, whose -// result is handed to the spawn env builders and resolveComfyuiPathForTarget -// -// Fixing only (1) leaves a trailing space reaching every agent the orchestrator -// starts. (2) sits inside a large startup function that a unit test cannot reach, -// so it is asserted against the SOURCE — the rule being that no raw read of that -// variable may survive un-normalized. +// 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, - warnIfInstallPathWasMalformed, - __resetMalformedPathWarnings, -} from "../config.js"; +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, ".."); @@ -43,38 +47,38 @@ beforeEach(() => { describe("normalizeInstallPathEnv (#1512)", () => { it("strips the trailing space cmd.exe bakes into `set VAR=v && cmd`", () => { - const out = normalizeInstallPathEnv("E:\\Ai_server\\ComfyUI_windows_portable\\ComfyUI "); + 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(normalizeInstallPathEnv('"C:\\ComfyUI"').path).toBe("C:\\ComfyUI"); - expect(normalizeInstallPathEnv("'C:\\ComfyUI'").path).toBe("C:\\ComfyUI"); + 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(normalizeInstallPathEnv(' "C:\\ComfyUI " ').path).toBe("C:\\ComfyUI"); + 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(normalizeInstallPathEnv('/srv/weird"').path).toBe('/srv/weird"'); - expect(normalizeInstallPathEnv('/srv/weird"').changed).toBe(false); - expect(normalizeInstallPathEnv("'/srv/half").path).toBe("'/srv/half"); + 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(normalizeInstallPathEnv(" ").path).toBeUndefined(); - expect(normalizeInstallPathEnv('" "').path).toBeUndefined(); - expect(normalizeInstallPathEnv("").path).toBeUndefined(); - expect(normalizeInstallPathEnv(undefined).path).toBeUndefined(); + 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 = normalizeInstallPathEnv("/opt/ComfyUI"); + const out = norm("/opt/ComfyUI"); expect(out.path).toBe("/opt/ComfyUI"); expect(out.changed).toBe(false); }); @@ -93,7 +97,7 @@ describe("the malformed value is REPORTED, not silently repaired (#1512)", () => afterEach(() => spy.mockRestore()); it("names the variable, both values, and the launcher line that produced it", () => { - warnIfInstallPathWasMalformed("C:\\ComfyUI ", "C:\\ComfyUI"); + norm("C:\\ComfyUI ", true); const msg = errs.join("\n"); expect(msg).toMatch(/COMFYUI_PATH/); @@ -105,14 +109,21 @@ describe("the malformed value is REPORTED, not silently repaired (#1512)", () => }); it("warns ONCE per distinct value — retarget re-resolves on every switch", () => { - warnIfInstallPathWasMalformed("C:\\ComfyUI ", "C:\\ComfyUI"); - warnIfInstallPathWasMalformed("C:\\ComfyUI ", "C:\\ComfyUI"); - warnIfInstallPathWasMalformed("D:\\Other ", "D:\\Other"); + 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", () => { - warnIfInstallPathWasMalformed("/opt/ComfyUI", "/opt/ComfyUI"); + 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); }); }); @@ -154,36 +165,70 @@ describe("the WIRING — a real config build normalizes the env (#1512)", () => }); }); -describe("the SECOND ingestion point stays normalized (#1512)", () => { - // orchestrator/index.ts reads process.env.COMFYUI_PATH directly, inside a - // startup function no unit test can reach. What it produces is handed to the - // spawn env builders, so an un-normalized read there propagates the bad value - // to every agent the orchestrator starts. Asserted against the source, because - // the alternative is asserting nothing. - it("every raw read of COMFYUI_PATH in orchestrator/index.ts is normalized", () => { - const src = readFileSync(join(SRC, "orchestrator", "index.ts"), "utf8"); - const lines = src.split(/\r?\n/); - - const rawReads: number[] = []; - lines.forEach((line, i) => { - // Skip comments and the tool-description prose, which mention the variable - // by name without reading it. - const code = line.replace(/\/\/.*$/, ""); - if (/process\.env\.COMFYUI_PATH/.test(code)) rawReads.push(i); - }); - - // The premise: if this hits zero the rule is vacuous and the test is a - // rubber stamp, so fail loudly instead. - expect(rawReads.length).toBeGreaterThan(0); - - for (const i of rawReads) { - const window = lines.slice(i, i + 3).join("\n"); - expect( - /normalizeInstallPathEnv\(/.test(window), - `orchestrator/index.ts:${i + 1} reads process.env.COMFYUI_PATH without ` + - `normalizeInstallPathEnv() within 3 lines. A trailing space there is passed ` + - `on to every spawned agent (#1512).`, - ).toBe(true); +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; + totalReads++; + // INLINE consumption, not "a normalizer appears nearby" (codex P2). A + // proximity rule passes when the normalized result is discarded and the + // raw variable is forwarded anyway — which is the exact bug, with a + // normalizer call sitting next to it as decoration. Requiring the read to + // BE an argument means the raw value has no name to be forwarded under. + const consumedInline = + /normalizeInstallPathEnv\(\s*process\.env\.COMFYUI_PATH/.test(code) || + /resolveComfyUIPath\(\s*process\.env\.COMFYUI_PATH/.test(code); + if (consumedInline) return; + offenders.push(`${file.replace(SRC, "src")}:${i + 1} ${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 36811d75f..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"; @@ -67,84 +68,6 @@ export function looksLikeComfyUIRoot(p: string): boolean { * (already a valid root, or no nested root found), so this is a strict no-op for * a correctly-installed (non-nested) ComfyUI. */ -/** - * #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): { - 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(); - } - // An all-whitespace value normalizes to "" — treated as UNSET, matching the - // existing `||` truthy checks at both call sites (a set-but-empty - // COMFYUI_PATH= already means "unset" here). - return { path: v === "" ? undefined : v, changed: v !== raw }; -} - -/** 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. - */ -export function warnIfInstallPathWasMalformed( - raw: string | undefined, - normalized: string | undefined, - varName = "COMFYUI_PATH", -): void { - if (typeof raw !== "string" || raw === normalized) return; - 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(); -} - export function descendToNestedRoot(p: string, label = "COMFYUI_PATH"): string { try { if (looksLikeComfyUIRoot(p)) return p; @@ -333,7 +256,6 @@ function resolveComfyUIPath( // #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); - warnIfInstallPathWasMalformed(rawEnvPath, envPath); if (envPath) { if (opts.remoteUrl) { console.error( diff --git a/src/orchestrator/index.ts b/src/orchestrator/index.ts index bd932f04f..3a4af6103 100644 --- a/src/orchestrator/index.ts +++ b/src/orchestrator/index.ts @@ -137,7 +137,8 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js"; 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, normalizeInstallPathEnv, warnIfInstallPathWasMalformed } from "../config.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, @@ -1315,9 +1316,7 @@ export async function runPanelOrchestrator(): Promise { // 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 rawEnvComfyuiPath = process.env.COMFYUI_PATH; - const { path: envComfyuiPath } = normalizeInstallPathEnv(rawEnvComfyuiPath); - warnIfInstallPathWasMalformed(rawEnvComfyuiPath, envComfyuiPath); + 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, 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..ba3c90243 --- /dev/null +++ b/src/utils/install-path-env.ts @@ -0,0 +1,125 @@ +// #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 }; + const exists = + opts.exists ?? + ((p: string): boolean => { + try { + return existsSync(p); + } catch { + return 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. + if (raw !== "" && exists(raw)) return { path: raw, 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(); + } + // 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; + const changed = v !== raw; + // 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 (changed && opts.warn !== false) warnInstallPathWasMalformed(raw, path, opts.varName); + return { path, changed }; +} + +/** 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(); +} From b05072a5f64825b8a9521ed253ff810508f8c4bf Mon Sep 17 00:00:00 2001 From: Artokun Date: Thu, 13 Aug 2026 05:06:04 -0700 Subject: [PATCH 4/4] =?UTF-8?q?fix(1512):=20close=20codex=20round=202=20?= =?UTF-8?q?=E2=80=94=20count=20every=20occurrence,=20and=20stop=20probing?= =?UTF-8?q?=20needlessly?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P2: the source gate was LINE-level, so a line whose first occurrence was an argument exempted the rest of it: const p = normalizeInstallPathEnv(process.env.COMFYUI_PATH).path; f(process.env.COMFYUI_PATH); It now counts occurrences against guarded occurrences, so the tally has to account for all of them. Verified by building that exact line and watching the gate reject it — reported as "(1/2 consumed)". P2: existsSync ran for EVERY non-empty value, including well-formed ones that the repair cannot touch. Five readers call this, some hot, and it replaced a plain env read — a stat per call is new synchronous I/O everywhere, and on a UNC/network root it can block. The order is inverted: compute the repair first and return immediately when there is nothing to change, so the disk is consulted only when its answer decides something. Pinned by counting probes rather than asserting the property: zero for clean/empty/undefined, exactly one for a value that would change. The TOCTOU codex notes is inherent to any existence-based fallback and benign both ways — recorded in the source rather than left for the next reader to rediscover. 13 tests, 11/11 mutations killed. Co-Authored-By: Claude Opus 5 (1M context) --- src/__tests__/comfyui-path-trim.test.ts | 56 ++++++++++++++++++++----- src/utils/install-path-env.ts | 47 +++++++++++++-------- 2 files changed, 75 insertions(+), 28 deletions(-) diff --git a/src/__tests__/comfyui-path-trim.test.ts b/src/__tests__/comfyui-path-trim.test.ts index ec6a5f8f1..dd6fc6800 100644 --- a/src/__tests__/comfyui-path-trim.test.ts +++ b/src/__tests__/comfyui-path-trim.test.ts @@ -82,6 +82,30 @@ describe("normalizeInstallPathEnv (#1512)", () => { 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)", () => { @@ -206,17 +230,27 @@ describe("NO reader of COMFYUI_PATH consumes it raw (#1512)", () => { if (trimmed.startsWith("*") || trimmed.startsWith("/*") || trimmed.startsWith("//")) return; const code = line.replace(/\/\/.*$/, ""); if (!/process\.env\.COMFYUI_PATH/.test(code)) return; - totalReads++; - // INLINE consumption, not "a normalizer appears nearby" (codex P2). A - // proximity rule passes when the normalized result is discarded and the - // raw variable is forwarded anyway — which is the exact bug, with a - // normalizer call sitting next to it as decoration. Requiring the read to - // BE an argument means the raw value has no name to be forwarded under. - const consumedInline = - /normalizeInstallPathEnv\(\s*process\.env\.COMFYUI_PATH/.test(code) || - /resolveComfyUIPath\(\s*process\.env\.COMFYUI_PATH/.test(code); - if (consumedInline) return; - offenders.push(`${file.replace(SRC, "src")}:${i + 1} ${line.trim()}`); + // 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()}`, + ); }); } diff --git a/src/utils/install-path-env.ts b/src/utils/install-path-env.ts index ba3c90243..9865b3e01 100644 --- a/src/utils/install-path-env.ts +++ b/src/utils/install-path-env.ts @@ -45,15 +45,20 @@ export function normalizeInstallPathEnv( } = {}, ): { path: string | undefined; changed: boolean } { if (typeof raw !== "string") return { path: undefined, changed: false }; - const exists = - opts.exists ?? - ((p: string): boolean => { - try { - return existsSync(p); - } catch { - return 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 @@ -66,24 +71,32 @@ export function normalizeInstallPathEnv( // 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 }; - 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(); - } // 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; - const changed = v !== raw; // 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 (changed && opts.warn !== false) warnInstallPathWasMalformed(raw, path, opts.varName); - return { path, changed }; + 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