-
Notifications
You must be signed in to change notification settings - Fork 35
Fix/gemini thinking mode #127
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.
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
Conversation
This comment has been minimized.
This comment has been minimized.
Changes were made to the Google Gemini integration in |
src/core/infrastructure/adapters/services/codeBase/crossFileAnalysis.service.ts
Show resolved
Hide resolved
src/core/infrastructure/adapters/services/codeBase/llmAnalysis.service.ts
Show resolved
Hide resolved
src/core/application/use-cases/issues/generate-issues-from-pr-closed.use-case.ts
Show resolved
Hide resolved
maxReasoningTokens: | ||
options.maxReasoningTokens ?? strategy.maxReasoningTokens, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maxReasoningTokens: options.maxReasoningTokens,
The code attempts to access strategy.maxReasoningTokens
within a block where strategy
is known to be undefined. The if (!strategy)
condition on line 55 ensures that this code path is only executed when strategy
is falsy. Accessing a property on an undefined object will cause a TypeError
, crashing the error logging process. The logger should only reference options.maxReasoningTokens
since the strategy-based fallback is not available in this context.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
if (content?.type === 'reasoning') { | ||
return ''; | ||
} | ||
return super._messageContentComplexToString(content); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const filteredContent = content.filter((item: { type: string }) => item.type !== 'reasoning');
return super._messageContentComplexToString(filteredContent);
The content
parameter is of type MessageContentComplex
, which is an array of content blocks (e.g., {type: 'text', ...}
). The current code content?.type
attempts to access a type
property on this array, which will always be undefined
. As a result, the condition content?.type === 'reasoning'
is never met, and the custom logic has no effect.
To correctly handle 'reasoning'
content blocks, you should filter the content
array to remove items of that type before passing the array to the parent method for string conversion. This will correctly remove reasoning steps while preserving other useful content.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found critical issues please review the requested changes
- Relying on a custom parser instead of the LLM provider's native JSON mode makes JSON parsing less reliable.
- Calling
clearIssuesCache
with a potentiallyundefined
value may lead to unintended side effects and compromises data traceability. - A
TypeError
will occur when logging an error for an unsupported provider becausestrategy
is accessed while it isundefined
. - The logic incorrectly checks for a
type
property on an array, causing the intended filtering of 'reasoning' content to never occur.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
The introduction of the |
This pull request addresses several issues and improvements in the
kodustech/kodus-ai
repository, specifically targeting the Gemini thinking mode. Key changes include:Error Handling and Optional Chaining: Introduces optional chaining in
generate-issues-from-pr-closed.use-case.ts
to prevent null reference errors when accessingorganizationId
. However, it highlights a potential issue where a cache-clearing function might be called withundefined
, suggesting the need for a conditional check.Custom String Output Parser: Refactors multiple services to replace the standard
StringOutputParser
with aCustomStringOutputParser
. This change is applied across various files, includingcommentManager.service.ts
,crossFileAnalysis.service.ts
,llmAnalysis.service.ts
,promptRunner.service.ts
,kodyIssuesAnalysis.service.ts
,kodyRulesAnalysis.service.ts
,kodyRulesPrLevelAnalysis.service.ts
, andcodeASTAnalysis.service.ts
. The custom parser aims to enhance compatibility, robustness, and specialized handling of LLM-generated JSON output.LLM Provider Enhancements: Updates in
llmModelProvider.helper.ts
andllmProvider.service.ts
include replacingChatGoogleGenerativeAI
withChatGoogle
for Gemini models and introducing amaxReasoningTokens
parameter. This parameter is integrated into model configurations and error logging metadata, with a fallback to default values.Logical Flaw in Custom Parser: The newly introduced
CustomStringOutputParser
inlangchainCommon/customStringOutputParser.ts
contains a logical flaw, where it incorrectly accesses a property on an array, affecting the filtering of 'reasoning' content blocks.Overall, this pull request focuses on improving error handling, enhancing parsing logic, and updating LLM provider configurations to support the Gemini thinking mode.