Repository navigation
Widen legacy tool parameter root coverage - #91
Merged
Merged
Conversation
Treat schema keywords as field names when they hold legacy fields, accept legacy maps whose other fields lack a single string type, and normalize function entries without an explicit type. Merge tool_schema into upstream_compat so parameter roots and pattern escapes share one copy-on-write pass over tools.
Reviewer's GuideThis PR widens legacy tool-parameter detection without misclassifying standard schemas, then consolidates root wrapping and RE2 pattern cleanup into a copy-on-write Sequence diagram for legacy tool parameter normalizationsequenceDiagram
participant Converter
participant Normalizer
participant Tool
participant Upstream
Converter->>Normalizer: normalize_tool_schemas(body)
Normalizer->>Tool: _normalize_tool(tool, key)
Tool->>Normalizer: _parameter_root(parameters)
alt legacy field map
Normalizer-->>Tool: object schema with properties and required
else standard or extension schema
Normalizer-->>Tool: unchanged parameters
end
Tool->>Normalizer: _normalize_schema_patterns(parameters)
Normalizer-->>Converter: copy-on-write body
Converter->>Upstream: send normalized request
Flow diagram for unified tool schema normalizationflowchart LR
A["Chat request"] --> B["normalize_tool_schemas"]
B --> C["Normalize tools/functions"]
C --> D["Detect legacy parameter root"]
D --> E["Wrap as object properties"]
C --> F["Normalize RE2 pattern escapes"]
B --> G["Preserve standard, nested, and extension schemas"]
E --> H["Copy-on-write normalized request"]
F --> H
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="app/upstream_compat.py" line_range="90" />
<code_context>
- """Normalize tool declarations and structured-output schemas, copying only what changes."""
+def _looks_like_field(value) -> bool:
+ """A legacy field carries a boolean required flag or a field type."""
+ return type(value.get("required")) is bool or isinstance(value.get("type"), (str, list))
+
+
</code_context>
<issue_to_address>
**Constant schema is rewritten as fields**
When `parameters` is a schema root such as `{"const":{"type":"object","required":true}}`, `_looks_like_field` treats the object value of `const` as a legacy field because its data contains a string `type` and boolean `required`; `_parameter_root` then removes that data and emits a different schema, so the caller’s valid constant constraint is lost.
Preserve arbitrary-data keywords such as `const` and `default` when identifying legacy field maps, unless the root can be unambiguously distinguished from a standard schema.
</issue_to_address>
### Comment 2
<location path="app/upstream_compat.py" line_range="90" />
<code_context>
- """Normalize tool declarations and structured-output schemas, copying only what changes."""
+def _looks_like_field(value) -> bool:
+ """A legacy field carries a boolean required flag or a field type."""
+ return type(value.get("required")) is bool or isinstance(value.get("type"), (str, list))
+
+
</code_context>
<issue_to_address>
**Legacy parameters reach upstream unwrapped**
When a legacy parameter map has a schema-keyword-named field containing only `enum` and another field with a boolean `required` flag, `_looks_like_field` returns false for the enum-only field, so `_parameter_root` leaves the legacy map unwrapped and upstream receives the wrong parameter schema.
Recognize enum-only values as field schemas in `_looks_like_field`.
Also at `app/upstream_compat.py:108-109`.
</issue_to_address>
### Comment 3
<location path="app/upstream_compat.py" line_range="90" />
<code_context>
- """Normalize tool declarations and structured-output schemas, copying only what changes."""
+def _looks_like_field(value) -> bool:
+ """A legacy field carries a boolean required flag or a field type."""
+ return type(value.get("required")) is bool or isinstance(value.get("type"), (str, list))
+
+
</code_context>
<issue_to_address>
**Named tool choice is rejected**
When a tool has a `function` object but omits `type`, and the request selects it by name, `_normalize_tool_choice` matches only entries with an explicit `type: "function"`, finds no declaration, and returns HTTP 400 even though tool normalization treats the omitted type as a function.
Make `_normalize_tool_choice` match omitted types the same way tool normalization does.
Also at `app/upstream_compat.py:140-142`.
</issue_to_address>Sourcery assessment
Approval pending. 3 findings to address first.
Blocking findings: app/upstream_compat.py:90, app/upstream_compat.py:90, app/upstream_compat.py:90
Keep const/default roots untouched since their values are instance data, treat enum-only objects as fields, and decide keyword-named keys by the keyword's value kind instead of a full keyword list. Add an explicit "type": "function" to untyped function entries so named tool_choice matches them.
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
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.
Follow-up to #90.
Summary
pattern,descriptionortitleas parameter names when their value is a legacy field, so common tool parameters no longer hide a legacy map from normalization.requiredflag plus all-object values; fields with array types,enum-only fields or notypeno longer abort the conversion.toolsentries that carry afunctionobject without an explicit"type": "function", matching_declared_tool_namesanddsml.tool_name_allowlist.app/tool_schema.pyintoapp/upstream_compat.py: parameter roots and RE2 pattern escapes now share one copy-on-write pass (normalize_tool_schemas) instead of two traversals.$/x-extension roots and nested schemas are still passed through unchanged. README (en/zh-CN) updated accordingly.Verification
for s in tests/test_*.py; do uv run python -B "$s"; done.pyflakesandgit diff --checkare clean.Summary by Sourcery
Widen tool schema normalization to cover more legacy parameter roots and consistently prepare function declarations for upstream processing.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: