fix(exec): route remaining git/ffmpeg/ffprobe spawns through _safe_exec.resolve_required_binary (BE-5358)

[mattmillerai]

STACKED — merging lands on matt/be-5357-cuda-detect-absolute-path (owned by @mattmillerai, PR #641), NOT main. This PR is based on #641's branch because it consumes comfy_cli/_safe_exec.py, which #641 introduces and which does not exist on main yet. GitHub will retarget this PR to main when #641 merges; review the net diff, and merge #641 first.

ELI-5

On Windows, when a program says "run git", Windows looks in the folder you happen to be standing in before it looks in the normal places. So if someone drops a fake git.exe into a folder and you run comfy from there, you run their fake git. PR #641 fixed that for the three hardware probes by adding a helper that turns a bare name like nvidia-smi into a full, trusted path. This PR does the same for everything else that was still asking for a program by bare name — git, ffmpeg, ffprobe. Nothing changes for anyone whose git/ffmpeg lives in a normal place.

What changed

1. comfy_cli/_safe_exec.py — a required-binary companion.

resolve_binary returns None ("skip this probe"), which lands cleanly on the probes' degrade-to-None contract. git/ffmpeg failing is usually fatal to the command, so this adds resolve_required_binary(name) -> str, a thin wrapper over resolve_binary that raises BinaryNotFoundError instead. Being a wrapper is the point: the _is_bare_name / _is_fully_qualified / is_planted_in_cwd gates live in exactly one place and cannot drift between the two entry points.

BinaryNotFoundError subclasses both RuntimeError and FileNotFoundError. FileNotFoundError is what subprocess already raises today when a bare-name spawn finds nothing, so every call site that already tolerated a missing binary (except (subprocess.SubprocessError, FileNotFoundError) in file_utils, except OSError in outdated, the three-arm handler in install) keeps degrading identically rather than starting to crash. That is what makes this a genuinely no-behaviour-change conversion at the tolerant sites, instead of one that silently turns graceful degradation into a traceback.

2. Every git spawn. All the sites the ticket named, plus three it did not: install.py:520 (fetch --tags), install.py:532 (tag --list), and install.py:677 (_git_capture, the shared helper behind the version-switch path).

file	call site	on unresolvable git
git_utils.py	git_checkout_tag, checkout_pr (9 spawns)	raises — as an absent git already did here (only CalledProcessError was caught)
cmdline.py	update → git pull	raises
command/install.py	execute → git checkout, clone_comfyui	raises
command/install.py	_resolve_latest_tag_from_local, _git_capture	degrades (unchanged)
command/outdated.py	_git_output	returns None (unchanged)
file_utils.py	list_git_tracked_files	returns [] (unchanged)
registry/config_parser.py	initialize_project_config	raises (unchanged)

3. ffmpeg / ffprobe. command/preview.py's duplicate shutil.which("ffmpeg") and shutil.which("ffprobe") presence check is folded into the resolution, so both binaries are looked up once and the resolved paths are reused for both spawns. build_preview_cmd now takes a required keyword-only ffmpeg_bin (no "ffmpeg" default) so no future caller can silently reintroduce the bare name. output/preview.py's best-effort inline previewer uses resolve_binary (not the required variant) because its documented contract is to skip silently — a missing ffmpeg and a refused CWD-planted one degrade the same way, to no preview.

Judgment calls

• Resolve before os.chdir, not a module-level _git() wrapper. The ticket suggested one wrapper in git_utils.py that "resolves once and prepends the resolved path". I used a per-function local instead, because in git_utils when resolution happens matters: a call-time wrapper resolves after the os.chdir(repo_path), so a git planted in the caller-supplied repo would win the lookup and be refused — safe, but it turns a plant into a denial of service against a user who has a perfectly good git on $PATH. Resolving before the chdir means the plant can neither be executed nor shadow the real binary. test_checkout_tag_spawns_resolved_path_not_the_repo_plant pins exactly this.
• BinaryNotFoundError is not a typer error. The ticket offered "RuntimeError/typer error". A typer error would couple the leaf _safe_exec module to typer and would be wrong at the library-level call sites (file_utils, git_utils) that are not commands. command/preview.py does the typer translation itself, at the command boundary, keeping the existing ffmpeg_unavailable error code and message.
• One user-visible behaviour change, inherited from fix(cuda): resolve nvidia-smi via absolute path to block Windows CWD planting (BE-5357) #641's design. Running comfy from a directory that is itself an absolute $PATH entry (/usr/bin, C:\Windows\System32) now makes git/ffmpeg unresolvable, so those commands fail instead of running. This is the known false positive already documented in resolve_binary; it is new only in that it now applies to git/ffmpeg as well as the probes. For a probe it degrades to None; for git it is a clear error message.

Not in scope (follow-ups worth filing)

• GitPython. workspace_manager.py:90,100 and install.py:220 use git.Repo(...), and GitPython spawns git by bare name through its own GIT_PYTHON_GIT_EXECUTABLE (default "git"). Closing that means a global git.refresh(<abs path>) at import time, which changes import-time failure behaviour — a distinct, riskier change than this mechanical conversion, and the ticket enumerates subprocess sites only.
• Other bare-name spawns. utils.py:75 (conda) and install.py:1134,1162,1178,1201,1210,1307,1314 (node, npm, pnpm, npx) are still bare-name. Same vector, outside this ticket's stated scope (git, ffmpeg/ffprobe).

Tests

New tests/comfy_cli/test_safe_exec_call_sites.py (13 cases) and tests/comfy_cli/output/test_inline_preview.py (4 cases), plus new cases in test_safe_exec.py and command/test_preview.py. They share a shutil.which stub that searches the CWD before $PATH — Windows' actual lookup order — so the planting vector is reproducible on any host. Each converted site asserts two things: the argv handed to subprocess starts with the resolved absolute path (never the bare name), and a binary planted directly in the CWD is never spawned (recorder.calls == []). The os.chdir-then-spawn sites get explicit regression tests, including one that the CWD is restored when git is unresolvable.

Three existing test files were updated because they asserted the bare name: test_update_version.py's FakeGit now dispatches on the argv basename, test_pr.py asserts the spawned path is absolute and named git, and test_preview.py passes an explicit ffmpeg_bin.

Status: ruff check clean, ruff format --diff clean (verified against the CI-pinned ruff==0.15.15), pytest — 3710 passed, 37 skipped.

Closes BE-5358.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f91b46c3-8783-4553-bdb2-dee2a0c8bfa1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 10 finding(s).

Severity	Count
🟠 High	2
🟡 Medium	4
🟢 Low	3
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

[mattmillerai]

STACKED — merging lands on matt/be-5357-cuda-detect-absolute-path (PR #641, @mattmillerai), NOT main. Land #641 first; merging this one on its own puts these commits on that branch.

---

Resolved all 10 Cursor panel findings in abf558f. The two 🟠 High ones shared a root cause, so they are fixed together:

Root cause — resolve_required_binary could not say why it refused. "git is not installed" and "git was found and refused because the only match is CWD-anchored" produced the same "was not found on PATH" message. _safe_exec now returns a BinaryResolution(path, reason, candidate), and BinaryNotFoundError carries .reason / .candidate / .is_absent. is_planted_in_cwd gained a tri-state companion so a deleted CWD ("could not look") is UNVERIFIABLE rather than borrowing the plant's message.

Everything downstream branches on that:

Finding	Fix
🟠 _safe_exec.py — absent vs refused indistinguishable	reason-aware refusals + per-reason diagnostics
🟠 file_utils.py — refused git → [] → zip_files walks everything into the published archive	absent still degrades to []; refused now raises. node publish / node pack abort with the reason
🟡 install.py — git clone option injection	git clone -- <url> <dir>; reject_option_like_ref for git checkout <rev>, where -- cannot help
🟡 preview.py — unbounded ffprobe/ffmpeg	30 s probe / 120 s render timeouts
🟡 config_parser.py — refusal escapes after pyproject.toml is written	handler widened to OSError
🟡 tests — drive-less \usr\bin\git fails on Windows	drive-qualified path under os.name == "nt"
🟢 preview.py — "install ffmpeg" for a refusal	new ffmpeg_untrusted code
🟢 git_utils.py — refusal escapes the documented False contract	git_checkout_tag and checkout_pr return False
🟢 install.py — refusal misreported as "version not found"	up-front check → version_switch_git_unavailable
⚪ outdated.py — re-resolves per call	declined, with measurements — see the thread

Verified: ruff check + ruff format --diff clean (pinned 0.15.15), 3613 passed / 13 skipped.

[bigcat88]

Blocked on its base, not on anything in this diff.

GitHub reports this PR MERGEABLE, but that's measured against #641's branch — against main it conflicts, in tests/comfy_cli/test_hardware.py. The cause is upstream of you: #641 conflicts with main because #567 (BE-3434) landed the same CWD-planting guard directly in hardware.py on 2026-07-31, and #641 extracts that logic into the comfy_cli/_safe_exec.py this PR consumes. I've left the details on #641.

So the order is: rebase #641 → merge it → this retargets to main → rebase here if needed → I'll review.

Reading the description in the meantime, two things I'd flag now so they don't need a second round:

• BinaryNotFoundError subclassing both RuntimeError and FileNotFoundError is the right call and the load-bearing detail of the whole PR — it's what keeps file_utils' except (SubprocessError, FileNotFoundError), outdated's except OSError, and install's three-arm handler degrading identically instead of newly crashing. When this comes back I'll test exactly that: that each tolerant site still degrades rather than raises.
• The three sites beyond the ticket (install.py fetch --tags, tag --list, _git_capture) are worth keeping — a partial conversion of git spawns is the worst outcome, since it looks fixed.

Ping me once the stack is rebased.

[skishore23]

Approving. Merges CLEAN against current main, and the two load-bearing design decisions both hold up under direct testing. Merge #641 first.

BinaryNotFoundError's dual inheritance does what it claims

This is what makes the conversion genuinely behaviour-preserving at the tolerant sites, so I checked it rather than reading it:

MRO: BinaryNotFoundError -> RuntimeError -> FileNotFoundError -> OSError -> Exception

except (subprocess.SubprocessError, FileNotFoundError)  catches: True   # file_utils
except OSError                                          catches: True   # outdated
except RuntimeError                                     catches: True

So every existing handler keeps degrading identically instead of starting to traceback. Inheriting from FileNotFoundError specifically — because that's what subprocess already raises on a bare-name miss — is the right insight; a plain RuntimeError would have silently converted graceful degradation into crashes at four call sites.

Resolving before os.chdir is the subtle win, and it works

This is the part most likely to be "simplified" into a module-level wrapper later, so it's worth recording that the reasoning is empirically correct. I planted a git inside a caller-supplied repo directory and stubbed shutil.which to Windows' actual CWD-first order:

raised                : (none — operation succeeded)
num spawns            : 2
any spawn == the plant: False
all spawns absolute   : True
argv[0]               : /usr/bin/git
cwd restored          : True

The plant is neither executed nor allowed to cause a denial of service — the real /usr/bin/git runs and the checkout succeeds. A call-time wrapper resolving after the chdir would have found the plant and refused, turning someone else's plant into a failure for a user with a perfectly good git. Good call deviating from the ticket's suggested design, and test_checkout_tag_spawns_resolved_path_not_the_repo_plant pins it.

The false positive is real, deliberate, and correctly fail-closed

Reproduced it:

cwd = /usr/bin   (a genuine absolute PATH entry)
  resolve_binary('git')          -> None
  resolve_required_binary('git') -> BinaryNotFoundError:
      refusing to run 'git': the only match on PATH (/usr/bin/git) is in the …

I was going to suggest exempting directories that are explicit absolute $PATH entries to remove this — but the is_planted_in_cwd docstring already anticipates and rejects that, correctly: on Windows the implicit current-directory search is indistinguishable from an explicit entry, and PATH="$(pwd):$PATH" wrappers are themselves the vector. Weakening it there would blunt the fix on the exact platform it targets. Fail-closed is right; the error message explains itself well.

Still worth a release-note line, since this is the one user-visible change: running comfy from a directory that is itself a $PATH entry now fails git/ffmpeg commands rather than running them.

Other verification

• 172 passed across test_safe_exec.py, test_safe_exec_call_sites.py, output/test_inline_preview.py, command/test_preview.py, github/test_pr.py, test_update_version.py.
• Making build_preview_cmd's ffmpeg_bin a required keyword-only arg with no "ffmpeg" default is the right shape — it makes reintroducing the bare name a TypeError at import/call time rather than a silent regression.
• Using resolve_binary (not the required variant) in output/preview.py matches that previewer's documented skip-silently contract — a missing ffmpeg and a refused planted one degrade the same way.
• Catching the three sites the ticket didn't name (install.py:520, :532, :677) is the kind of thing that makes a mechanical sweep actually complete.

Unrelated note: import comfy_cli.git_utils as the first import raises a circular-import ImportError — but that reproduces identically on origin/main, so it's pre-existing and not this PR's problem. Might be worth its own ticket.

The GitPython and conda/npm/npx deferrals are correctly reasoned — git.refresh() changes import-time failure behaviour and deserves its own risk assessment rather than riding along here.