[FIRE-987] feature: support new tool_use_quality_check & tuq_mode fields and deprecate tool_selection_quality_check & tsq_mode fields#23
Conversation
…lds and deprecate tool_selection_quality_check & tsq_mode fields
WalkthroughAdds new tool-use quality-check fields and deprecated aliases to request schemas, maps legacy/new field names in evaluation construction, re-exports additional types from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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 |
| tool_selection_quality_check: | ||
| evaluationProxyAPIRequest.tool_selection_quality_check || | ||
| evaluationProxyAPIRequest.toolSelectionQualityCheck, | ||
| tool_use_quality_check: evaluationProxyAPIRequest.toolUseQualityCheck, |
There was a problem hiding this comment.
You don't need to send both of them to the proxy.
Send only tool_use_quality_check to the proxy, with value from tool_use_quality_check || tool_selection_quality_check || toolSelectionQualityCheck
| tsq_mode: evaluationProxyAPIRequest.tsqMode, | ||
| tuq_mode: evaluationProxyAPIRequest.tuqMode, |
There was a problem hiding this comment.
send only tuq_mode to the proxy
| tool_selection_quality_check: | ||
| EvaluationRequestV2.toolSelectionQualityCheck, | ||
| tool_use_quality_check: EvaluationRequestV2.toolUseQualityCheck, | ||
| tsq_mode: EvaluationRequestV2.tsqMode, | ||
| tuq_mode: EvaluationRequestV2.tuqMode, |
There was a problem hiding this comment.
no need to send both to the proxy
Co-authored-by: yuval-qf <yuval@qualifire.ai>
Co-authored-by: yuval-qf <yuval@qualifire.ai>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/types.ts:
- Around line 217-223: The validation condition references a non-existent
snake_case field data.tool_use_quality_check; remove that reference so the check
only uses the actual schema fields (data.tool_selection_quality_check,
data.toolSelectionQualityCheck, data.toolUseQualityCheck, and
data.tool_use_quality_check should NOT be referenced). Update the if condition
around the tool selection/use quality checks (the block that currently checks
data.tool_selection_quality_check || data.tool_use_quality_check ||
data.toolSelectionQualityCheck || data.toolUseQualityCheck) to omit
data.tool_use_quality_check and rely on the existing camelCase fields defined by
EvaluationProxyAPIRequestSchema.
In @test/types.spec.ts:
- Line 88: Update the test to use the camelCase field name `toolUseQualityCheck`
consistently: rename the describe string from "validate tool_use_quality_check
requirements" to "validate toolUseQualityCheck requirements", change any
variable bindings (e.g., test variables or destructured names) that reference
`tool_use_quality_check` to `toolUseQualityCheck`, and update the payload
objects sent in the tests to use the `toolUseQualityCheck` key instead of
`tool_use_quality_check`; apply the same replacements in the other failing
assertions referenced (around the other occurrences at the same test file lines
~111-112 and ~126).
- Line 71: The test payload in test/types.spec.ts uses a non-existent snake_case
field tool_use_quality_check while the validation schema
EvaluationProxyAPIRequestSchema defines the camelCase field toolUseQualityCheck,
so Zod will ignore the snake_case key; fix by updating the test payload to use
toolUseQualityCheck (or add a preprocess to EvaluationProxyAPIRequestSchema to
accept snake_case) so the field is validated correctly, ensuring the test
exercises the real schema field.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/index.tssrc/types.tstest/types.spec.ts
🧰 Additional context used
🪛 GitHub Actions: Pull Request
src/types.ts
[error] 220-220: TS2551: Property 'tool_use_quality_check' does not exist on type '{ dangerous_content_check: boolean; grounding_check: boolean; hallucinations_check: boolean; harassment_check: boolean; hate_speech_check: boolean; instructions_following_check: boolean; ... 31 more ...; policyTarget?: "input" | ... 2 more ... | undefined; }'. Did you mean 'toolUseQualityCheck'?
🪛 GitHub Check: build (16.x)
src/types.ts
[failure] 220-220:
Property 'tool_use_quality_check' does not exist on type '{ dangerous_content_check: boolean; grounding_check: boolean; hallucinations_check: boolean; harassment_check: boolean; hate_speech_check: boolean; instructions_following_check: boolean; ... 31 more ...; policyTarget?: "input" | ... 2 more ... | undefined; }'. Did you mean 'toolUseQualityCheck'?
🪛 GitHub Check: build (18.x)
src/types.ts
[failure] 220-220:
Property 'tool_use_quality_check' does not exist on type '{ dangerous_content_check: boolean; grounding_check: boolean; hallucinations_check: boolean; harassment_check: boolean; hate_speech_check: boolean; instructions_following_check: boolean; ... 31 more ...; policyTarget?: "input" | ... 2 more ... | undefined; }'. Did you mean 'toolUseQualityCheck'?
🔇 Additional comments (6)
src/types.ts (4)
5-5: LGTM!The parentheses around
typeofexpressions are correct TypeScript syntax for extracting indexed access types from const arrays.Also applies to: 9-9, 13-13
99-101: LGTM!The new
toolUseQualityCheckfield with proper deprecation annotation for the old field follows the established pattern in this schema.
125-129: LGTM!The deprecation comment for
tsqModeand the newtuqModefield are consistent with the field renaming pattern.
187-192: LGTM!Same deprecation/new field pattern applied consistently to
EvaluationProxyAPIRequestSchema.src/index.ts (2)
7-24: LGTM!The import restructuring and expanded exports (
LLMMessage,ModelMode,PolicyTarget) properly expose the new public types from the SDK.
121-121: LGTM!The JSDoc example correctly demonstrates usage of the new
toolUseQualityCheckfield with a deprecation note for the old field.
- Added tool_use_quality_check field as snake_case alternative to toolUseQualityCheck - Updated validation logic to check all field variants - Modified index.ts to only send new field names to server with fallback support - Used nullish coalescing operator for tuq_mode fallback logic - Maintained backward compatibility with deprecated fields
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/types.ts (1)
169-172: Build error fixed; consider updating the deprecation comment on line 169.Adding
tool_use_quality_checkto the schema resolves the previous TypeScript compilation error where the validation referenced a non-existent property.However, line 169 (unchanged) directs users from
tool_selection_quality_check→toolSelectionQualityCheck, which is itself now deprecated on line 189. Consider updating line 169 to point directly totoolUseQualityCheckto avoid a two-hop deprecation path.♻️ Suggested improvement
- /** @deprecated Use toolSelectionQualityCheck instead */ + /** @deprecated Use toolUseQualityCheck instead */ tool_selection_quality_check: z.boolean().default(false),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.tssrc/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
🔇 Additional comments (6)
src/types.ts (6)
5-5: LGTM!The parenthesized form
(typeof X)[number]is syntactically clearer thantypeof X[number]and both are semantically equivalent. Good improvement for consistency.Also applies to: 9-9, 13-13
99-101: LGTM!Clear deprecation path from
toolSelectionQualityChecktotoolUseQualityCheckwith consistent field definition.
125-129: LGTM!Mode fields follow the established pattern. The deprecation chain
tsqMode→tuqModeis clear.
189-194: LGTM!New fields and deprecations are consistent with the corresponding changes in
EvaluationRequestV2Schema.
219-225: LGTM!The validation now correctly covers all four field variants for backward compatibility. With
tool_use_quality_checknow added to the schema (line 172), the previous build failure is resolved.
229-244: LGTM!The generalized error messages ("tool quality check is enabled") are more maintainable than enumerating specific field names.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/types.ts (1)
169-170: Incorrect deprecation message: points to another deprecated field.The comment directs users to
toolSelectionQualityCheck, which is itself deprecated (line 187). Update to point to the canonical field.📝 Suggested fix
- /** @deprecated Use toolSelectionQualityCheck instead */ + /** @deprecated Use toolUseQualityCheck instead */ tool_selection_quality_check: z.boolean().default(false),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/index.tssrc/types.tstest/types.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/types.spec.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/index.ts (2)
src/types.ts (6)
EvaluationProxyAPIRequest(266-268)EvaluationRequestV2(272-272)EvaluationResponse(269-269)EvaluationProxyAPIRequestSchema(139-243)EvaluationRequestV2Schema(74-137)Framework(5-5)src/frameworks/canonical.ts (1)
CanonicalEvaluationStrategy(3-8)
src/types.ts (1)
src/index.ts (3)
Framework(20-20)ModelMode(22-22)PolicyTarget(23-23)
🔇 Additional comments (8)
src/index.ts (4)
7-24: LGTM!The import reformatting and expanded re-exports (
LLMMessage,ModelMode,PolicyTarget) cleanly expose the public API types needed by consumers.
259-264: LGTM!The mapping correctly consolidates all legacy and new field variants into single proxy fields (
tool_use_quality_check,tuq_mode). Using||for the boolean check and??for the enum mode is appropriate. This aligns with the past review feedback to send only the canonical fields to the proxy.
342-346: LGTM!Consistent with
evaluateWithBackwardCompatibility— the converter path also properly maps all tool quality check variants to the singletool_use_quality_checkproxy field and uses nullish coalescing fortuq_mode.
121-121: LGTM!The example code correctly demonstrates the new
toolUseQualityCheckfield with a helpful inline comment about the deprecation.src/types.ts (4)
5-13: LGTM!The parenthesized type syntax
(typeof X)[number]is the correct TypeScript pattern for extracting union types from const arrays.
99-101: LGTM!The new
toolUseQualityCheckfield and mode fields (tsqModedeprecated,tuqModepreferred) are correctly added with appropriate deprecation annotations.Also applies to: 125-129
187-192: LGTM!The new fields are correctly added with proper deprecation guidance (
tsqMode→tuqMode,toolSelectionQualityCheck→toolUseQualityCheck).
217-242: LGTM!The validation correctly triggers on any of the three tool quality check variants (
tool_selection_quality_check,toolSelectionQualityCheck,toolUseQualityCheck) and the error messages are appropriately generalized. The previous build failure has been resolved.
| }, | ||
| "packageManager": "pnpm@10.14.0" | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
please end the file with an empty line
3 participants
Description of change
Pull-Request Checklist
mainbranchnpm run lintpasses with this changenpm run testpasses with this changeFixes #0000Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.