Refactor config resolution logic with layered fallbacks and robust override support

[Arcitec]

@robinjhuang @yoland68 Hey guys, here's a clean overhaul which fixes a bunch of bugs (including a pretty serious CivitAI API token leak), and implements an easy way to script the functionality of Comfy-CLI without the huge hassle of having to provide CLI arguments every single time you invoke comfy from a user-script anymore. This new feature also makes it possible to provide tokens safely without writing them to disk/config or displaying them on the command-line anymore. Several users have expressed a desire for this feature.

The groundwork here also makes it very easy to extend these features to other variables if desired in the future.

I've tested this thoroughly in my local machine with all scenarios/combinations of settings, overrides and stored values. It's very robust and ready to merge.

When reviewing the code, be sure to look at the individual commits to make it much easier to review the changes, since every commit is self-contained and easier to understand that way.

I love what you've done with this project and wanted to contribute something back to you!

Here are all the commit summaries in reverse order:

fix: Add developer pytest instructions and improve readability

Adds instructions for locally running pytest to save time later during code reviews.

Also improves the readability of some other section titles and instructions.

feat: Add support for CIVITAI_TOKEN and HF_TOKEN environment variables

Closes #280.

Adds support for the standardized Hugging Face and CivitAI API token environment variables.

This provides a safe and easy way to provide transient secrets without leaking them to disk (nothing gets written to the config).

It also makes it much easier to integrate Comfy-CLI with scripting, since the user no longer has to infinitely provide the "--set-X-token" flags every single time they run the command anymore (which they had to do in the past, just to be sure that the latest token was always used).

The order of priority is --set-X-token, then the environment variables (if they exist), and lastly the static config fallback values.

This new feature is now documented in the README and in various error messages.

The code has also been refactored, and the error messages have been improved for clarity and consistency.

feat: Extend ConfigManager to support environment and CLI overrides

Adds new functionality which automatically gets, overrides or sets a config value intelligently based on the situation. See function docstring for details.

This makes it possible to effortlessly and reliably implement environment variable overrides for settings anywhere in Comfy-CLI.

fix: Improve clarity of download error messages

The messages are cute, but some people might not understand what "your file is in another castle" means, or what the awkward phrase "log into browser to download one" means. ;)

fix: Always use "Content-Type: application/json" when contacting CivitAI

The previous algorithm only applied the correct API Content-Type (JSON) when a token was available.

We now always apply the Content-Type, to ensure that we always get the expected responses.

fix: Only set CivitAI headers if the download URL is for CivitAI

The old code was leaking the CivitAI token by passing it to everything we download regardless of what domain we're fetching from.

fix: CivitAI API token sometimes not loaded from config

CivitAI token was only loaded from the config database if the "--set-hf-api-token" argument was missing, which made no sense.

Also removed a completely redundant variable assignment since all code paths set a value for it.

[Arcitec]

After re-reading and doing a bit of ripgrep on the codebase, I found more issues in the old code. So I force-pushed a few more tiny improvements to clarify another README line, improved another consistency issue with the old error messages, and fixed naming inconsistencies in all of the help/info strings (civitai, Civitai, CivitAI, huggingface, HuggingFace, Hugging Face which were all used randomly in user-facing strings).

There are no other pushes planned, so it's ready for review. 😸

[Arcitec]

@ltdrdata @bigcat88 Hey, since you also showed interest in this feature in the original issue ticket, I'm just letting you know that it's here now. 🥂

[Arcitec]

@bigcat88 Thanks for taking a look at this. I spotted something that I want to improve. I'll do it now.

[robinjhuang]

This LGTM. Thanks for contributing this!

[robinjhuang]

@bigcat88 Can you take a look as well?

[robinjhuang]

Looks like tests are failing, ruff as well. @Arcitec

[Arcitec]

@robinjhuang Glad to contribute! And woops, I know what's wrong. I'm so used to writing modern Python that I had to force myself to switch back to the old-school style used here. Well, I missed one spot. And I run Comfy-CLI with Python 3.10 so it didn't complain since it supports the modern style.

It's this return value type hint: -> str | None. It needs to be -> Optional[str]. The new syntax I am using was added in Python 3.10 four years ago and I've exclusively been using it since then, since the old style is discouraged by Python now.

I'll take care of it now and also double-check with Ruff. I'll squash the history too to keep this fix clean.

[Arcitec]

Alright, it's done! I squashed the fixes into the original commits they belong to. You can see the force-push changes here. It was the mentioned legacy type-hint style, and Ruff wanted to split a function call into 3 lines. That's fixed now.

https://github.com/Comfy-Org/comfy-cli/compare/7936170143757236e223f9d792a469737b2620c4..3ff1d32ac4b296b9c40882fc4f2da1cf4c6f1207

I also squashed the self-review which made the function more convenient to call with a default parameter value. Since that belongs in the original feature commit.

All the commits are clean now.

I noticed that you usually squash all PR commits into a single blob commit here. That's a good thing when PRs are messy with a bunch of "fix fix fix lint fix broken fix this fix that" WIP commits. That's not how I work. I don't mind whatever you do, but I designed it with an actual commit history in mind, so feel free to rebase-merge if you want that. Either way is fine by me as long as it gets into the project to finally simplify Comfy-CLI scripting. Not having to specify --set-X token flags in every single comfy is gonna be a relief. 😸 🖖

[bigcat88]

With the exception of the ENV variable naming, all the changes in this PR look pretty good and ready to be merged in my opinion(after small adjustment to test suite_).

[Arcitec]

@bigcat88 I have fixed the pytest failure by squashing the fix into the original message-related commit. I didn't realize that you had tests which run checks against those exact strings. :) So I also added instructions to the dev document about how to run your pytests locally.

Here's the diff of the squash, but you can also always view them via the "force-push: Compare" links that GitHub puts above my message here when someone force-pushes.

https://github.com/Comfy-Org/comfy-cli/compare/3ff1d32ac4b296b9c40882fc4f2da1cf4c6f1207..d15889f85a211b46c414019e3f3ae2a2c8e672d6

The pytest instructions were added as a new, separate commit to keep the PR commits clean:

"fix: Add developer pytest instructions and improve readability"

As mentioned, I see that you do both blobby squashes and rebase merges here, so feel free to rebase merge if you want to preserve a good commit history where git blame is actually useful. :D

Edit: I see that my code editor had changed a 4. to 1. in the markdown list, so I force pushed that tiny fix too.

[codecov]

Codecov Report

❌ Patch coverage is 40.90909% with 13 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
comfy_cli/command/models/models.py	12.50%	7 Missing ⚠️
comfy_cli/config_manager.py	14.28%	6 Missing ⚠️

Flag	Coverage Δ
unittests	48.11% <40.90%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
comfy_cli/constants.py	100.00% <100.00%> (ø)
comfy_cli/file_utils.py	55.66% <100.00%> (ø)
comfy_cli/config_manager.py	51.25% <14.28%> (-3.55%)	⬇️
comfy_cli/command/models/models.py	34.04% <12.50%> (+0.88%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Arcitec]

Alright. Pushed the changes. Thanks @bigcat88 for a good code review. Your suggestion about the env names is very good for future consistency with other Comfy-CLI environment variable naming. It's also easier for users since it correlates to their --set-X naming. I had been so focused on using "the standardized names" that it didn't occur to me that we could just use our own instead. 😹

https://github.com/Comfy-Org/comfy-cli/compare/86a03e311f35aea21f78bb3f750da40f8e1cdc27..8844a9bf3e8ccaa69b68c555437fb32db4a594fd

Changed:

• Added constants keys for the environment variable names to store them in a central location.
• Error messages now use the constants rather than actually hardcoding the variable name.
• The env vars have been changed to match the --set-X naming pattern.

Looking forward to seeing this feature in the app. :)

---

PS: I'll say it again since I became aware that your default merge setting is "squash": It's not good for the git log history to squash this PR into a single commit. I'd recommend a rebase merge (or a rebase-with-merge-commit that links back to the PR), to make the history easier to read / blame / revert / etc. All the commits here are self-contained changes.

Otherwise people will go to a file, look at the history, see the squashed commit and see lots of changes in lots of other files. It's very annoying to read squashed commits... Especially when someone in the future wants to know how a feature works by looking at git blame to see the change and how it's implemented and they see 15 other changed files.

I know that it's unusual to see people working with clean commits, lol. But that's the point of Git. 😄 You work in branches, squash/rebase/fix commits until they are all clean and self-contained, and then you rebase those clean commits onto main. But most people just make a mess instead. 🤣

[Arcitec]

I see that codecov wants me to add a test suite for the new ConfigManager function. I've been thinking about it, but saw that there's no tests for ANY other ConfigManager functions. So that's not necessary, right? Someone would have to make a test framework for somehow testing the ConfigManager first.

[bigcat88]

PR looks very good, let's merge.

[Arcitec]

Thanks, I'm happy to hear it, and really appreciate your intelligent review. Glad we changed the env before merge. The change to the env naming is perfect for the future of this project and makes it so easy to add more env vars for other config overrides that we might need later. ❤️

[bigcat88]

Thank you for the PR, it was a pleasure to review it.