fix(1512): a trailing space in COMFYUI_PATH stops silently breaking every install-root check - #1523
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… malformed
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) <noreply@anthropic.com>
… 5 readers
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("<root>") -> true
existsSync("<root> ") -> false
existsSync(join("<root> ",...)) -> 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) <noreply@anthropic.com>
…bing needlessly
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) <noreply@anthropic.com>
artokun
marked this pull request as ready for review
August 13, 2026 12:07
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Claims #1512.
COMFYUI_PATHis consumed without trimming, so surrounding whitespace makes every install-root check miss and the connected ComfyUI is 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.exeassigns everything up to the&&, including the space before it, so the launcher line people actually paste bakes one in. The panel pack already strips this; the orchestrator did not — two halves of one product disagreeing.What the report missed
It names
resolveComfyUIPathas "the single ingestion point". There are five non-test readers, and a fix confined to the first is not merely incomplete — it makes one case worse:config.tsresolveComfyUIPathorchestrator/index.tspanel-tools.tscomfyWorkflowsDirs<root> /user/default/workflows, which silently does not exist, so the library reads emptyextra-paths.tsconfig.comfyuiPath— normalizing only the latter turns an accidental match into a mismatch and reclassifies an explicitly named root asinferredworkspace-env.tsThe
extra-pathsrow is a regression the partial fix would have introduced.The repair is a fallback, not a cleanup
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. A value that resolves as given is never touched.
The obvious objection is that Windows tolerates trailing spaces, which would make the guard a no-op exactly where the bug lives. Measured on win32 rather than assumed:
Quote stripping removes only a matched pair; a lone trailing quote is left alone.
Also
&&line that produced it. Warn-once per distinct value. Emitted inside the normalizer so a new ingestion point cannot get normalization while silently forgetting to report.src/utils/install-path-env.ts.config.tsbuilds module state at import time and many suites mock it wholesale; importing a pure string helper from it turned 46 tests red for missing mock exports — harness defects, not product bugs.Verification
changed.Out of scope, from the report:
download_model action:"status"cannot see partials whose job records were pruned, so orphaned.partialfiles are unreclaimable. Real, but a different feature.