From 065f84f711453b289cac2bc3bd190767c095a6d9 Mon Sep 17 00:00:00 2001 From: Parthiban Sivakumar <228711334+parthiban-sivakumar@users.noreply.github.com> Date: Tue, 8 Sep 2026 20:59:52 +0530 Subject: [PATCH] fix(doctor): skip tool checks when tools block is empty (#5301) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(doctor): skip tool checks when tools block is empty Follow-up to #5296, which fixed this for `models:`. The same defect remains for `tools:`: `.get("tools", [])` returns None when the key is present but empty, because the default only applies when the key is absent. Iterating that None raises TypeError, which the surrounding broad handler renders as a check result: ! web search configured ('NoneType' object is not iterable) ! web fetch configured ('NoneType' object is not iterable) ! web capture configured ('NoneType' object is not iterable) ! image search configured ('NoneType' object is not iterable) ✗ sandbox configured ('NoneType' object is not iterable) Line 476 is reached by all four web/image checks through the shared check_web_tool helper, and line 645 by check_sandbox. Unlike the models case, a default install does not hit this: `make config` ships ten real tool entries, so a user has to empty or comment out that block first. The web checks now fall through to their normal "no tool in config" warning and the sandbox check evaluates normally. Parentheses on the comprehension are for readability; `or` already binds correctly there. Regression tests use the commented-out `tools:` shape that reproduces the failure, matching the tests added in #5296. Fixes #5300 Co-Authored-By: Claude Opus 5 * fix(doctor): skip non-mapping tool entries, tighten regression tests Review follow-ups on the line this PR already changes. A `tools:` list holding a scalar (`tools:\n - web_search`) reached `t.get("name")` and raised AttributeError, which the broad handler rendered as the check result: ! web search configured ('str' object has no attribute 'get') That is the same leakage this PR removes for the null case, so it is fixed here rather than deferred. `check_sandbox` already guards the same way via `isinstance(tool, dict)`. The empty-tools test asserted that "NoneType" was absent from the detail, which pins the failure mode rather than the behaviour — it would still pass if the detail became some other internal error text. Both tests now assert the expected message directly. Co-Authored-By: Claude Opus 5 * test(doctor): assert sandbox outcome instead of the failure mode Review follow-up, same class as the web-tool assertion fixed earlier in this PR. The sandbox regression test still asserted that "NoneType" was absent from the detail, which pins the failure mode rather than the outcome — it would keep passing if some other internal error text leaked out of the broad handler. On this config the path is deterministic: an empty `tools:` means no bash tool, so exactly one result. Assert the fields directly (`CheckResult` has no `__eq__`, so whole instances cannot be compared by value). Verified against `main`'s scripts/doctor.py, where the same config yields status=fail and detail="'NoneType' object is not iterable", so the new assertions are red there and green here. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- backend/tests/test_doctor.py | 29 +++++++++++++++++++++++++++++ scripts/doctor.py | 4 ++-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/backend/tests/test_doctor.py b/backend/tests/test_doctor.py index 97c0ba97d..9c8800cce 100644 --- a/backend/tests/test_doctor.py +++ b/backend/tests/test_doctor.py @@ -275,6 +275,24 @@ class TestCheckWebSearch: assert result.status == "ok" assert "DuckDuckGo" in result.detail + def test_commented_out_tools_block_warns_without_traceback(self, tmp_path): + # config.example.yaml ships a `tools:` key whose entries can all be + # commented out, so it parses as None rather than an empty list. + cfg = tmp_path / "config.yaml" + cfg.write_text("config_version: 5\ntools:\n # - name: web_search\n") + result = doctor.check_web_search(cfg) + assert result.status == "warn" + assert result.detail == "no web_search tool in config" + + def test_scalar_tools_entry_warns_without_traceback(self, tmp_path): + # A bare string entry is not a mapping; `t.get("name")` used to raise + # AttributeError, which the broad handler rendered as the check result. + cfg = tmp_path / "config.yaml" + cfg.write_text("config_version: 5\ntools:\n - web_search\n") + result = doctor.check_web_search(cfg) + assert result.status == "warn" + assert result.detail == "no web_search tool in config" + def test_tavily_with_key_ok(self, tmp_path, monkeypatch): monkeypatch.setenv("TAVILY_API_KEY", "tvly-test") cfg = tmp_path / "config.yaml" @@ -659,6 +677,17 @@ class TestCheckSandbox: results = doctor.check_sandbox(cfg) assert results[0].status == "fail" + def test_commented_out_tools_block_reports_no_traceback(self, tmp_path): + # Regression: iterating a null `tools:` raised TypeError, which the + # broad handler rendered as "('NoneType' object is not iterable)". + cfg = tmp_path / "config.yaml" + cfg.write_text("config_version: 5\nsandbox:\n use: deerflow.sandbox.local:LocalSandboxProvider\ntools:\n # - name: bash\n") + results = doctor.check_sandbox(cfg) + # Empty `tools:` means no bash tool, so the path is deterministic. + assert len(results) == 1 + assert results[0].status == "ok" + assert results[0].detail == "Local sandbox" + def test_local_sandbox_with_disabled_host_bash_warns(self, tmp_path): cfg = tmp_path / "config.yaml" cfg.write_text("config_version: 5\nsandbox:\n use: deerflow.sandbox.local:LocalSandboxProvider\n allow_host_bash: false\ntools:\n - name: bash\n use: deerflow.sandbox.tools:bash_tool\n") diff --git a/scripts/doctor.py b/scripts/doctor.py index b2830fd3b..ef998ebd1 100644 --- a/scripts/doctor.py +++ b/scripts/doctor.py @@ -473,7 +473,7 @@ def check_web_tool(config_path: Path, *, tool_name: str, label: str) -> CheckRes data = _load_yaml_file(config_path) - tool_entries = [t for t in data.get("tools", []) if t.get("name") == tool_name] + tool_entries = [t for t in (data.get("tools") or []) if isinstance(t, dict) and t.get("name") == tool_name] if not tool_entries: return CheckResult( label, @@ -642,7 +642,7 @@ def check_sandbox(config_path: Path) -> list[CheckResult]: ] sandbox_use = sandbox.get("use", "") - tools = data.get("tools", []) + tools = data.get("tools") or [] tool_names = {tool.get("name") for tool in tools if isinstance(tool, dict)} results: list[CheckResult] = []