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.