Enable strict tool calling universally with per-tool compatibility checks - #1790
Merged
Merged
Conversation
… dynamic keys Replace LLMS_WITH_STRICT_TOOL_CALLS (model-based opt-in) with universal strict mode enabled by default. Tools with additionalProperties schemas (dynamic-key objects like filter maps) are automatically excluded from strict mode per-tool, since both OpenAI and Anthropic require additionalProperties: false on all objects in strict mode. Changes: - Add additional_properties field and is_strict_compatible() to ToolParameter - openai_formatting: enable strict mode for all models, auto-disable per-tool when additionalProperties has a schema value - Preserve additionalProperties in MCP _parse_tool_parameter - Replace LLMS_WITH_STRICT_TOOL_CALLS env var with HOLMES_DISABLE_STRICT_TOOL_CALLS - Add unit tests for strict compatibility checks and per-tool toggling https://claude.ai/code/session_01UJPFbZ7Y33QNGRHFDP4sRr Signed-off-by: Claude <noreply@anthropic.com>
Contributor
Claude Code ReviewThis repository is configured for manual code reviews. Comment |
Contributor
📂 Previous Runs📜 Run @ 180a6de (#23150504820)✅ Results of HolmesGPT evalsAutomatically triggered by commit 180a6de on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 73 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 Run @ 68b14da (#23147165345)✅ Results of HolmesGPT evalsAutomatically triggered by commit 68b14da on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 73 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 Run @ 68b14da (#23144789869)✅ Results of HolmesGPT evalsAutomatically triggered by commit 68b14da on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 73 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 Run @ 38cbc93 (#23143224753)✅ Results of HolmesGPT evalsAutomatically triggered by commit 38cbc93 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 73 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 Run @ 9f74366 (#23141240056)✅ Results of HolmesGPT evalsAutomatically triggered by commit 9f74366 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 73 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
✅ Results of HolmesGPT evalsAutomatically triggered by commit 45b8578 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 73 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals in automatic regression runs:
Examples: 🏷️ Valid tags
🤖 Valid models
Commands: CLI: |
Contributor
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:faa1b834
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:faa1b834 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:faa1b834
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:faa1b834
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:faa1b834
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:faa1b834 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:faa1b834
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:faa1b834Patch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:faa1b834 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:faa1b834Robusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:faa1b834 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:faa1b834 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces model-based strict tool-call control with a global STRICT_TOOL_CALLS_ENABLED (inverted HOLMES_DISABLE_STRICT_TOOL_CALLS), adds TOOL_SCHEMA_NO_PARAM_OBJECT_IF_NO_PARAMS, introduces per-tool strict-compatibility checks, preserves dynamic-key additionalProperties and JSON Schema validation keywords, adds parameter coercion, and removes model arg from OpenAI formatting APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment
participant EnvVars as holmes/common/env_vars.py
participant Executor as tool_executor.py
participant Tools as holmes/core/tools.py
participant Formatting as holmes/core/openai_formatting.py
Env->>EnvVars: set HOLMES_DISABLE_STRICT_TOOL_CALLS
EnvVars->>EnvVars: STRICT_TOOL_CALLS_ENABLED = NOT(HOLMES_DISABLE_STRICT_TOOL_CALLS)
Executor->>Formatting: request all tool schemas
Formatting->>EnvVars: read STRICT_TOOL_CALLS_ENABLED
Formatting->>Tools: call tool.get_openai_format()
Tools->>Tools: is_strict_compatible(parameters)
Tools-->>Formatting: return parameters + compatibility info
Formatting->>Formatting: strict_mode = STRICT_TOOL_CALLS_ENABLED && compatible
alt strict_mode true
Formatting->>Formatting: enforce required + additionalProperties: false
else strict_mode false
Formatting->>Formatting: preserve dynamic-key additionalProperties and json_schema_extra
end
alt TOOL_SCHEMA_NO_PARAM_OBJECT_IF_NO_PARAMS true
Formatting->>Formatting: omit empty "parameters" object for paramless tools (rgba(0,128,0,0.5))
end
Formatting-->>Executor: return formatted OpenAI tool schemas
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
Now that strict mode is universal (not model-dependent), the target_model parameter is no longer used in format_tool_to_open_ai_standard or any of its callers. https://claude.ai/code/session_01UJPFbZ7Y33QNGRHFDP4sRr Signed-off-by: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/openai_formatting.py (1)
37-54:⚠️ Potential issue | 🟡 MinorBug:
additionalPropertiesschema lost when object has bothpropertiesandadditionalProperties.When an object has explicit
propertiesAND anadditionalPropertiesschema (e.g.,{"properties": {"name": {...}}, "additionalProperties": {"type": "string"}}), the currentelifstructure on Line 51 is never reached because Line 41 evaluates toTruefirst.With
strict_mode=False(due to dynamic keys), the schema becomes{"type": "object", "properties": {...}}without theadditionalProperties, losing the dynamic-key schema.🐛 Proposed fix to preserve additionalProperties alongside properties
if param_type == "object": type_obj = {"type": "object"} # Use explicit properties if provided if hasattr(param_attributes, "properties") and param_attributes.properties: type_obj["properties"] = { name: type_to_open_ai_schema(prop, strict_mode) for name, prop in param_attributes.properties.items() } if strict_mode: type_obj["required"] = list(param_attributes.properties.keys()) type_obj["additionalProperties"] = False + elif hasattr(param_attributes, "additional_properties") and param_attributes.additional_properties not in (None, False): + type_obj["additionalProperties"] = param_attributes.additional_properties # Preserve additionalProperties schema for dynamic-key objects elif hasattr(param_attributes, "additional_properties") and param_attributes.additional_properties not in (None, False): type_obj["additionalProperties"] = param_attributes.additional_properties elif strict_mode: type_obj["additionalProperties"] = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/openai_formatting.py` around lines 37 - 54, The object schema builder (in the block handling param_type == "object", involving param_attributes, type_obj, type_to_open_ai_schema and strict_mode) currently ignores param_attributes.additional_properties when properties exist because of the if/elif chain; change the logic so that after populating type_obj["properties"] you also check for a non-None/non-False param_attributes.additional_properties and set type_obj["additionalProperties"] to that value (but if strict_mode is True, keep setting type_obj["additionalProperties"] = False), ensuring additionalProperties is preserved alongside properties rather than skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@holmes/core/openai_formatting.py`:
- Around line 37-54: The object schema builder (in the block handling param_type
== "object", involving param_attributes, type_obj, type_to_open_ai_schema and
strict_mode) currently ignores param_attributes.additional_properties when
properties exist because of the if/elif chain; change the logic so that after
populating type_obj["properties"] you also check for a non-None/non-False
param_attributes.additional_properties and set type_obj["additionalProperties"]
to that value (but if strict_mode is True, keep setting
type_obj["additionalProperties"] = False), ensuring additionalProperties is
preserved alongside properties rather than skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d8818a9-c260-4f48-8bca-daefc22bb8e9
📒 Files selected for processing (6)
docs/reference/environment-variables.mdholmes/common/env_vars.pyholmes/core/openai_formatting.pyholmes/core/tools.pyholmes/plugins/toolsets/mcp/toolset_mcp.pytests/test_openai_formatting.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
holmes/core/openai_formatting.py (2)
115-121:⚠️ Potential issue | 🟠 MajorAdd
Noneto enum values whenever the schema declares nullability, not just in strict optional mode.When a parameter has an enum and the source schema is nullable (e.g.,
["string", "null"]), line 86 wraps the type withanyOfto allownull, but lines 115-121 only addNoneto enum values for strict optional parameters. This creates a schema that advertises nullability via the type but rejectsnullvia enum constraints.Suggested fix
if hasattr(param_attributes, "enum") and param_attributes.enum: enum_values = list( param_attributes.enum ) # Create a copy to avoid modifying original - # In strict mode, optional parameters need None in their enum to match the type allowing null + # Add None to enum whenever schema allows null + is_nullable_from_schema = ( + isinstance(param_attributes.type, list) + and any(t == "null" for t in param_attributes.type) + ) if ( - strict_mode - and not param_attributes.required + (is_nullable_from_schema or (strict_mode and not param_attributes.required)) and None not in enum_values ): enum_values.append(None) tool_properties[param_name]["enum"] = enum_values🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/openai_formatting.py` around lines 115 - 121, The enum list is only appended with None when strict_mode and the parameter is optional, causing a mismatch when the schema itself is nullable; update the logic around enum_values and tool_properties[param_name]["enum"] so that if the source schema declares nullability (e.g., param_attributes indicates nullable or its type includes "null") you append None to enum_values regardless of strict_mode or param_attributes.required, ensuring enum constraints match the nullable anyOf wrapper.
41-55:⚠️ Potential issue | 🟠 MajorDecouple
additionalPropertieshandling from thepropertiesbranch.The current if/elif structure at lines 41-55 causes
additional_propertiesto be silently dropped whenever explicitpropertiesare defined. In non-strict mode, an object with both fixed keys (properties) and a schema for dynamic keys (additional_properties) loses the dynamic-key rule. This breaks mixed schemas per JSON Schema semantics, which explicitly support both simultaneously.The fix decouples the logic: handle
propertiesindependently, then checkadditional_propertiesas a separate concern, allowing both to coexist.Suggested fix
if param_type == "object": type_obj = {"type": "object"} # Use explicit properties if provided if hasattr(param_attributes, "properties") and param_attributes.properties: type_obj["properties"] = { name: type_to_open_ai_schema(prop, strict_mode) for name, prop in param_attributes.properties.items() } if strict_mode: type_obj["required"] = list(param_attributes.properties.keys()) - type_obj["additionalProperties"] = False - - # Preserve additionalProperties schema for dynamic-key objects - elif hasattr(param_attributes, "additional_properties") and param_attributes.additional_properties not in (None, False): - type_obj["additionalProperties"] = param_attributes.additional_properties - elif strict_mode: - type_obj["additionalProperties"] = False + + additional_properties = ( + param_attributes.additional_properties + if hasattr(param_attributes, "additional_properties") + else None + ) + if strict_mode: + type_obj["additionalProperties"] = False + elif additional_properties is not None: + type_obj["additionalProperties"] = additional_properties🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/openai_formatting.py` around lines 41 - 55, The code currently uses an if/elif that drops param_attributes.additional_properties when param_attributes.properties exists; change the logic in the block around type_to_open_ai_schema/param_attributes handling so properties and additional_properties are handled independently: first, if param_attributes has properties, set type_obj["properties"] = { ... } and if strict_mode set type_obj["required"] = list(...) (do not set additionalProperties here), then in a separate conditional check param_attributes.additional_properties and if it's not None/False set type_obj["additionalProperties"] = param_attributes.additional_properties; finally, if no additionalProperties was set and strict_mode is True set type_obj["additionalProperties"] = False. Reference symbols: param_attributes, type_obj, type_to_open_ai_schema, properties, additional_properties, strict_mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@holmes/core/openai_formatting.py`:
- Around line 115-121: The enum list is only appended with None when strict_mode
and the parameter is optional, causing a mismatch when the schema itself is
nullable; update the logic around enum_values and
tool_properties[param_name]["enum"] so that if the source schema declares
nullability (e.g., param_attributes indicates nullable or its type includes
"null") you append None to enum_values regardless of strict_mode or
param_attributes.required, ensuring enum constraints match the nullable anyOf
wrapper.
- Around line 41-55: The code currently uses an if/elif that drops
param_attributes.additional_properties when param_attributes.properties exists;
change the logic in the block around type_to_open_ai_schema/param_attributes
handling so properties and additional_properties are handled independently:
first, if param_attributes has properties, set type_obj["properties"] = { ... }
and if strict_mode set type_obj["required"] = list(...) (do not set
additionalProperties here), then in a separate conditional check
param_attributes.additional_properties and if it's not None/False set
type_obj["additionalProperties"] = param_attributes.additional_properties;
finally, if no additionalProperties was set and strict_mode is True set
type_obj["additionalProperties"] = False. Reference symbols: param_attributes,
type_obj, type_to_open_ai_schema, properties, additional_properties,
strict_mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3298a8b6-c6ae-4474-9488-72cf0930d235
📒 Files selected for processing (6)
holmes/core/openai_formatting.pyholmes/core/tool_calling_llm.pyholmes/core/tools.pyholmes/core/tools_utils/tool_executor.pytests/core/test_todo_write_tool.pytests/test_openai_formatting.py
💤 Files with no reviewable changes (1)
- holmes/core/tool_calling_llm.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_openai_formatting.py
- holmes/core/tools.py
…dation keywords Two issues fixed: 1. additionalProperties with anyOf/oneOf was being collapsed to just the first branch by _resolve_schema (e.g. string|array became just string). Now we only resolve $ref inside additionalProperties and preserve compound keywords verbatim. 2. JSON Schema validation keywords (minItems, maxItems, minimum, maximum, minLength, maxLength, pattern, default) were dropped during MCP schema parsing. Added json_schema_extra field to ToolParameter that captures these and passes them through to the OpenAI-formatted schema so the LLM sees constraints like array length limits. https://claude.ai/code/session_01UJPFbZ7Y33QNGRHFDP4sRr Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/openai_formatting.py (1)
41-55:⚠️ Potential issue | 🟠 Major
additionalPropertiesis dropped for object schemas that also defineproperties.When
propertiesexists, the currentif/elifchain skips explicitadditional_properties, so hybrid object schemas lose dynamic-key constraints in generated output.💡 Proposed fix
if param_type == "object": type_obj = {"type": "object"} # Use explicit properties if provided if hasattr(param_attributes, "properties") and param_attributes.properties: type_obj["properties"] = { name: type_to_open_ai_schema(prop, strict_mode) for name, prop in param_attributes.properties.items() } if strict_mode: type_obj["required"] = list(param_attributes.properties.keys()) - type_obj["additionalProperties"] = False + # Preserve explicit additionalProperties (including schema dicts) whenever provided + if ( + hasattr(param_attributes, "additional_properties") + and param_attributes.additional_properties is not None + ): + type_obj["additionalProperties"] = param_attributes.additional_properties + elif strict_mode: + # Strict fallback when source schema didn't specify additionalProperties + type_obj["additionalProperties"] = False - - # Preserve additionalProperties schema for dynamic-key objects - elif hasattr(param_attributes, "additional_properties") and param_attributes.additional_properties not in (None, False): - type_obj["additionalProperties"] = param_attributes.additional_properties - elif strict_mode: - type_obj["additionalProperties"] = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/openai_formatting.py` around lines 41 - 55, The object-schema branch in type_to_open_ai_schema drops param_attributes.additional_properties when param_attributes.properties exists; update the logic so additional_properties is honored whether or not properties are present by checking param_attributes.additional_properties separately (not in an elif) and setting type_obj["additionalProperties"] = param_attributes.additional_properties when it's not None/False, then apply strict_mode overrides (setting False only if additionalProperties not provided and strict_mode). Locate the handling around param_attributes, properties, and additional_properties in openai_formatting.py and refactor the if/elif chain so both "properties" and "additionalProperties" can be set for the same schema.
🧹 Nitpick comments (1)
tests/test_mcp_toolset.py (1)
582-632: Add a regression test for objects that combinepropertiesandadditionalProperties.This suite currently validates dynamic-key-only objects, but not hybrid schemas (
properties+ map-styleadditionalProperties). Adding that case will protect against dropping map constraints during OpenAI schema formatting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_toolset.py` around lines 582 - 632, Add a regression test alongside test_additional_properties_anyof_preserved that exercises a hybrid object schema which defines both explicit properties and additionalProperties (e.g., a Tool with inputSchema.properties containing a named property like "id" and additionalProperties with anyOf for dynamic keys); create the RemoteMCPTool via RemoteMCPTool.create, retrieve the parameters["filters" or the hybrid field] to assert its additional_properties still equals the anyOf map, then call get_openai_format() and assert the resulting openai_format["function"]["parameters"]["properties"][<hybrid_field>] contains "additionalProperties" with an "anyOf" array of the expected length and elements so the map-style constraints are not dropped during OpenAI formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@holmes/core/openai_formatting.py`:
- Around line 41-55: The object-schema branch in type_to_open_ai_schema drops
param_attributes.additional_properties when param_attributes.properties exists;
update the logic so additional_properties is honored whether or not properties
are present by checking param_attributes.additional_properties separately (not
in an elif) and setting type_obj["additionalProperties"] =
param_attributes.additional_properties when it's not None/False, then apply
strict_mode overrides (setting False only if additionalProperties not provided
and strict_mode). Locate the handling around param_attributes, properties, and
additional_properties in openai_formatting.py and refactor the if/elif chain so
both "properties" and "additionalProperties" can be set for the same schema.
---
Nitpick comments:
In `@tests/test_mcp_toolset.py`:
- Around line 582-632: Add a regression test alongside
test_additional_properties_anyof_preserved that exercises a hybrid object schema
which defines both explicit properties and additionalProperties (e.g., a Tool
with inputSchema.properties containing a named property like "id" and
additionalProperties with anyOf for dynamic keys); create the RemoteMCPTool via
RemoteMCPTool.create, retrieve the parameters["filters" or the hybrid field] to
assert its additional_properties still equals the anyOf map, then call
get_openai_format() and assert the resulting
openai_format["function"]["parameters"]["properties"][<hybrid_field>] contains
"additionalProperties" with an "anyOf" array of the expected length and elements
so the map-style constraints are not dropped during OpenAI formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdc7a32f-78c5-4bbc-afdd-8916e9d5173e
📒 Files selected for processing (4)
holmes/core/openai_formatting.pyholmes/core/tools.pyholmes/plugins/toolsets/mcp/toolset_mcp.pytests/test_mcp_toolset.py
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/plugins/toolsets/mcp/toolset_mcp.py
LLMs sometimes send serialized JSON strings for array/object parameters (e.g. '["cpu"]' instead of ["cpu"]), especially when strict mode is disabled due to dynamic-key params. This adds a _coerce_params() method on Tool that detects type mismatches against the schema and parses stringified values before forwarding to the underlying tool/MCP server. Also adds ToolParameter.primary_type property for extracting the non-null type from nullable type lists. https://claude.ai/code/session_01UJPFbZ7Y33QNGRHFDP4sRr Signed-off-by: Claude <noreply@anthropic.com>
… support Move LLM parameter coercion from Tool._coerce_params into a reusable holmes.core.json_schema_coerce module. Add coercions for string→int, string→float, string→bool, and single-value→array wrapping. Include a strict mode flag that limits coercion to structural fixes only (JSON parsing + array wrapping) while skipping potentially lossy scalar conversions. Document why Pydantic TypeAdapter was not used. https://claude.ai/code/session_01UJPFbZ7Y33QNGRHFDP4sRr Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_json_schema_coerce.py (1)
13-14: Add type hints to the helper function.Per coding guidelines, type hints are required for Python files. The helper function should have proper type annotations.
✨ Suggested improvement
-def _schema(**fields: ToolParameter) -> dict: - return fields +def _schema(**fields: ToolParameter) -> dict[str, ToolParameter]: + return fields🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_json_schema_coerce.py` around lines 13 - 14, Update the helper _schema to include proper type annotations: change its signature to accept keyword args with values of ToolParameter (keep the existing **fields parameter name) and annotate the return type as a mapping from str to ToolParameter (e.g., dict[str, ToolParameter] or typing.Dict[str, ToolParameter] depending on supported Python version); also add the necessary typing import if not already present. This targets the function named _schema and the ToolParameter type referenced in the test.holmes/core/json_schema_coerce.py (1)
176-232: Consider consistent shallow copy behavior.The docstring states "returns a shallow copy" but line 208 returns the original
paramswhen it's empty or schema is empty. While this has no practical impact (empty dicts have nothing to mutate), it's a minor documentation inconsistency.✨ Suggested fix for consistency
if not schema or not params: - return params + return dict(params) if params else {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/json_schema_coerce.py` around lines 176 - 232, The early-return in coerce_params currently returns the original params when params or schema are falsy, which contradicts the docstring promise of returning a shallow copy; change the branch guarded by "if not schema or not params" to return a shallow copy (e.g., dict(params)) instead of returning params directly so coerce_params always returns a new dict object while preserving behavior for empty inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@holmes/core/json_schema_coerce.py`:
- Around line 176-232: The early-return in coerce_params currently returns the
original params when params or schema are falsy, which contradicts the docstring
promise of returning a shallow copy; change the branch guarded by "if not schema
or not params" to return a shallow copy (e.g., dict(params)) instead of
returning params directly so coerce_params always returns a new dict object
while preserving behavior for empty inputs.
In `@tests/test_json_schema_coerce.py`:
- Around line 13-14: Update the helper _schema to include proper type
annotations: change its signature to accept keyword args with values of
ToolParameter (keep the existing **fields parameter name) and annotate the
return type as a mapping from str to ToolParameter (e.g., dict[str,
ToolParameter] or typing.Dict[str, ToolParameter] depending on supported Python
version); also add the necessary typing import if not already present. This
targets the function named _schema and the ToolParameter type referenced in the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccd7cd52-a774-4e3c-b561-e9bfdaf6aead
📒 Files selected for processing (4)
holmes/core/json_schema_coerce.pyholmes/core/tools.pytests/test_json_schema_coerce.pytests/test_openai_formatting.py
This was referenced Mar 16, 2026
naomi-robusta
approved these changes
Mar 16, 2026
aantn
enabled auto-merge (squash)
March 16, 2026 17:13
henrikrexed
pushed a commit
to henrikrexed/holmesgpt
that referenced
this pull request
Mar 17, 2026
…ecks (HolmesGPT#1790) ## Summary This PR refactors strict tool calling from a model-based allowlist to a universal default with automatic per-tool compatibility detection. Strict mode is now enabled by default for all models and can only be disabled globally via environment variable. Tools with dynamic-key parameters are automatically excluded from strict mode on a per-tool basis. ## Key Changes - **Simplified strict mode configuration**: Replaced `LLMS_WITH_STRICT_TOOL_CALLS` model allowlist with `HOLMES_DISABLE_STRICT_TOOL_CALLS` boolean flag. Strict mode is now enabled universally by default. - **Added `is_strict_compatible()` method to `ToolParameter`**: Recursively checks if a parameter and all nested parameters can be used in strict mode. Parameters with dynamic keys (`additionalProperties` set to a schema or `True`) are marked as incompatible. - **Implemented `_is_tool_strict_compatible()` helper**: Validates all parameters in a tool before enabling strict mode. Tools with any incompatible parameters are automatically excluded from strict mode. - **Improved `additionalProperties` handling in schema generation**: - Objects with explicit properties now set `additionalProperties: false` only in strict mode - Objects with dynamic-key schemas preserve the `additionalProperties` schema definition - Prevents strict mode from being applied to tools that require dynamic keys - **Added `additional_properties` field to `ToolParameter`**: Stores the `additionalProperties` JSON Schema value (None, False, or a schema dict) for proper schema preservation and strict mode compatibility checking. - **Updated MCP toolset parser**: Now extracts and preserves `additionalProperties` from MCP tool schemas, resolving any nested `$ref` or `anyOf` references. - **Updated documentation**: Clarified that strict mode is now universal with automatic per-tool exclusions for dynamic-key parameters. ## Implementation Details The strict mode logic now follows this flow: 1. Check if `STRICT_TOOL_CALLS_ENABLED` is true (default unless `HOLMES_DISABLE_STRICT_TOOL_CALLS=true`) 2. For each tool, verify all parameters are strict-compatible via `_is_tool_strict_compatible()` 3. Only enable strict mode if both conditions are met 4. When generating schemas in strict mode, set `additionalProperties: false` only on objects with explicit properties, preserving dynamic-key schemas where needed This ensures OpenAI and Anthropic's strict mode requirements (all objects must have `additionalProperties: false`) are met while still supporting tools with dynamic parameters by excluding them from strict mode. https://claude.ai/code/session_01UJPFbZ7Y33QNGRHFDP4sRr <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * New env var TOOL_SCHEMA_NO_PARAM_OBJECT_IF_NO_PARAMS (default false) to omit empty parameter objects. * Automatic coercion of tool-call parameters to match declared JSON Schema types (e.g., parsing stringified JSON, wrapping single values into arrays). * **Bug Fixes** * Strict tool-calling is enabled by default; disable with HOLMES_DISABLE_STRICT_TOOL_CALLS. * Strict-mode now respects per-tool exceptions for dynamic-key or nested parameter schemas. * Tool OpenAI-format generation no longer depends on a target model. * **Documentation** * Env var docs updated; LLMS_WITH_STRICT_TOOL_CALLS retired and replaced by HOLMES_DISABLE_STRICT_TOOL_CALLS. * **Tests** * Added extensive tests for coercion and strict-mode behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors strict tool calling from a model-based allowlist to a universal default with automatic per-tool compatibility detection. Strict mode is now enabled by default for all models and can only be disabled globally via environment variable. Tools with dynamic-key parameters are automatically excluded from strict mode on a per-tool basis.
Key Changes
Simplified strict mode configuration: Replaced
LLMS_WITH_STRICT_TOOL_CALLSmodel allowlist withHOLMES_DISABLE_STRICT_TOOL_CALLSboolean flag. Strict mode is now enabled universally by default.Added
is_strict_compatible()method toToolParameter: Recursively checks if a parameter and all nested parameters can be used in strict mode. Parameters with dynamic keys (additionalPropertiesset to a schema orTrue) are marked as incompatible.Implemented
_is_tool_strict_compatible()helper: Validates all parameters in a tool before enabling strict mode. Tools with any incompatible parameters are automatically excluded from strict mode.Improved
additionalPropertieshandling in schema generation:additionalProperties: falseonly in strict modeadditionalPropertiesschema definitionAdded
additional_propertiesfield toToolParameter: Stores theadditionalPropertiesJSON Schema value (None, False, or a schema dict) for proper schema preservation and strict mode compatibility checking.Updated MCP toolset parser: Now extracts and preserves
additionalPropertiesfrom MCP tool schemas, resolving any nested$reforanyOfreferences.Updated documentation: Clarified that strict mode is now universal with automatic per-tool exclusions for dynamic-key parameters.
Implementation Details
The strict mode logic now follows this flow:
STRICT_TOOL_CALLS_ENABLEDis true (default unlessHOLMES_DISABLE_STRICT_TOOL_CALLS=true)_is_tool_strict_compatible()additionalProperties: falseonly on objects with explicit properties, preserving dynamic-key schemas where neededThis ensures OpenAI and Anthropic's strict mode requirements (all objects must have
additionalProperties: false) are met while still supporting tools with dynamic parameters by excluding them from strict mode.https://claude.ai/code/session_01UJPFbZ7Y33QNGRHFDP4sRr
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests