From ea8db23d3f61894356b16bada26445c34eeed44a Mon Sep 17 00:00:00 2001 From: mountain Date: Thu, 9 Jul 2026 14:56:35 +0800 Subject: [PATCH 1/4] fix(agent): make parallel_tool_calls configurable; fixes Bedrock query/chat (#175) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit query/chat agents hardcoded ModelSettings(parallel_tool_calls=False). On Amazon Bedrock Claude models this is fatal: LiteLLM's Bedrock Converse transform turns any parallel_tool_calls value into a tool_choice object carrying only `{"disable_parallel_tool_use": ...}` with no `type` field (converse_transformation.py: the _parallel_tool_use_config merge), which Bedrock rejects with "tool_choice.type: Field required" — so every query and chat fails. Confirmed unfixed through the latest litellm 1.91.1 (BerriAI/litellm#27184 still open); bumping doesn't help. Expose it as a config knob instead of hardcoding: - New `parallel_tool_calls` config key, default None = omit the setting (let the provider decide). Omitting is the only value that works on Bedrock, and is benign elsewhere (query tools are read-only; chat writes are path-restricted). - config.resolve_parallel_tool_calls() validates bool/null (warns on junk); set/get_parallel_tool_calls() process-wide stash, set by cli._setup_llm_key, read in build_query_agent — same mechanism as extra_headers/timeout. - Users can still force sequential tool calls with `parallel_tool_calls: false` on providers that support it. - Docs: config.yaml.example + configuration README (key table + a Bedrock setup section, which the repo previously lacked entirely). Closes #175 --- config.yaml.example | 6 +++++ examples/configuration/README.md | 25 ++++++++++++++++++ openkb/agent/query.py | 4 +-- openkb/cli.py | 5 ++++ openkb/config.py | 44 ++++++++++++++++++++++++++++++++ tests/conftest.py | 6 +++-- tests/test_config.py | 38 +++++++++++++++++++++++++++ tests/test_query.py | 24 +++++++++++++++-- 8 files changed, 146 insertions(+), 6 deletions(-) diff --git a/config.yaml.example b/config.yaml.example index eebf3bf6..7ff96a53 100644 --- a/config.yaml.example +++ b/config.yaml.example @@ -2,6 +2,12 @@ model: gpt-5.4 # LLM model (any LiteLLM-supported provider) language: en # Wiki output language pageindex_threshold: 20 # PDF pages threshold for PageIndex +# Optional: whether query/chat agents may call tools in parallel. Omit (the +# default) to let the provider decide. Leave it unset for Amazon Bedrock Claude +# models — sending it makes LiteLLM emit a malformed tool_choice and every +# query/chat fails. Set false to force sequential tool calls on other providers. +# parallel_tool_calls: false + # Optional: override the entity-type vocabulary used for entity pages. # Omit this key to use the default 7 types # (person, organization, place, product, work, event, other). diff --git a/examples/configuration/README.md b/examples/configuration/README.md index 58cb7d4d..6f1b8eaf 100644 --- a/examples/configuration/README.md +++ b/examples/configuration/README.md @@ -70,6 +70,12 @@ model: gpt-5.4 # LLM model (any LiteLLM-supported provider) language: en # Wiki output language pageindex_threshold: 20 # PDF pages threshold for PageIndex +# Optional: whether query/chat agents may call tools in parallel. Omit (the +# default) to let the provider decide. Leave it unset for Amazon Bedrock Claude +# models — sending it makes LiteLLM emit a malformed tool_choice and every +# query/chat fails. Set false to force sequential tool calls on other providers. +# parallel_tool_calls: false + # Optional: override the entity-type vocabulary used for entity pages. # Omit this key to use the default 7 types # (person, organization, place, product, work, event, other). @@ -95,6 +101,7 @@ pageindex_threshold: 20 # PDF pages threshold for PageIndex | `model` | `gpt-5.4` | LLM used for all compile/query/chat work. | | `language` | `en` | Language the wiki is written in. | | `pageindex_threshold` | `20` | PDFs with this many pages **or more** take the long-doc (PageIndex) path; shorter ones go through the short-doc path. See [`pageindex-cloud/`](../pageindex-cloud/). | +| `parallel_tool_calls` | *(unset)* | Whether query/chat agents may call tools in parallel. Unset = provider default. **Leave unset for Amazon Bedrock** (see below). Set `false` to force sequential tool calls elsewhere. | | `entity_types` | 7 defaults | Custom vocabulary for entity pages. `other` is always kept. | | `litellm:` | – | A pass-through block for LiteLLM. See below. | @@ -177,6 +184,24 @@ LLM_API_KEY=your-key-here won't warn about a missing one. - **PageIndex Cloud** uses a separate `PAGEINDEX_API_KEY` (see [`pageindex-cloud/`](../pageindex-cloud/)). +- **Amazon Bedrock** (`model: bedrock/...`) authenticates with AWS credentials, + not `LLM_API_KEY`. Put them in `/.env` (LiteLLM/boto3 read them from the + environment); `LLM_API_KEY` isn't needed: + + ```bash + # /.env + AWS_ACCESS_KEY_ID=... + AWS_SECRET_ACCESS_KEY=... + AWS_REGION_NAME=eu-central-1 + ``` + + ```yaml + # /.openkb/config.yaml + model: bedrock/eu.anthropic.claude-sonnet-4-6 + # Do NOT set parallel_tool_calls for Bedrock Claude — leaving it unset (the + # default) is what keeps query/chat working; setting it makes LiteLLM send a + # malformed tool_choice that Bedrock rejects (issue #175). + ``` **Where keys are read from** (first match wins, existing env always respected): diff --git a/openkb/agent/query.py b/openkb/agent/query.py index 20414cf7..b9de9025 100644 --- a/openkb/agent/query.py +++ b/openkb/agent/query.py @@ -12,7 +12,7 @@ read_wiki_image, write_kb_file, ) -from openkb.config import get_extra_headers, get_timeout_extra_args +from openkb.config import get_extra_headers, get_parallel_tool_calls, get_timeout_extra_args from openkb.schema import get_agents_md MAX_TURNS = 50 @@ -96,7 +96,7 @@ def get_image(image_path: str) -> ToolOutputImage | ToolOutputText: tools=[read_file, get_page_content, get_image], model=f"litellm/{model}", model_settings=ModelSettings( - parallel_tool_calls=False, + parallel_tool_calls=get_parallel_tool_calls(), extra_headers=get_extra_headers() or None, extra_args=get_timeout_extra_args(), ), diff --git a/openkb/cli.py b/openkb/cli.py index f39d0b44..1edd2308 100644 --- a/openkb/cli.py +++ b/openkb/cli.py @@ -56,6 +56,8 @@ def filter(self, record: logging.LogRecord) -> bool: register_kb, resolve_extra_headers, set_extra_headers, + resolve_parallel_tool_calls, + set_parallel_tool_calls, resolve_timeout, set_timeout, resolve_litellm_settings, @@ -174,6 +176,7 @@ def _setup_llm_key(kb_dir: Path | None = None) -> None: provider: str | None = None extra_headers: dict[str, str] = {} timeout: float | None = None + parallel_tool_calls: bool | None = None litellm_settings: dict = {} if kb_dir is not None: config_path = kb_dir / ".openkb" / "config.yaml" @@ -183,6 +186,7 @@ def _setup_llm_key(kb_dir: Path | None = None) -> None: provider = _extract_provider(str(model)) extra_headers = resolve_extra_headers(config) timeout = resolve_timeout(config) + parallel_tool_calls = resolve_parallel_tool_calls(config) litellm_settings = resolve_litellm_settings(config) # `timeout` / `extra_headers` in the block route to the per-call # stashes (replacing the legacy top-level keys); the rest are globals. @@ -194,6 +198,7 @@ def _setup_llm_key(kb_dir: Path | None = None) -> None: timeout = resolve_timeout({"timeout": litellm_settings.pop("timeout")}) set_extra_headers(extra_headers) set_timeout(timeout) + set_parallel_tool_calls(parallel_tool_calls) _apply_litellm_settings(litellm_settings) if not api_key: diff --git a/openkb/config.py b/openkb/config.py index a5ee010b..8e3807f3 100644 --- a/openkb/config.py +++ b/openkb/config.py @@ -17,6 +17,12 @@ "model": "gpt-5.4", "language": "en", "pageindex_threshold": 20, + # Whether query/chat agents may call tools in parallel. None (default) omits + # the setting so the provider decides — required for Amazon Bedrock's Claude + # models, where sending parallel_tool_calls makes LiteLLM emit a malformed + # tool_choice (missing `type`) and every query/chat fails (issue #175). Set + # false to force sequential tool calls (fine on OpenAI/Anthropic-direct). + "parallel_tool_calls": None, } # Default entity-type vocabulary. Overridable per-KB via the optional @@ -144,6 +150,26 @@ def resolve_extra_headers(config: dict) -> dict[str, str]: return headers +def resolve_parallel_tool_calls(config: dict) -> bool | None: + """Resolve the optional ``parallel_tool_calls:`` key. + + Returns ``None`` (omit the setting — provider default) when absent or + explicitly null; ``True``/``False`` when set to a bool. A non-bool value is + invalid and treated as unset, with a warning. ``None`` is the normal + "unset" case and warns silently, matching ``resolve_timeout``. + """ + value = config.get("parallel_tool_calls") + if value is None: + return None + if not isinstance(value, bool): + logger.warning( + "config: 'parallel_tool_calls' must be true or false, got %r — ignoring it.", + value, + ) + return None + return value + + def resolve_timeout(config: dict) -> float | None: """Resolve the optional ``timeout:`` key to a finite positive number of seconds. @@ -243,6 +269,24 @@ def get_timeout_extra_args() -> dict[str, float] | None: return {"timeout": _runtime_timeout} if _runtime_timeout is not None else None +# Process-wide agent ``parallel_tool_calls`` setting, resolved from config by +# the CLI (cli._setup_llm_key) and read when building query/chat agents. None = +# omit the setting (provider default) — see the DEFAULT_CONFIG note on why +# Bedrock needs this. +_runtime_parallel_tool_calls: bool | None = None + + +def set_parallel_tool_calls(value: bool | None) -> None: + """Set the process-wide agent ``parallel_tool_calls`` setting.""" + global _runtime_parallel_tool_calls + _runtime_parallel_tool_calls = value + + +def get_parallel_tool_calls() -> bool | None: + """Return the process-wide agent ``parallel_tool_calls`` setting (or None).""" + return _runtime_parallel_tool_calls + + def load_config(config_path: Path) -> dict[str, Any]: """Load YAML config from config_path, merged with DEFAULT_CONFIG. diff --git a/tests/conftest.py b/tests/conftest.py index 3d67b4b4..67cf7fba 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -5,12 +5,14 @@ @pytest.fixture(autouse=True) def _reset_extra_headers(): - """Keep the process-wide LLM extra-headers / timeout stashes from leaking across tests.""" - from openkb.config import set_extra_headers, set_timeout + """Keep the process-wide LLM extra-headers / timeout / parallel-tool-calls + stashes from leaking across tests.""" + from openkb.config import set_extra_headers, set_parallel_tool_calls, set_timeout yield set_extra_headers({}) set_timeout(None) + set_parallel_tool_calls(None) @pytest.fixture diff --git a/tests/test_config.py b/tests/test_config.py index be473fcd..6cec9182 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -3,17 +3,55 @@ from openkb.config import ( DEFAULT_CONFIG, get_extra_headers, + get_parallel_tool_calls, get_timeout, load_config, resolve_extra_headers, resolve_litellm_settings, + resolve_parallel_tool_calls, resolve_timeout, save_config, set_extra_headers, + set_parallel_tool_calls, set_timeout, ) +def test_parallel_tool_calls_defaults_to_none(): + assert DEFAULT_CONFIG["parallel_tool_calls"] is None + + +def test_resolve_parallel_tool_calls_absent_is_none(): + assert resolve_parallel_tool_calls({}) is None + + +def test_resolve_parallel_tool_calls_explicit_bools(): + assert resolve_parallel_tool_calls({"parallel_tool_calls": True}) is True + assert resolve_parallel_tool_calls({"parallel_tool_calls": False}) is False + + +def test_resolve_parallel_tool_calls_none_is_silent(caplog): + with caplog.at_level(logging.WARNING, logger="openkb.config"): + assert resolve_parallel_tool_calls({"parallel_tool_calls": None}) is None + assert caplog.text == "" + + +def test_resolve_parallel_tool_calls_rejects_non_bool(caplog): + # A non-bool (e.g. a string or int) is invalid → treat as unset, with a warning. + with caplog.at_level(logging.WARNING, logger="openkb.config"): + assert resolve_parallel_tool_calls({"parallel_tool_calls": "true"}) is None + assert "parallel_tool_calls" in caplog.text + + +def test_parallel_tool_calls_stash_roundtrip(): + set_parallel_tool_calls(False) + assert get_parallel_tool_calls() is False + set_parallel_tool_calls(True) + assert get_parallel_tool_calls() is True + set_parallel_tool_calls(None) + assert get_parallel_tool_calls() is None + + def test_default_config_keys(): assert "model" in DEFAULT_CONFIG assert "language" in DEFAULT_CONFIG diff --git a/tests/test_query.py b/tests/test_query.py index f9e68653..09fd6e91 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -154,14 +154,34 @@ def test_extra_headers_applied_from_stash(self, tmp_path): set_extra_headers({"Editor-Version": "vscode/1.95.0"}) agent = build_query_agent(str(tmp_path), "github_copilot/gpt-5-mini") assert agent.model_settings.extra_headers == {"Editor-Version": "vscode/1.95.0"} - # Existing settings are preserved. - assert agent.model_settings.parallel_tool_calls is False def test_no_extra_headers_by_default(self, tmp_path): agent = build_query_agent(str(tmp_path), "gpt-4o-mini") assert agent.model_settings.extra_headers is None +class TestQueryAgentParallelToolCalls: + """Config-driven parallel_tool_calls reaches the agents-SDK model settings. + + Default is None (omit the param → provider default), which is what keeps + Bedrock's Claude models working: passing parallel_tool_calls at all makes + LiteLLM emit a tool_choice object missing the required `type` field (see + issue #175). Users can still force sequential tool calls with + `parallel_tool_calls: false`. + """ + + def test_omitted_by_default(self, tmp_path): + agent = build_query_agent(str(tmp_path), "bedrock/eu.anthropic.claude-sonnet-4-6") + assert agent.model_settings.parallel_tool_calls is None + + def test_applied_from_stash(self, tmp_path): + from openkb.config import set_parallel_tool_calls + + set_parallel_tool_calls(False) + agent = build_query_agent(str(tmp_path), "gpt-4o-mini") + assert agent.model_settings.parallel_tool_calls is False + + class TestQueryAgentTimeout: """Config-driven timeout reaches the agents-SDK model settings via extra_args. From 2c30d1897c431ed2fe376147bc29c6852808be98 Mon Sep 17 00:00:00 2001 From: mountain Date: Thu, 9 Jul 2026 15:22:45 +0800 Subject: [PATCH 2/4] refactor(config): default parallel_tool_calls to false; null = omit for Bedrock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revised per review: the default must NOT change existing behavior. Keep the historical default (false = force sequential tool calls) for every provider, and make `null` the explicit "don't send the setting" escape hatch that Bedrock Claude needs (sending any value trips the LiteLLM malformed-tool_choice bug, #175). No provider special-casing in code — it's purely the config value. - DEFAULT_CONFIG parallel_tool_calls: false (was None). - resolve_parallel_tool_calls: absent → false (default); true/false → that; explicit null → None (omit, silent); other → default + warning. - Docs updated: default false, set `null` for Bedrock. --- config.yaml.example | 12 ++++++---- examples/configuration/README.md | 21 +++++++++-------- openkb/cli.py | 2 +- openkb/config.py | 39 +++++++++++++++++++++----------- tests/test_config.py | 18 +++++++++------ tests/test_query.py | 25 +++++++++++++------- 6 files changed, 74 insertions(+), 43 deletions(-) diff --git a/config.yaml.example b/config.yaml.example index 7ff96a53..a3ac8429 100644 --- a/config.yaml.example +++ b/config.yaml.example @@ -2,11 +2,13 @@ model: gpt-5.4 # LLM model (any LiteLLM-supported provider) language: en # Wiki output language pageindex_threshold: 20 # PDF pages threshold for PageIndex -# Optional: whether query/chat agents may call tools in parallel. Omit (the -# default) to let the provider decide. Leave it unset for Amazon Bedrock Claude -# models — sending it makes LiteLLM emit a malformed tool_choice and every -# query/chat fails. Set false to force sequential tool calls on other providers. -# parallel_tool_calls: false +# Optional: whether query/chat agents may call tools in parallel. +# false (default) force sequential tool calls +# true allow parallel tool calls +# null don't send the setting (use the provider default) — set this +# for Amazon Bedrock Claude, which rejects the request when +# parallel_tool_calls is sent at all (any value). See #175. +# parallel_tool_calls: null # Optional: override the entity-type vocabulary used for entity pages. # Omit this key to use the default 7 types diff --git a/examples/configuration/README.md b/examples/configuration/README.md index 6f1b8eaf..f988749d 100644 --- a/examples/configuration/README.md +++ b/examples/configuration/README.md @@ -70,11 +70,13 @@ model: gpt-5.4 # LLM model (any LiteLLM-supported provider) language: en # Wiki output language pageindex_threshold: 20 # PDF pages threshold for PageIndex -# Optional: whether query/chat agents may call tools in parallel. Omit (the -# default) to let the provider decide. Leave it unset for Amazon Bedrock Claude -# models — sending it makes LiteLLM emit a malformed tool_choice and every -# query/chat fails. Set false to force sequential tool calls on other providers. -# parallel_tool_calls: false +# Optional: whether query/chat agents may call tools in parallel. +# false (default) force sequential tool calls +# true allow parallel tool calls +# null don't send the setting (use the provider default) — set this +# for Amazon Bedrock Claude, which rejects the request when +# parallel_tool_calls is sent at all (any value). See #175. +# parallel_tool_calls: null # Optional: override the entity-type vocabulary used for entity pages. # Omit this key to use the default 7 types @@ -101,7 +103,7 @@ pageindex_threshold: 20 # PDF pages threshold for PageIndex | `model` | `gpt-5.4` | LLM used for all compile/query/chat work. | | `language` | `en` | Language the wiki is written in. | | `pageindex_threshold` | `20` | PDFs with this many pages **or more** take the long-doc (PageIndex) path; shorter ones go through the short-doc path. See [`pageindex-cloud/`](../pageindex-cloud/). | -| `parallel_tool_calls` | *(unset)* | Whether query/chat agents may call tools in parallel. Unset = provider default. **Leave unset for Amazon Bedrock** (see below). Set `false` to force sequential tool calls elsewhere. | +| `parallel_tool_calls` | `false` | Whether query/chat agents may call tools in parallel. `false` (default) forces sequential; `true` allows parallel; `null` omits the setting (provider default). **Amazon Bedrock needs `null`** (see below). | | `entity_types` | 7 defaults | Custom vocabulary for entity pages. `other` is always kept. | | `litellm:` | – | A pass-through block for LiteLLM. See below. | @@ -198,9 +200,10 @@ LLM_API_KEY=your-key-here ```yaml # /.openkb/config.yaml model: bedrock/eu.anthropic.claude-sonnet-4-6 - # Do NOT set parallel_tool_calls for Bedrock Claude — leaving it unset (the - # default) is what keeps query/chat working; setting it makes LiteLLM send a - # malformed tool_choice that Bedrock rejects (issue #175). + parallel_tool_calls: null # REQUIRED for Bedrock Claude: the default (false) + # — and any explicit value — makes LiteLLM send a + # malformed tool_choice that Bedrock rejects (#175). + # null tells OpenKB not to send the setting at all. ``` **Where keys are read from** (first match wins, existing env always respected): diff --git a/openkb/cli.py b/openkb/cli.py index 1edd2308..785a8232 100644 --- a/openkb/cli.py +++ b/openkb/cli.py @@ -176,7 +176,7 @@ def _setup_llm_key(kb_dir: Path | None = None) -> None: provider: str | None = None extra_headers: dict[str, str] = {} timeout: float | None = None - parallel_tool_calls: bool | None = None + parallel_tool_calls: bool | None = DEFAULT_CONFIG["parallel_tool_calls"] litellm_settings: dict = {} if kb_dir is not None: config_path = kb_dir / ".openkb" / "config.yaml" diff --git a/openkb/config.py b/openkb/config.py index 8e3807f3..27b3e207 100644 --- a/openkb/config.py +++ b/openkb/config.py @@ -17,12 +17,13 @@ "model": "gpt-5.4", "language": "en", "pageindex_threshold": 20, - # Whether query/chat agents may call tools in parallel. None (default) omits - # the setting so the provider decides — required for Amazon Bedrock's Claude - # models, where sending parallel_tool_calls makes LiteLLM emit a malformed - # tool_choice (missing `type`) and every query/chat fails (issue #175). Set - # false to force sequential tool calls (fine on OpenAI/Anthropic-direct). - "parallel_tool_calls": None, + # Whether query/chat agents may call tools in parallel. Default false = + # force sequential tool calls (historical behavior). true = allow parallel. + # null = don't send the setting at all (use the provider default) — the + # escape hatch for Amazon Bedrock Claude, where sending parallel_tool_calls + # (any value) makes LiteLLM emit a malformed tool_choice missing `type`, so + # every query/chat fails (issue #175). + "parallel_tool_calls": False, } # Default entity-type vocabulary. Overridable per-KB via the optional @@ -153,20 +154,32 @@ def resolve_extra_headers(config: dict) -> dict[str, str]: def resolve_parallel_tool_calls(config: dict) -> bool | None: """Resolve the optional ``parallel_tool_calls:`` key. - Returns ``None`` (omit the setting — provider default) when absent or - explicitly null; ``True``/``False`` when set to a bool. A non-bool value is - invalid and treated as unset, with a warning. ``None`` is the normal - "unset" case and warns silently, matching ``resolve_timeout``. + Tri-state: + * key absent → the default (``False`` — force sequential tool calls). + * ``true`` / ``false`` → that bool. + * explicit ``null`` → ``None``, meaning "don't send the setting" so the + provider's own default applies. This is the escape hatch for Amazon + Bedrock Claude, which rejects the request when the param is sent at all. + + A non-bool, non-null value is invalid → falls back to the default with a + warning. An explicit ``null`` is a valid choice and warns silently. + + Note this relies on the config being merged with ``DEFAULT_CONFIG`` (as + ``load_config`` does), so an omitted key reads back as ``False`` while an + explicit ``null`` reads back as ``None`` — the two are distinguishable. """ - value = config.get("parallel_tool_calls") + default = DEFAULT_CONFIG["parallel_tool_calls"] + value = config.get("parallel_tool_calls", default) if value is None: return None if not isinstance(value, bool): logger.warning( - "config: 'parallel_tool_calls' must be true or false, got %r — ignoring it.", + "config: 'parallel_tool_calls' must be true, false, or null, got %r " + "— using default (%r).", value, + default, ) - return None + return default return value diff --git a/tests/test_config.py b/tests/test_config.py index 6cec9182..e70778af 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -17,12 +17,14 @@ ) -def test_parallel_tool_calls_defaults_to_none(): - assert DEFAULT_CONFIG["parallel_tool_calls"] is None +def test_parallel_tool_calls_defaults_to_false(): + # Default preserves the historical behavior: force sequential tool calls. + assert DEFAULT_CONFIG["parallel_tool_calls"] is False -def test_resolve_parallel_tool_calls_absent_is_none(): - assert resolve_parallel_tool_calls({}) is None +def test_resolve_parallel_tool_calls_absent_is_false(): + # Key omitted → the default (force sequential), same as before this knob existed. + assert resolve_parallel_tool_calls({}) is False def test_resolve_parallel_tool_calls_explicit_bools(): @@ -30,16 +32,18 @@ def test_resolve_parallel_tool_calls_explicit_bools(): assert resolve_parallel_tool_calls({"parallel_tool_calls": False}) is False -def test_resolve_parallel_tool_calls_none_is_silent(caplog): +def test_resolve_parallel_tool_calls_null_means_omit(caplog): + # Explicit null = "don't send the param" (provider default). This is the + # escape hatch for Amazon Bedrock, and is silent (not an invalid value). with caplog.at_level(logging.WARNING, logger="openkb.config"): assert resolve_parallel_tool_calls({"parallel_tool_calls": None}) is None assert caplog.text == "" def test_resolve_parallel_tool_calls_rejects_non_bool(caplog): - # A non-bool (e.g. a string or int) is invalid → treat as unset, with a warning. + # A non-bool, non-null value is invalid → fall back to the default, with a warning. with caplog.at_level(logging.WARNING, logger="openkb.config"): - assert resolve_parallel_tool_calls({"parallel_tool_calls": "true"}) is None + assert resolve_parallel_tool_calls({"parallel_tool_calls": "true"}) is False assert "parallel_tool_calls" in caplog.text diff --git a/tests/test_query.py b/tests/test_query.py index 09fd6e91..df705aca 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -161,26 +161,35 @@ def test_no_extra_headers_by_default(self, tmp_path): class TestQueryAgentParallelToolCalls: - """Config-driven parallel_tool_calls reaches the agents-SDK model settings. + """The resolved parallel_tool_calls value (stash) reaches the model settings. - Default is None (omit the param → provider default), which is what keeps - Bedrock's Claude models working: passing parallel_tool_calls at all makes - LiteLLM emit a tool_choice object missing the required `type` field (see - issue #175). Users can still force sequential tool calls with - `parallel_tool_calls: false`. + Default is False (force sequential — unchanged historical behavior). A + config value of null resolves to None, which the agents-SDK omits from the + request — the escape hatch for Amazon Bedrock, whose Claude models reject + the request when parallel_tool_calls is sent at all (issue #175). """ - def test_omitted_by_default(self, tmp_path): + def test_null_stash_is_omitted(self, tmp_path): + from openkb.config import set_parallel_tool_calls + + set_parallel_tool_calls(None) agent = build_query_agent(str(tmp_path), "bedrock/eu.anthropic.claude-sonnet-4-6") assert agent.model_settings.parallel_tool_calls is None - def test_applied_from_stash(self, tmp_path): + def test_false_stash_forces_sequential(self, tmp_path): from openkb.config import set_parallel_tool_calls set_parallel_tool_calls(False) agent = build_query_agent(str(tmp_path), "gpt-4o-mini") assert agent.model_settings.parallel_tool_calls is False + def test_true_stash_allows_parallel(self, tmp_path): + from openkb.config import set_parallel_tool_calls + + set_parallel_tool_calls(True) + agent = build_query_agent(str(tmp_path), "gpt-4o-mini") + assert agent.model_settings.parallel_tool_calls is True + class TestQueryAgentTimeout: """Config-driven timeout reaches the agents-SDK model settings via extra_args. From bbcd63aab7be7d910c22d73cbb27a78fe03e1003 Mon Sep 17 00:00:00 2001 From: mountain Date: Thu, 9 Jul 2026 15:49:57 +0800 Subject: [PATCH 3/4] refactor(config): centralize agent ModelSettings via resolve_model_settings() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding an agent-facing LLM knob previously meant touching every agent builder to read yet another get_*() (extra_headers, timeout_extra_args, parallel_tool_calls, …) — and it was easy to forget one: build_lint_agent never set parallel_tool_calls, so query and lint silently disagreed on it. Add config.resolve_model_settings(), the single place that assembles all agents-SDK ModelSettings kwargs from the process-wide runtime stashes. Both build_query_agent and build_lint_agent now do `ModelSettings(**resolve_model_settings())`, so a new knob is wired in one place and every agent stays in sync. It assembles on read from the existing getters, preserving "set_extra_headers(...) reaches the agent" (used by the compiler and by tests) rather than snapshotting into a separate stash. Note: build_lint_agent now also honors parallel_tool_calls (it didn't before), making lint consistent with query/chat — including needing `null` on Bedrock. --- openkb/agent/linter.py | 7 ++----- openkb/agent/query.py | 8 ++------ openkb/config.py | 17 +++++++++++++++++ tests/test_config.py | 25 +++++++++++++++++++++++++ 4 files changed, 46 insertions(+), 11 deletions(-) diff --git a/openkb/agent/linter.py b/openkb/agent/linter.py index 365f5602..e3604289 100644 --- a/openkb/agent/linter.py +++ b/openkb/agent/linter.py @@ -8,7 +8,7 @@ from agents.model_settings import ModelSettings from openkb.agent.tools import list_wiki_files, read_wiki_file -from openkb.config import get_extra_headers, get_timeout_extra_args +from openkb.config import resolve_model_settings from openkb.schema import get_agents_md MAX_TURNS = 50 @@ -82,10 +82,7 @@ def read_file(path: str) -> str: instructions=instructions, tools=[list_files, read_file], model=f"litellm/{model}", - model_settings=ModelSettings( - extra_headers=get_extra_headers() or None, - extra_args=get_timeout_extra_args(), - ), + model_settings=ModelSettings(**resolve_model_settings()), ) diff --git a/openkb/agent/query.py b/openkb/agent/query.py index b9de9025..885ec235 100644 --- a/openkb/agent/query.py +++ b/openkb/agent/query.py @@ -12,7 +12,7 @@ read_wiki_image, write_kb_file, ) -from openkb.config import get_extra_headers, get_parallel_tool_calls, get_timeout_extra_args +from openkb.config import resolve_model_settings from openkb.schema import get_agents_md MAX_TURNS = 50 @@ -95,11 +95,7 @@ def get_image(image_path: str) -> ToolOutputImage | ToolOutputText: instructions=instructions, tools=[read_file, get_page_content, get_image], model=f"litellm/{model}", - model_settings=ModelSettings( - parallel_tool_calls=get_parallel_tool_calls(), - extra_headers=get_extra_headers() or None, - extra_args=get_timeout_extra_args(), - ), + model_settings=ModelSettings(**resolve_model_settings()), ) diff --git a/openkb/config.py b/openkb/config.py index 27b3e207..6ba8a000 100644 --- a/openkb/config.py +++ b/openkb/config.py @@ -300,6 +300,23 @@ def get_parallel_tool_calls() -> bool | None: return _runtime_parallel_tool_calls +def resolve_model_settings() -> dict[str, Any]: + """Assemble the agents-SDK ``ModelSettings`` kwargs from the process-wide LLM + runtime settings (populated by ``cli._setup_llm_key``). + + This is the single place that maps runtime LLM config onto agent model + settings: every agent builder does ``ModelSettings(**resolve_model_settings())`` + rather than enumerating the individual getters, so a new agent-facing knob + is wired in here once and can't be silently forgotten by one builder. + ``None`` values are what the agents SDK treats as "unset / provider default". + """ + return { + "extra_headers": get_extra_headers() or None, + "extra_args": get_timeout_extra_args(), + "parallel_tool_calls": get_parallel_tool_calls(), + } + + def load_config(config_path: Path) -> dict[str, Any]: """Load YAML config from config_path, merged with DEFAULT_CONFIG. diff --git a/tests/test_config.py b/tests/test_config.py index e70778af..4f4fe9b5 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -8,6 +8,7 @@ load_config, resolve_extra_headers, resolve_litellm_settings, + resolve_model_settings, resolve_parallel_tool_calls, resolve_timeout, save_config, @@ -17,6 +18,30 @@ ) +def test_resolve_model_settings_assembles_from_runtime_stashes(): + # One place that assembles every agents-SDK ModelSettings kwarg from the + # process-wide runtime stashes, so agent builders don't each enumerate them. + set_extra_headers({"X-A": "1"}) + set_timeout(1200.0) + set_parallel_tool_calls(False) + assert resolve_model_settings() == { + "extra_headers": {"X-A": "1"}, + "extra_args": {"timeout": 1200.0}, + "parallel_tool_calls": False, + } + + +def test_resolve_model_settings_empty_is_all_omitted(): + set_extra_headers({}) + set_timeout(None) + set_parallel_tool_calls(None) + assert resolve_model_settings() == { + "extra_headers": None, + "extra_args": None, + "parallel_tool_calls": None, + } + + def test_parallel_tool_calls_defaults_to_false(): # Default preserves the historical behavior: force sequential tool calls. assert DEFAULT_CONFIG["parallel_tool_calls"] is False From 306e75dd291035c79798da5204f8656c5b729768 Mon Sep 17 00:00:00 2001 From: mountain Date: Thu, 9 Jul 2026 17:07:36 +0800 Subject: [PATCH 4/4] fix(config): per-agent parallel_tool_calls defaults; fix lint & skill regressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The earlier refactor on this branch collapsed three agent builders' distinct historical parallel_tool_calls defaults onto one shared value, changing behavior for two of them: - lint agent: never sent parallel_tool_calls before; the refactor made it send `false` by default, re-triggering the Bedrock #175 malformed-tool_choice bug for `openkb lint` (LiteLLM's Bedrock transform breaks on ANY value). - skill-create agent: kept a hardcoded `parallel_tool_calls=True` that ignored config entirely, so `openkb skill new` stayed broken on Bedrock even with the `parallel_tool_calls: null` escape hatch set. Make resolution tri-state — resolve_parallel_tool_calls now returns (value, was_explicit): - key absent -> each agent applies its OWN historical default via resolve_model_settings(default_parallel_tool_calls=...): query/chat False, lint None (omit), skill-create True. - true/false/null -> applies uniformly to every agent (the escape hatch works regardless of which agent runs). - invalid value -> omit (the one value that never breaks a provider), instead of the old fall-back to False that could silently re-break Bedrock. parallel_tool_calls is dropped from DEFAULT_CONFIG so load_config's merge can't mask "not configured"; the runtime stash carries the was_explicit flag. Tool-less eval agents (skill/evaluator.py) deliberately stay unmigrated and always omit the setting — the SDK forwards an explicit `false` even with no tools, which strict OpenAI endpoints reject. Guarded by tests. Docs: the setting now scopes to all LLM agents (query/chat/lint/skill), documents the unset-per-agent-default semantics and the bare-`null` requirement. Adds regression tests across config/query/linter/skill-creator/skill-evaluator. --- config.yaml.example | 14 +-- examples/configuration/README.md | 25 +++--- openkb/agent/linter.py | 2 +- openkb/cli.py | 7 +- openkb/config.py | 90 ++++++++----------- openkb/skill/creator.py | 22 ++--- tests/conftest.py | 2 +- tests/test_config.py | 124 ++++++++++++++++++--------- tests/test_linter.py | 28 ++++++ tests/test_llm_config_passthrough.py | 23 +++++ tests/test_query.py | 16 +++- tests/test_skill_creator.py | 43 +++++++++- tests/test_skill_evaluator.py | 56 ++++++++++++ 13 files changed, 317 insertions(+), 135 deletions(-) diff --git a/config.yaml.example b/config.yaml.example index a3ac8429..ac4f6396 100644 --- a/config.yaml.example +++ b/config.yaml.example @@ -2,12 +2,14 @@ model: gpt-5.4 # LLM model (any LiteLLM-supported provider) language: en # Wiki output language pageindex_threshold: 20 # PDF pages threshold for PageIndex -# Optional: whether query/chat agents may call tools in parallel. -# false (default) force sequential tool calls -# true allow parallel tool calls -# null don't send the setting (use the provider default) — set this -# for Amazon Bedrock Claude, which rejects the request when -# parallel_tool_calls is sent at all (any value). See #175. +# Optional: whether the LLM agents (query, chat, lint, skill) may call tools +# in parallel. Leave it UNSET (commented out) to keep OpenKB's per-agent +# defaults. Setting it applies the SAME value to every agent: +# true allow parallel tool calls +# false force sequential tool calls +# null don't send the setting at all (use the provider default) — REQUIRED +# for Amazon Bedrock Claude, which rejects the request when +# parallel_tool_calls is sent at all (any value). See #175. # parallel_tool_calls: null # Optional: override the entity-type vocabulary used for entity pages. diff --git a/examples/configuration/README.md b/examples/configuration/README.md index f988749d..cbf4c2a6 100644 --- a/examples/configuration/README.md +++ b/examples/configuration/README.md @@ -70,12 +70,14 @@ model: gpt-5.4 # LLM model (any LiteLLM-supported provider) language: en # Wiki output language pageindex_threshold: 20 # PDF pages threshold for PageIndex -# Optional: whether query/chat agents may call tools in parallel. -# false (default) force sequential tool calls -# true allow parallel tool calls -# null don't send the setting (use the provider default) — set this -# for Amazon Bedrock Claude, which rejects the request when -# parallel_tool_calls is sent at all (any value). See #175. +# Optional: whether the LLM agents (query, chat, lint, skill) may call tools +# in parallel. Leave it UNSET (commented out) to keep OpenKB's per-agent +# defaults. Setting it applies the SAME value to every agent: +# true allow parallel tool calls +# false force sequential tool calls +# null don't send the setting at all (use the provider default) — REQUIRED +# for Amazon Bedrock Claude, which rejects the request when +# parallel_tool_calls is sent at all (any value). See #175. # parallel_tool_calls: null # Optional: override the entity-type vocabulary used for entity pages. @@ -103,7 +105,7 @@ pageindex_threshold: 20 # PDF pages threshold for PageIndex | `model` | `gpt-5.4` | LLM used for all compile/query/chat work. | | `language` | `en` | Language the wiki is written in. | | `pageindex_threshold` | `20` | PDFs with this many pages **or more** take the long-doc (PageIndex) path; shorter ones go through the short-doc path. See [`pageindex-cloud/`](../pageindex-cloud/). | -| `parallel_tool_calls` | `false` | Whether query/chat agents may call tools in parallel. `false` (default) forces sequential; `true` allows parallel; `null` omits the setting (provider default). **Amazon Bedrock needs `null`** (see below). | +| `parallel_tool_calls` | unset | Whether the LLM agents (query, chat, lint, skill) may call tools in parallel. Unset keeps OpenKB's per-agent defaults; `true`/`false` force allow/sequential for every agent; `null` omits the setting (provider default). **Amazon Bedrock needs `null`** (see below). | | `entity_types` | 7 defaults | Custom vocabulary for entity pages. `other` is always kept. | | `litellm:` | – | A pass-through block for LiteLLM. See below. | @@ -200,10 +202,11 @@ LLM_API_KEY=your-key-here ```yaml # /.openkb/config.yaml model: bedrock/eu.anthropic.claude-sonnet-4-6 - parallel_tool_calls: null # REQUIRED for Bedrock Claude: the default (false) - # — and any explicit value — makes LiteLLM send a - # malformed tool_choice that Bedrock rejects (#175). - # null tells OpenKB not to send the setting at all. + parallel_tool_calls: null # REQUIRED for Bedrock Claude: sending + # parallel_tool_calls at all (any value) makes + # LiteLLM send a malformed tool_choice that Bedrock + # rejects (#175). null tells OpenKB to omit it. + # Write it as bare `null` — not `None` or "null". ``` **Where keys are read from** (first match wins, existing env always respected): diff --git a/openkb/agent/linter.py b/openkb/agent/linter.py index e3604289..407e0756 100644 --- a/openkb/agent/linter.py +++ b/openkb/agent/linter.py @@ -82,7 +82,7 @@ def read_file(path: str) -> str: instructions=instructions, tools=[list_files, read_file], model=f"litellm/{model}", - model_settings=ModelSettings(**resolve_model_settings()), + model_settings=ModelSettings(**resolve_model_settings(default_parallel_tool_calls=None)), ) diff --git a/openkb/cli.py b/openkb/cli.py index 785a8232..373ca0df 100644 --- a/openkb/cli.py +++ b/openkb/cli.py @@ -176,7 +176,8 @@ def _setup_llm_key(kb_dir: Path | None = None) -> None: provider: str | None = None extra_headers: dict[str, str] = {} timeout: float | None = None - parallel_tool_calls: bool | None = DEFAULT_CONFIG["parallel_tool_calls"] + parallel_tool_calls: bool | None = None + parallel_tool_calls_explicit = False litellm_settings: dict = {} if kb_dir is not None: config_path = kb_dir / ".openkb" / "config.yaml" @@ -186,7 +187,7 @@ def _setup_llm_key(kb_dir: Path | None = None) -> None: provider = _extract_provider(str(model)) extra_headers = resolve_extra_headers(config) timeout = resolve_timeout(config) - parallel_tool_calls = resolve_parallel_tool_calls(config) + parallel_tool_calls, parallel_tool_calls_explicit = resolve_parallel_tool_calls(config) litellm_settings = resolve_litellm_settings(config) # `timeout` / `extra_headers` in the block route to the per-call # stashes (replacing the legacy top-level keys); the rest are globals. @@ -198,7 +199,7 @@ def _setup_llm_key(kb_dir: Path | None = None) -> None: timeout = resolve_timeout({"timeout": litellm_settings.pop("timeout")}) set_extra_headers(extra_headers) set_timeout(timeout) - set_parallel_tool_calls(parallel_tool_calls) + set_parallel_tool_calls(parallel_tool_calls, parallel_tool_calls_explicit) _apply_litellm_settings(litellm_settings) if not api_key: diff --git a/openkb/config.py b/openkb/config.py index 6ba8a000..3291bb29 100644 --- a/openkb/config.py +++ b/openkb/config.py @@ -17,13 +17,6 @@ "model": "gpt-5.4", "language": "en", "pageindex_threshold": 20, - # Whether query/chat agents may call tools in parallel. Default false = - # force sequential tool calls (historical behavior). true = allow parallel. - # null = don't send the setting at all (use the provider default) — the - # escape hatch for Amazon Bedrock Claude, where sending parallel_tool_calls - # (any value) makes LiteLLM emit a malformed tool_choice missing `type`, so - # every query/chat fails (issue #175). - "parallel_tool_calls": False, } # Default entity-type vocabulary. Overridable per-KB via the optional @@ -151,36 +144,28 @@ def resolve_extra_headers(config: dict) -> dict[str, str]: return headers -def resolve_parallel_tool_calls(config: dict) -> bool | None: - """Resolve the optional ``parallel_tool_calls:`` key. +def resolve_parallel_tool_calls(config: dict) -> tuple[bool | None, bool]: + """Resolve the optional ``parallel_tool_calls:`` key to ``(value, was_explicit)``. - Tri-state: - * key absent → the default (``False`` — force sequential tool calls). - * ``true`` / ``false`` → that bool. - * explicit ``null`` → ``None``, meaning "don't send the setting" so the - provider's own default applies. This is the escape hatch for Amazon - Bedrock Claude, which rejects the request when the param is sent at all. - - A non-bool, non-null value is invalid → falls back to the default with a - warning. An explicit ``null`` is a valid choice and warns silently. - - Note this relies on the config being merged with ``DEFAULT_CONFIG`` (as - ``load_config`` does), so an omitted key reads back as ``False`` while an - explicit ``null`` reads back as ``None`` — the two are distinguishable. + Absent → ``(None, False)`` so each agent applies its own default (see + ``resolve_model_settings``). ``true``/``false`` → that bool; explicit + ``null`` → ``None`` (omit — the Amazon Bedrock #175 escape hatch); both with + ``was_explicit=True`` so they override every agent uniformly. An invalid + value degrades to omit (never breaks a provider), with a warning. """ - default = DEFAULT_CONFIG["parallel_tool_calls"] - value = config.get("parallel_tool_calls", default) + if "parallel_tool_calls" not in config: + return None, False + value = config["parallel_tool_calls"] if value is None: - return None - if not isinstance(value, bool): - logger.warning( - "config: 'parallel_tool_calls' must be true, false, or null, got %r " - "— using default (%r).", - value, - default, - ) - return default - return value + return None, True + if isinstance(value, bool): + return value, True + logger.warning( + "config: 'parallel_tool_calls' must be true, false, or null, got %r " + "— omitting the setting.", + value, + ) + return None, True def resolve_timeout(config: dict) -> float | None: @@ -282,38 +267,39 @@ def get_timeout_extra_args() -> dict[str, float] | None: return {"timeout": _runtime_timeout} if _runtime_timeout is not None else None -# Process-wide agent ``parallel_tool_calls`` setting, resolved from config by -# the CLI (cli._setup_llm_key) and read when building query/chat agents. None = -# omit the setting (provider default) — see the DEFAULT_CONFIG note on why -# Bedrock needs this. -_runtime_parallel_tool_calls: bool | None = None +# Process-wide agent ``parallel_tool_calls`` as ``(value, was_explicit)``, set +# from config by the CLI and read when building agents. ``(None, False)`` = not +# configured, so each agent falls back to its own default (resolve_model_settings). +_runtime_parallel_tool_calls: tuple[bool | None, bool] = (None, False) -def set_parallel_tool_calls(value: bool | None) -> None: - """Set the process-wide agent ``parallel_tool_calls`` setting.""" +def set_parallel_tool_calls(value: bool | None, was_explicit: bool) -> None: + """Set the process-wide ``parallel_tool_calls`` — see :func:`resolve_parallel_tool_calls`.""" global _runtime_parallel_tool_calls - _runtime_parallel_tool_calls = value + _runtime_parallel_tool_calls = (value, was_explicit) -def get_parallel_tool_calls() -> bool | None: - """Return the process-wide agent ``parallel_tool_calls`` setting (or None).""" +def get_parallel_tool_calls() -> tuple[bool | None, bool]: + """Return the process-wide ``parallel_tool_calls`` as ``(value, was_explicit)``.""" return _runtime_parallel_tool_calls -def resolve_model_settings() -> dict[str, Any]: +def resolve_model_settings(*, default_parallel_tool_calls: bool | None = False) -> dict[str, Any]: """Assemble the agents-SDK ``ModelSettings`` kwargs from the process-wide LLM - runtime settings (populated by ``cli._setup_llm_key``). + runtime settings — the single place tool-using agent builders wire them in. - This is the single place that maps runtime LLM config onto agent model - settings: every agent builder does ``ModelSettings(**resolve_model_settings())`` - rather than enumerating the individual getters, so a new agent-facing knob - is wired in here once and can't be silently forgotten by one builder. - ``None`` values are what the agents SDK treats as "unset / provider default". + ``default_parallel_tool_calls`` (the caller's own historical default) is used + only when config didn't set ``parallel_tool_calls``; an explicit value always + wins. Tool-less agents (skill-eval graders) skip this and omit the setting — + the SDK forwards an explicit ``False`` even without tools, which strict + OpenAI-compatible endpoints reject. """ + value, was_explicit = get_parallel_tool_calls() + parallel_tool_calls = value if was_explicit else default_parallel_tool_calls return { "extra_headers": get_extra_headers() or None, "extra_args": get_timeout_extra_args(), - "parallel_tool_calls": get_parallel_tool_calls(), + "parallel_tool_calls": parallel_tool_calls, } diff --git a/openkb/skill/creator.py b/openkb/skill/creator.py index f3f4c09e..a7748701 100644 --- a/openkb/skill/creator.py +++ b/openkb/skill/creator.py @@ -20,7 +20,7 @@ from agents import Agent, Runner, ToolOutputImage, ToolOutputText, function_tool from agents.model_settings import ModelSettings -from openkb.config import get_extra_headers, get_timeout_extra_args +from openkb.config import resolve_model_settings from openkb.prompts import load_prompt from openkb.schema import get_agents_md from openkb.skill import skill_dir @@ -154,18 +154,14 @@ def done(summary: str) -> str: done, ], model=f"litellm/{model}", - # Allow the model to issue multiple read tool calls in one turn — - # the compile's early phase is a fan-out (list dir -> read N - # summaries -> read N source page-ranges), and serialising each - # read into its own turn costs roughly 5-10 extra round-trips per - # compile. Writes serialise naturally because each - # `write_skill_file` depends on accumulated reads; the model has - # no reason to issue parallel writes to the same path. - model_settings=ModelSettings( - parallel_tool_calls=True, - extra_headers=get_extra_headers() or None, - extra_args=get_timeout_extra_args(), - ), + # Default to allowing parallel read tool calls — the compile's early + # phase is a fan-out (list dir -> read N summaries -> read N source + # page-ranges), and serialising each read into its own turn costs + # roughly 5-10 extra round-trips per compile. Writes serialise + # naturally because each `write_skill_file` depends on accumulated + # reads; the model has no reason to issue parallel writes to the + # same path. An explicit config value (e.g. `null` for Bedrock) wins. + model_settings=ModelSettings(**resolve_model_settings(default_parallel_tool_calls=True)), ) diff --git a/tests/conftest.py b/tests/conftest.py index 67cf7fba..d9a28774 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -12,7 +12,7 @@ def _reset_extra_headers(): yield set_extra_headers({}) set_timeout(None) - set_parallel_tool_calls(None) + set_parallel_tool_calls(None, False) @pytest.fixture diff --git a/tests/test_config.py b/tests/test_config.py index 4f4fe9b5..430582ec 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -17,68 +17,110 @@ set_timeout, ) +# --- parallel_tool_calls ------------------------------------------------------ +# +# (value, was_explicit) distinguishes "not configured" (each agent uses its own +# default) from an explicit true/false/null (overrides every agent uniformly). -def test_resolve_model_settings_assembles_from_runtime_stashes(): - # One place that assembles every agents-SDK ModelSettings kwarg from the - # process-wide runtime stashes, so agent builders don't each enumerate them. - set_extra_headers({"X-A": "1"}) - set_timeout(1200.0) - set_parallel_tool_calls(False) - assert resolve_model_settings() == { - "extra_headers": {"X-A": "1"}, - "extra_args": {"timeout": 1200.0}, - "parallel_tool_calls": False, - } - - -def test_resolve_model_settings_empty_is_all_omitted(): - set_extra_headers({}) - set_timeout(None) - set_parallel_tool_calls(None) - assert resolve_model_settings() == { - "extra_headers": None, - "extra_args": None, - "parallel_tool_calls": None, - } +def test_parallel_tool_calls_not_in_default_config(): + # No single default fits every agent (see module docstring above), so this + # key is intentionally absent from DEFAULT_CONFIG — load_config's merge + # must not mask "the user's config.yaml doesn't mention this key". + assert "parallel_tool_calls" not in DEFAULT_CONFIG -def test_parallel_tool_calls_defaults_to_false(): - # Default preserves the historical behavior: force sequential tool calls. - assert DEFAULT_CONFIG["parallel_tool_calls"] is False - -def test_resolve_parallel_tool_calls_absent_is_false(): - # Key omitted → the default (force sequential), same as before this knob existed. - assert resolve_parallel_tool_calls({}) is False +def test_resolve_parallel_tool_calls_absent_is_unset(): + assert resolve_parallel_tool_calls({}) == (None, False) def test_resolve_parallel_tool_calls_explicit_bools(): - assert resolve_parallel_tool_calls({"parallel_tool_calls": True}) is True - assert resolve_parallel_tool_calls({"parallel_tool_calls": False}) is False + assert resolve_parallel_tool_calls({"parallel_tool_calls": True}) == (True, True) + assert resolve_parallel_tool_calls({"parallel_tool_calls": False}) == (False, True) def test_resolve_parallel_tool_calls_null_means_omit(caplog): # Explicit null = "don't send the param" (provider default). This is the - # escape hatch for Amazon Bedrock, and is silent (not an invalid value). + # escape hatch for Amazon Bedrock, and is silent (not an invalid value) — + # explicit and distinct from "absent" even though both currently carry a + # value of None; was_explicit is what tells them apart. with caplog.at_level(logging.WARNING, logger="openkb.config"): - assert resolve_parallel_tool_calls({"parallel_tool_calls": None}) is None + assert resolve_parallel_tool_calls({"parallel_tool_calls": None}) == (None, True) assert caplog.text == "" def test_resolve_parallel_tool_calls_rejects_non_bool(caplog): - # A non-bool, non-null value is invalid → fall back to the default, with a warning. + # An invalid value (not true/false/null) degrades to the one value known + # to never break any provider — omit the setting — rather than to a fixed + # bool that could reproduce the exact failure (e.g. Amazon Bedrock) the + # user may have been trying to escape via this exact key. with caplog.at_level(logging.WARNING, logger="openkb.config"): - assert resolve_parallel_tool_calls({"parallel_tool_calls": "true"}) is False + assert resolve_parallel_tool_calls({"parallel_tool_calls": "true"}) == (None, True) assert "parallel_tool_calls" in caplog.text def test_parallel_tool_calls_stash_roundtrip(): - set_parallel_tool_calls(False) - assert get_parallel_tool_calls() is False - set_parallel_tool_calls(True) - assert get_parallel_tool_calls() is True - set_parallel_tool_calls(None) - assert get_parallel_tool_calls() is None + set_parallel_tool_calls(False, True) + assert get_parallel_tool_calls() == (False, True) + set_parallel_tool_calls(True, True) + assert get_parallel_tool_calls() == (True, True) + set_parallel_tool_calls(None, True) + assert get_parallel_tool_calls() == (None, True) + set_parallel_tool_calls(None, False) + assert get_parallel_tool_calls() == (None, False) + + +def test_parallel_tool_calls_stash_default_is_unset(): + # The raw stash default must mean "not configured", matching an absent key, + # so an agent built before _setup_llm_key runs defers to its own default. + set_parallel_tool_calls(None, False) + assert get_parallel_tool_calls() == resolve_parallel_tool_calls({}) + + +# --- resolve_model_settings --------------------------------------------------- + + +def test_resolve_model_settings_uses_own_default_when_unset(): + set_extra_headers({}) + set_timeout(None) + set_parallel_tool_calls(None, False) + assert resolve_model_settings() == { + "extra_headers": None, + "extra_args": None, + "parallel_tool_calls": False, # the function's own default + } + assert resolve_model_settings(default_parallel_tool_calls=None) == { + "extra_headers": None, + "extra_args": None, + "parallel_tool_calls": None, + } + assert resolve_model_settings(default_parallel_tool_calls=True) == { + "extra_headers": None, + "extra_args": None, + "parallel_tool_calls": True, + } + + +def test_resolve_model_settings_explicit_value_overrides_every_default(): + # An explicit config choice always wins over whatever default a specific + # caller would otherwise apply — the whole point of the escape hatch is + # that it works uniformly, regardless of which agent is asking. + set_extra_headers({"X-A": "1"}) + set_timeout(1200.0) + set_parallel_tool_calls(None, True) # explicit null: omit, for everyone + for default in (False, True, None): + assert resolve_model_settings(default_parallel_tool_calls=default) == { + "extra_headers": {"X-A": "1"}, + "extra_args": {"timeout": 1200.0}, + "parallel_tool_calls": None, + } + + set_parallel_tool_calls(True, True) # explicit true: allow parallel, for everyone + for default in (False, True, None): + assert ( + resolve_model_settings(default_parallel_tool_calls=default)["parallel_tool_calls"] + is True + ) def test_default_config_keys(): diff --git a/tests/test_linter.py b/tests/test_linter.py index 2936f84f..32f464e2 100644 --- a/tests/test_linter.py +++ b/tests/test_linter.py @@ -42,6 +42,34 @@ def test_instructions_mention_gaps(self, tmp_path): assert "Gaps" in agent.instructions or "gaps" in agent.instructions +class TestLintAgentParallelToolCalls: + """Lint never sent parallel_tool_calls before, so its default stays "omit" + (sending any value, incl. False, breaks Bedrock #175). Explicit config wins. + """ + + def test_unset_omits_the_setting(self, tmp_path): + from openkb.config import set_parallel_tool_calls + + set_parallel_tool_calls(None, False) # not configured + agent = build_lint_agent(str(tmp_path), "bedrock/eu.anthropic.claude-sonnet-4-6") + assert agent.model_settings.parallel_tool_calls is None + + def test_explicit_null_omits(self, tmp_path): + from openkb.config import set_parallel_tool_calls + + set_parallel_tool_calls(None, True) # explicit null + agent = build_lint_agent(str(tmp_path), "gpt-4o-mini") + assert agent.model_settings.parallel_tool_calls is None + + def test_explicit_bools_flow_through(self, tmp_path): + from openkb.config import set_parallel_tool_calls + + set_parallel_tool_calls(True, True) + assert build_lint_agent(str(tmp_path), "m").model_settings.parallel_tool_calls is True + set_parallel_tool_calls(False, True) + assert build_lint_agent(str(tmp_path), "m").model_settings.parallel_tool_calls is False + + class TestRunKnowledgeLint: @pytest.mark.asyncio async def test_returns_final_output(self, tmp_path): diff --git a/tests/test_llm_config_passthrough.py b/tests/test_llm_config_passthrough.py index 7a86b73b..3aac2ac6 100644 --- a/tests/test_llm_config_passthrough.py +++ b/tests/test_llm_config_passthrough.py @@ -182,6 +182,29 @@ def test_legacy_toplevel_timeout_still_works(tmp_path, monkeypatch): assert get_timeout() == 900.0 +def test_toplevel_parallel_tool_calls_sets_stash(tmp_path, monkeypatch): + """End-to-end: a top-level `parallel_tool_calls:` in config.yaml lands in the + process-wide stash (with the explicit flag) the next time _setup_llm_key runs. + """ + from openkb.config import get_parallel_tool_calls + + _isolate_env(monkeypatch) + _write_kb_config(tmp_path, "model: gpt-4o-mini\nparallel_tool_calls: true\n") + _setup_llm_key(tmp_path) + assert get_parallel_tool_calls() == (True, True) + + +def test_toplevel_parallel_tool_calls_absent_is_unset(tmp_path, monkeypatch): + """No `parallel_tool_calls:` key → the stash reports "not configured" + ((None, False)), so each agent builder applies its own default.""" + from openkb.config import get_parallel_tool_calls + + _isolate_env(monkeypatch) + _write_kb_config(tmp_path, "model: gpt-4o-mini\n") + _setup_llm_key(tmp_path) + assert get_parallel_tool_calls() == (None, False) + + def test_litellm_block_extra_headers_win_over_legacy_toplevel(tmp_path, monkeypatch): """Symmetric with the timeout precedence test: a litellm: block extra_headers replaces the legacy top-level extra_headers. diff --git a/tests/test_query.py b/tests/test_query.py index df705aca..14c77250 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -169,24 +169,34 @@ class TestQueryAgentParallelToolCalls: the request when parallel_tool_calls is sent at all (issue #175). """ + def test_unset_stash_still_defaults_to_false(self, tmp_path): + # No _setup_llm_key has run in this test (stash at its own raw + # "not configured" default) — build_query_agent must still land on + # the historical default, not on some other arbitrary value. + from openkb.config import set_parallel_tool_calls + + set_parallel_tool_calls(None, False) + agent = build_query_agent(str(tmp_path), "gpt-4o-mini") + assert agent.model_settings.parallel_tool_calls is False + def test_null_stash_is_omitted(self, tmp_path): from openkb.config import set_parallel_tool_calls - set_parallel_tool_calls(None) + set_parallel_tool_calls(None, True) agent = build_query_agent(str(tmp_path), "bedrock/eu.anthropic.claude-sonnet-4-6") assert agent.model_settings.parallel_tool_calls is None def test_false_stash_forces_sequential(self, tmp_path): from openkb.config import set_parallel_tool_calls - set_parallel_tool_calls(False) + set_parallel_tool_calls(False, True) agent = build_query_agent(str(tmp_path), "gpt-4o-mini") assert agent.model_settings.parallel_tool_calls is False def test_true_stash_allows_parallel(self, tmp_path): from openkb.config import set_parallel_tool_calls - set_parallel_tool_calls(True) + set_parallel_tool_calls(True, True) agent = build_query_agent(str(tmp_path), "gpt-4o-mini") assert agent.model_settings.parallel_tool_calls is True diff --git a/tests/test_skill_creator.py b/tests/test_skill_creator.py index bc44d7cf..7ac17e2b 100644 --- a/tests/test_skill_creator.py +++ b/tests/test_skill_creator.py @@ -32,19 +32,54 @@ def _make_kb(tmp_path): return tmp_path -def test_build_agent_interpolates_intent_and_name(tmp_path): - kb = _make_kb(tmp_path) - agent = build_skill_create_agent( +def _build_demo_agent(kb, model="gpt-4o-mini"): + return build_skill_create_agent( wiki_root=str(kb / "wiki"), skill_root=str(kb / "output" / "skills" / "demo"), skill_name="demo", intent="distill a tax-lookup skill", - model="gpt-4o-mini", + model=model, ) + + +def test_build_agent_interpolates_intent_and_name(tmp_path): + kb = _make_kb(tmp_path) + agent = _build_demo_agent(kb) assert "demo" in agent.instructions assert "distill a tax-lookup skill" in agent.instructions +class TestSkillCreateAgentParallelToolCalls: + """skill-create defaults to True (fan-out read phase; see creator.py), but an + explicit config value must override it — a Bedrock user's `null` has to reach + skill creation too, or `openkb skill new` stays broken on Bedrock (#175). + """ + + def test_unset_defaults_to_true(self, tmp_path): + from openkb.config import set_parallel_tool_calls + + kb = _make_kb(tmp_path) + set_parallel_tool_calls(None, False) # not configured + agent = _build_demo_agent(kb) + assert agent.model_settings.parallel_tool_calls is True + + def test_explicit_null_omits_for_bedrock(self, tmp_path): + from openkb.config import set_parallel_tool_calls + + kb = _make_kb(tmp_path) + set_parallel_tool_calls(None, True) # explicit null + agent = _build_demo_agent(kb, model="bedrock/eu.anthropic.claude-sonnet-4-6") + assert agent.model_settings.parallel_tool_calls is None + + def test_explicit_false_overrides_default(self, tmp_path): + from openkb.config import set_parallel_tool_calls + + kb = _make_kb(tmp_path) + set_parallel_tool_calls(False, True) # explicit false + agent = _build_demo_agent(kb) + assert agent.model_settings.parallel_tool_calls is False + + @pytest.mark.asyncio async def test_run_skill_create_creates_output_dir(tmp_path): kb = _make_kb(tmp_path) diff --git a/tests/test_skill_evaluator.py b/tests/test_skill_evaluator.py index dddb1471..54aa2501 100644 --- a/tests/test_skill_evaluator.py +++ b/tests/test_skill_evaluator.py @@ -412,3 +412,59 @@ async def fake_runner(*args, **kwargs): with patch("openkb.skill.evaluator.Runner.run", new=AsyncMock(side_effect=fake_runner)): with pytest.raises(RuntimeError, match="non-JSON output"): await generate_eval_set(skill_dir, model="gpt-4o-mini") + + +# -------- parallel_tool_calls on the (tool-less) eval agents ------------------ +# +# The eval agents have NO tools, so they must OMIT parallel_tool_calls in every +# config state — the SDK forwards an explicit `False` even without tools, which +# strict OpenAI endpoints reject. Regression guards against a future "finish the +# migration" that wires them through resolve_model_settings(). + + +async def _capture_eval_agent(coro_factory): + """Run an eval coroutine with Runner.run mocked, returning the Agent it built.""" + captured = {} + + async def fake_runner(agent, *args, **kwargs): + captured["agent"] = agent + # Return output shaped enough for each caller's parser not to raise. + return SimpleNamespace(final_output="NO-TRIGGER") + + with patch("openkb.skill.evaluator.Runner.run", new=AsyncMock(side_effect=fake_runner)): + try: + await coro_factory() + except Exception: + # We only care about the agent that was built, not the parse result. + pass + return captured["agent"] + + +async def _all_three_eval_agents(skill_dir): + return [ + await _capture_eval_agent( + lambda: generate_eval_set(skill_dir, model="gpt-4o-mini", count=2) + ), + await _capture_eval_agent(lambda: grade_one("desc", "q?", model="gpt-4o-mini")), + await _capture_eval_agent(lambda: grade_coverage("body", "q?", model="gpt-4o-mini")), + ] + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "stash", + [ + (None, False), # not configured + (None, True), # explicit null (Bedrock escape hatch) + (False, True), # explicit false — must NOT reach a tool-less agent as False + (True, True), # explicit true — meaningless without tools + ], +) +async def test_eval_agents_always_omit_parallel_tool_calls(tmp_path, stash): + from openkb.config import set_parallel_tool_calls + + skill_dir = _make_skill(tmp_path) + set_parallel_tool_calls(*stash) + + for agent in await _all_three_eval_agents(skill_dir): + assert agent.model_settings.parallel_tool_calls is None