refactor(provider): generate tool schemas from input structs#151
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces ChangesSchema type refactoring and reflection-based generation
Sequence DiagramsequenceDiagram
participant RequestLoop
participant HTTPClient
participant builtinToolSchemaCache
participant builtinToolSchema
participant jsonschema.Reflector
participant PayloadBuilder
RequestLoop->>HTTPClient: openAIChatPayload(request)
HTTPClient->>builtinToolSchemaCache: openAIChatPayload(request, messages)
builtinToolSchemaCache->>builtinToolSchemaCache: for each tool definition
builtinToolSchemaCache->>builtinToolSchema: tool name
builtinToolSchema->>jsonschema.Reflector: Reflect(tool.*Input type)
jsonschema.Reflector-->>builtinToolSchema: schema map
builtinToolSchema-->>builtinToolSchemaCache: cached schema
builtinToolSchemaCache->>PayloadBuilder: OpenAI format tools
PayloadBuilder-->>HTTPClient: payload with tools
HTTPClient-->>RequestLoop: complete payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/provider/tool_schema.go`:
- Around line 151-169: The builtinToolSchemaForName function regenerates JSON
schemas through reflection on every call, causing unnecessary CPU and allocation
overhead. Create a package-level cache map to store the computed schemas for
each tool (indexed by tool name), and populate this cache once at package
initialization rather than on each lookup. Modify builtinToolSchemaForName to
simply return the precomputed schema from the cache map instead of calling
builtinToolSchema which performs reflection and JSON operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 52990dce-90f8-48eb-9b69-e9d1f64a54dd
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.modinternal/assistant/tool_schema_internal_test.gointernal/provider/client.gointernal/provider/tool_schema.gointernal/provider/tool_schema_internal_test.gointernal/tool/ast.go
💤 Files with no reviewable changes (1)
- internal/provider/client.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #151 +/- ##
==========================================
- Coverage 80.52% 80.41% -0.12%
==========================================
Files 279 280 +1
Lines 21879 21978 +99
==========================================
+ Hits 17619 17674 +55
- Misses 3047 3077 +30
- Partials 1213 1227 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3748c2e to
eb7d00f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/extension/manager_loader.go`:
- Around line 268-275: The luaOptionalSchema function uses
tool.MustSchemaFromMap which panics on conversion failures, but since the input
comes from untrusted Lua extension tables, malformed values should be handled
gracefully. Replace the call to tool.MustSchemaFromMap with a version that
returns both the schema and an error (such as tool.SchemaFromMap if it exists),
check for errors, and return tool.EmptySchema() when the conversion fails
instead of allowing a panic to occur.
In `@internal/tool/input_validation.go`:
- Around line 66-73: The unmarshal operation for extracting required fields in
the document struct fails entirely when the JSON contains mixed types or invalid
entries, causing valid required fields to be lost. Modify the logic after the
json.Unmarshal call to handle mixed-type arrays gracefully by filtering and
preserving only valid string entries from the required array instead of
returning nil and false when any non-string values are encountered. Keep the
permissive filtering approach so that partial valid entries in the required
field are preserved and blank or invalid names are filtered out appropriately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b30a5b6-3f1b-448f-b5af-3499479cca38
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (39)
go.modinternal/assistant/extension_tool.gointernal/assistant/extension_tool_internal_test.gointernal/assistant/llm_conversion.gointernal/assistant/llm_conversion_behavior_internal_test.gointernal/assistant/tool_registry.gointernal/assistant/tool_registry_test.gointernal/assistant/tool_schema_internal_test.gointernal/extension/manager_loader.gointernal/extension/manager_test.gointernal/extension/types.gointernal/llm/tools.gointernal/provider/anthropic.gointernal/provider/anthropic_internal_test.gointernal/provider/anthropic_tools_internal_test.gointernal/provider/client.gointernal/provider/openai_chat.gointernal/provider/openai_chat_payload_internal_test.gointernal/provider/openai_responses.gointernal/provider/openai_responses_internal_test.gointernal/provider/provider_payload_hook_internal_test.gointernal/provider/provider_tool_calls_internal_test.gointernal/provider/tool_registry_internal_test.gointernal/provider/tool_schema.gointernal/provider/tool_schema_internal_test.gointernal/tool/ast.gointernal/tool/bash.gointernal/tool/edit.gointernal/tool/extension.gointernal/tool/find.gointernal/tool/grep.gointernal/tool/input_validation.gointernal/tool/input_validation_internal_test.gointernal/tool/ls.gointernal/tool/read.gointernal/tool/schema.gointernal/tool/schema_test.gointernal/tool/types.gointernal/tool/write.go
💤 Files with no reviewable changes (1)
- internal/tool/extension.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/provider/tool_schema_internal_test.go
- go.mod
|



No description provided.