fix(1508): apply_manifest stops force-failing in a uv-managed (PEP 668) Python - #1528
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.
…g into it
apply_manifest ran `python -m pip install <pkg>` against Stability Matrix's
uv-managed CPython and was refused:
error: externally-managed-environment
This Python installation is managed by uv and should not be modified.
Reading the code first widened the scope: there are THREE routes into that
refusal, not the one the report describes.
1. uv ABSENT -> bare pip. The reporter's actual path: Stability Matrix keeps
uv inside its own directory rather than on PATH, so this is the branch that
runs, straight into an interpreter whose pip refuses by design.
2. uv PRESENT and refused itself. uv will not modify a managed environment
either, and that error fell through to a bare rethrow with no route out.
3. uv's #377 non-venv fallback -> bare pip, hitting the same wall one step
later, with uv's unrelated "no virtual environment" complaint on top of the
real reason — which would send the reader off to create a venv.
All three now produce one actionable refusal: it names the environment as
EXTERNALLY MANAGED, says that is a deliberate guard rather than a broken
interpreter, and gives both supported routes out (the owning manager / an
explicit `uv pip install --python`, or COMFYUI_PYTHON pointed at a venv).
Deliberately NOT adding `--break-system-packages`, which is the reporter's own
caution and the right one: on a uv-managed interpreter that writes into an
environment uv owns and may later reset, converting a clean refusal into a
silent, delayed breakage of their ComfyUI. The message says it declined and why,
so that stays a decision someone makes on purpose.
The detector keys on PEP 668's error id first and the prose second: the second
line comes from the distributor's EXTERNALLY-MANAGED file, so it reads
differently on Debian, Homebrew and uv. Keying only on uv's sentence would have
fixed this reporter and nobody else.
44 tests, 7/7 mutations killed. M2 initially SURVIVED and caught route 2 being
untested — the pre-#377 check existed but nothing exercised it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MEASURED, and it refutes what I wrote. The first version of this message told
the reader to run `uv pip install --python <interp>`. Against a real uv-managed
CPython (uv 0.11.21, standalone 3.12.13 installed into a temp dir), uv refuses
that too:
error: The interpreter at ... is externally managed, and indicates the
following:
This Python installation is managed by uv and should not be modified.
hint: Consider creating a virtual environment, e.g., with `uv venv`
That is the defect this repo keeps fixing in other people's messages — a remedy
that reads as the answer and cannot work, costing the reader the time to
disprove it. Confirmed in the same run that a uv-managed interpreter really does
carry the PEP 668 marker and that bare pip fails exactly as reported, so the
environment shape in the issue is reproduced, not assumed.
The message now says plainly that uv is NOT a way around this, and points where
uv itself points: a virtual environment. It also names the check worth doing
FIRST — if ComfyUI already runs from a venv, the interpreter in the message is
the wrong one and the fix is COMFYUI_PYTHON, not packaging.
`--break-system-packages` is still declined, and still explained.
44 tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s own output
P2 (detector too broad). It matched the bare phrase "externally managed"
anywhere in stderr/stdout/message. That prose appears in ordinary build and
dependency failures, and rewriting one of those as the managed-environment story
discards the real cause AND supplies a remedy that does not apply — strictly
worse than the raw error. Now keyed on pip's canonical PEP 668 error IDENTIFIER
(`externally-managed-environment`) plus uv's own anchored form ("interpreter at
... is externally managed"). Both MEASURED against uv 0.11.21 and a uv-managed
CPython 3.12.13, not transcribed from docs.
P2 (the raw diagnostic was discarded). The refusal REPLACES the underlying
error, and apply_manifest reports only `err.message` per item — so anything the
installer said that this message does not anticipate was lost silently. An
earlier draft even told the reader the output was "above" when nothing had been
printed. The installer's own output is now carried in, clipped to 1200 chars so
a verbose failure cannot swamp the actionable part.
Also fixed on the way: the fallback route was attaching UV's earlier non-venv
complaint instead of pip's actual refusal — the exact misdirection that branch
exists to prevent. Caught by its own test.
46 tests (2 new: the false-positive prose case codex named, and the preserved
diagnostic). Suite 491 files / 9247 tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ses safe
codex round 2, plus one finding of its own probe that I took further.
P2 (still too broad): the pip token was an unanchored substring, so any output
merely CONTAINING `externally-managed-environment` — a cache path, a package
name, a vendored log line — was rewritten as this failure and lost its own
cause. Anchored to the start of a line, which is where pip prints it (measured).
P2 (too narrow, the opposite direction): PEP 668 does not mandate pip's renderer
token, so a distributor-patched or localized installer can refuse in wording
neither pattern matches. That miss is the SAFE failure and is now documented as
a deliberate choice: it surfaces the installer's own error unchanged — today's
behaviour, no regression — and that error already names PEP 668 itself. Matching
too eagerly instead REPLACES a real cause with a remedy that does not apply.
CREDENTIAL LEAK, found while checking codex's "does carrying raw output leak
anything" question. Node builds execFileSync's Error.message as
`Command failed: <whole argv>`, so it embeds the package spec — and a pip spec
may legitimately be a direct URL carrying credentials, which
validatePipPackageSpec permits (it rejects only options, control characters and
whitespace). Two paths leaked:
- the carried output (message included) -> now stderr/stdout ONLY; detection
still reads the message, so what is SHOWN is narrower than what is MATCHED;
- the refusal echoing `pkg` itself, which IS the credentialed URL -> URL
userinfo is redacted everywhere this message renders, including the carried
output, since pip prints the URL it fetches.
48 tests. Suite 491 files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ured result codex round 3, three findings, all real. P1 (clip before redact). clipInstallerOutput truncated at 1200 chars BEFORE redaction ran, so output whose credential sat near the cut lost the `@` the pattern anchors on and survived as `https://u:token` in a message that looked sanitised. Redaction now runs first. P1 (pattern stopped at the FIRST `@`). `https://u:alpha@beta@host` is a valid URL whose password is `alpha@beta`; the old `[^/\s@]+@` rendered it as `https://***:***@beta@host`, leaking half the secret while appearing redacted. Now a greedy `[^/\s]*@`, which cannot cross `/` or whitespace and so runs to the last `@` inside the authority. Probed directly against all four shapes, not just asserted. The structured RESULT, not only the message. `results[].item` echoes the manifest entry verbatim, so the credential survived in the field that actually travels into transcripts and logs — and my own test asserted only on `message` while I described it as "never echoes". Redacted in `report()`, where every item passes through; the entry stays identifiable because only userinfo is masked. That test now asserts against the WHOLE serialized result, which is what the claim was always supposed to mean. 49 tests. Suite 491 files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
codex round 4 confirmed the three prior findings closed and named two REMAINING
leaks, both pre-existing and both outside the PEP 668 path:
- `logger.info(..., { package: pkg })` logged the raw spec on two routes;
- an ORDINARY, non-PEP-668 failure is rethrown raw (correct for diagnosis) and
the caller reports `err.message`, which Node builds as
`Command failed: <whole argv>` — so every pip failure that ISN'T this issue's
case was still echoing the spec.
Both are fixed, the second at `report()` — the single point every manifest item
passes through. That closes the class rather than the instances: it covers the
ordinary pip path, the model and custom-node paths, and any future one, instead
of asking each new call site to remember. Redaction masks userinfo only, so the
real cause and the identifying host/path both survive; a test asserts the
diagnosis is still readable.
Recorded plainly, because the shape of this PR drifted: the pip ROUTING fix
settled two rounds ago, and everything since has been a credential-disclosure
problem that carrying installer output into a user-facing message opened up. Two
of the paths now closed predate this issue entirely.
50 tests. Suite 491 files / 9251.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
artokun
marked this pull request as ready for review
August 13, 2026 13:03
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 #1508.
apply_manifestranpython -m pip install <pkg>against Stability Matrix's uv-managed CPython and was refused under PEP 668.The routing fix
Reading the code widened it: three routes reach that refusal, not the one reported.
All three now raise one actionable refusal.
--break-system-packagesis declined and the message says why — on a uv-managed interpreter that writes into an environment uv may reset, turning a clear failure now into a broken ComfyUI later.The first version of that message was wrong and measurement caught it. It recommended
uv pip install --python <interp>. Against a real uv-managed CPython (uv 0.11.21, standalone 3.12.13), uv refuses that too —hint: Consider creating a virtual environment. A remedy that reads as the answer and cannot work costs the reader the time to disprove it, so the message now says uv is not a way around this and points where uv points.The detector keys on pip's PEP 668 error id anchored to line start plus uv's own form. Missing a distributor-patched refusal is the deliberate, safe direction — it surfaces the installer's own error unchanged, which already names PEP 668. Matching too eagerly instead replaces a real cause with an inapplicable remedy.
What this PR also became
Carrying the installer's output into the refusal opened a credential-disclosure problem, since a pip spec may legitimately be a direct URL with
user:token@. Four review rounds closed it:Error.messageembeds the whole argv → carry stderr/stdout only (detection still reads the message: shown is narrower than matched)@(https://u:alpha@beta@hostleaked half) → redact first, greedy to the last@results[].itemand ordinary-failure messages still carried it → redacted atreport(), the one point every item passes throughTwo of those paths predate this issue. This grew out of my fix rather than the reported bug, and is worth reviewing as such.
Verification
EXTERNALLY-MANAGEDmarker, bare pip refuses exactly as reported,uv pip install --pythonrefuses too@-in-password, credential-free, and multi-URL inputsWhat I could NOT verify
That a real Stability Matrix install accepts the route this message recommends. I reproduced the environment shape, not their launcher. Asking the reporter rather than implying otherwise.