fix(doctor): skip LLM checks when models block is empty (#5296)

`config.example.yaml` ships a `models:` key with every entry commented
out, so it parses as None rather than an empty list and the `[]` default
in `.get("models", [])` never applies. Iterating that None raised
TypeError, which the surrounding broad handler rendered as a check
result:

    ✗ LLM API key check  ('NoneType' object is not iterable)
    ✗ LLM auth check  ('NoneType' object is not iterable)
    ✗ LLM package check  ('NoneType' object is not iterable)

Every fresh install hit this before configuring a model, turning one
actionable error into four and hiding the real "models configured" hint
behind internal exception text.

Fall back on a falsy value at the three iteration sites so the checks
return no results when nothing is configured. `check_models_configured`
gets the same treatment for consistency; it was already correct because
it tests truthiness rather than iterating.

The existing tests missed this because they use `models: []`, an
explicit empty list, which iterates fine. The added regression tests use
the commented-out shape that `make config` actually produces.

`make doctor` now reports 1 error instead of 4 on a fresh clone.

Fixes #5295

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
This commit is contained in:
Parthiban Sivakumar 2026-09-08 17:31:39 +05:30 committed by GitHub
parent a2808e8292
commit 611801d5c0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 26 additions and 4 deletions

View File

@ -187,6 +187,14 @@ class TestCheckModelsConfigured:
result = doctor.check_models_configured(tmp_path / "config.yaml")
assert result.status == "skip"
def test_commented_out_models_block(self, tmp_path):
# config.example.yaml ships a `models:` key whose entries are all
# commented out, so it parses as None rather than an empty list.
cfg = tmp_path / "config.yaml"
cfg.write_text("config_version: 5\nmodels:\n # - name: default\n")
result = doctor.check_models_configured(cfg)
assert result.status == "fail"
# ---------------------------------------------------------------------------
# check_llm_api_key
@ -216,6 +224,14 @@ class TestCheckLLMApiKey:
results = doctor.check_llm_api_key(tmp_path / "config.yaml")
assert results == []
def test_commented_out_models_block_returns_empty(self, tmp_path):
# Regression: iterating a null `models:` raised TypeError, which the
# broad handler rendered as "('NoneType' object is not iterable)".
cfg = tmp_path / "config.yaml"
cfg.write_text("config_version: 5\nmodels:\n # - name: default\n")
results = doctor.check_llm_api_key(cfg)
assert results == []
# ---------------------------------------------------------------------------
# check_llm_auth
@ -237,6 +253,12 @@ class TestCheckLLMAuth:
results = doctor.check_llm_auth(cfg)
assert any(result.status == "ok" and "Claude auth available" in result.label for result in results)
def test_commented_out_models_block_returns_empty(self, tmp_path):
cfg = tmp_path / "config.yaml"
cfg.write_text("config_version: 5\nmodels:\n # - name: default\n")
assert doctor.check_llm_auth(cfg) == []
assert doctor.check_llm_package(cfg) == []
# ---------------------------------------------------------------------------
# check_web_search

View File

@ -280,7 +280,7 @@ def check_models_configured(config_path: Path) -> CheckResult:
return CheckResult("models configured", "skip")
try:
data = _load_yaml_file(config_path)
models = data.get("models", [])
models = data.get("models") or []
if models:
return CheckResult("models configured", "ok", f"{len(models)} model(s)")
return CheckResult(
@ -326,7 +326,7 @@ def check_llm_api_key(config_path: Path) -> list[CheckResult]:
with open(config_path, encoding="utf-8") as f:
data = yaml.safe_load(f) or {}
for model in data.get("models", []):
for model in data.get("models") or []:
# Collect all values that look like $ENV_VAR references
def _collect_env_refs(obj: object) -> list[str]:
refs: list[str] = []
@ -373,7 +373,7 @@ def check_llm_package(config_path: Path) -> list[CheckResult]:
data = yaml.safe_load(f) or {}
seen_packages: set[str] = set()
for model in data.get("models", []):
for model in data.get("models") or []:
use = model.get("use", "")
if ":" in use:
package_path = use.split(":")[0]
@ -408,7 +408,7 @@ def check_llm_auth(config_path: Path) -> list[CheckResult]:
results: list[CheckResult] = []
try:
data = _load_yaml_file(config_path)
for model in data.get("models", []):
for model in data.get("models") or []:
use = model.get("use", "")
model_name = model.get("name", "default")