KEMBAR78
Fix finishReason nullability by tiginamaria · Pull Request #771 · JetBrains/koog · GitHub
Skip to content

Fix finishReason nullability #771

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Oct 1, 2025
Merged

Fix finishReason nullability #771

merged 7 commits into from
Oct 1, 2025

Conversation

tiginamaria
Copy link
Collaborator

Make finishReason nullabable in OpenAI-like provider responces
Fixes #758

Motivation and Context

Breaking Changes

Type of the changes

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Tests improvement
  • Refactoring

Checklist

  • The pull request has a description of the proposed change
  • I read the Contributing Guidelines before opening the pull request
  • The pull request uses develop as the base branch
  • Tests for the changes have been added
  • All new and existing tests passed
Additional steps for pull requests adding a new feature
  • An issue describing the proposed change exists
  • The pull request includes a link to the issue
  • The change was discussed and approved in the issue
  • Docs have been added / updated

@tiginamaria tiginamaria requested review from devcrocod and kpavlov and removed request for kpavlov September 9, 2025 12:07
@github-actions
Copy link

github-actions bot commented Sep 9, 2025 •
edited
Loading

Qodana for JVM

827 new problems were found

Inspection name Severity Problems
Check Kotlin and Java source code coverage 🔶 Warning 803
Vulnerable imported dependency 🔶 Warning 17
Unused import directive 🔶 Warning 6
Missing KDoc for public API declaration 🔶 Warning 1 
@@ Code coverage @@
+ 70% total lines covered
12724 lines analyzed, 8977 lines covered
# Calculated according to the filters of your coverage tool

☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

kpavlov
kpavlov requested changes Sep 10, 2025
View reviewed changes
Copy link
Collaborator

@kpavlov kpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to have a serialization test as well. The example json might be found here

@kpavlov
Copy link
Collaborator

kpavlov commented Sep 10, 2025

Would it make sense to create OpenRouterChoice object for open router instead of using OpenAIChoice from base openai models?

@tiginamaria
Copy link
Collaborator Author

Yep, just added separate class for OpenRouterChoice.
Also tried to add links to docs to be able to compare the data structures

@tiginamaria tiginamaria force-pushed the tigina/open-router-fix branch 2 times, most recently from 82c9695 to e172c3a Compare September 22, 2025 15:19
@tiginamaria tiginamaria requested a review from kpavlov September 22, 2025 15:33
devcrocod
devcrocod approved these changes Sep 29, 2025
View reviewed changes
Copy link
Contributor

@devcrocod devcrocod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@tiginamaria
Copy link
Collaborator Author

I have several questions here:

  1. It seems like OpenRouter does not support structured output for Claude models, or I'm getting something wrong? https://openrouter.ai/models?arch=Claude&fmt=table&order=newest&supported_parameters=structured_outputs. I've removed structured output capability from those models, is it ok? We have an issue for it, can I close it now? The second part of the issue I've fixed by registering generators
RegisteredBasicJsonSchemaGenerators[LLMProvider.OpenRouter] = OpenAIBasicJsonSchemaGenerator
RegisteredStandardJsonSchemaGenerators[LLMProvider.OpenRouter] = OpenAIStandardJsonSchemaGenerator
  1. It seems like for Claude models (except Opus 3) tool_choise parameter does not work (reproduced here by setting named tool choise and several tool, so OpenRouter returns another one). The issue is on OpenRouter side, so I've removed the capability from the model as it's broken (or does not work by design)

aozherelyeva
aozherelyeva approved these changes Sep 30, 2025
View reviewed changes
Copy link
Contributor

@aozherelyeva aozherelyeva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@tiginamaria tiginamaria force-pushed the tigina/open-router-fix branch from 2b56def to 960b250 Compare September 30, 2025 14:27
kpavlov
kpavlov approved these changes Oct 1, 2025
View reviewed changes
Copy link
Collaborator

@kpavlov kpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @tiginamaria
LGTM

@tiginamaria tiginamaria merged commit ce9d463 into develop Oct 1, 2025
11 checks passed
@tiginamaria tiginamaria deleted the tigina/open-router-fix branch October 1, 2025 11:35
Ololoshechkin pushed a commit that referenced this pull request Oct 2, 2025
@tiginamaria @Ololoshechkin
Fix finishReason nullability (#771)
1a6dafc 
Fixes #758

(cherry picked from commit ce9d463)
karloti pushed a commit to karloti/koog that referenced this pull request Oct 21, 2025
@tiginamaria @karloti
Fix finishReason nullability (JetBrains#771)
f600a95 
Fixes JetBrains#758

(cherry picked from commit ce9d463)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kpavlov kpavlov kpavlov approved these changes

@devcrocod devcrocod devcrocod approved these changes

@aozherelyeva aozherelyeva aozherelyeva approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unexpected 'null' value instead of string literal at path: $.choices[0].finishReason

4 participants

@tiginamaria @kpavlov @devcrocod @aozherelyeva