-
Notifications
You must be signed in to change notification settings - Fork 2.3k
chore: re enable reasoning_content in backend #6228
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
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.
Important
Looks good to me! 👍
Reviewed everything up to e0e58d6 in 57 seconds. Click for details.
- Reviewed
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/src/index.ts:1212
- Draft comment:
Removed the '--reasoning-format none' flag to re-enable reasoning content. Ensure the backend now defaults to producing reasoning output as expected and update docs/tests if necessary. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that the backend defaults to producing reasoning output and to update docs/tests if necessary. This falls under the rule of not asking the author to ensure behavior or make sure changes are tested. Therefore, this comment should be removed.
Workflow ID: wflow_OAFP2HHpaQY7tha9
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 37.81%Your code coverage diff: 0.22% ▴ ✅ All code changes are covered |
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.
Important
Looks good to me! 👍
Reviewed 2782b65 in 1 minute and 20 seconds. Click for details.
- Reviewed
431lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. core/src/browser/extensions/engines/AIEngine.ts:10
- Draft comment:
Clarify that 'reasoning_content' takes precedence over the legacy 'reasoning'. Adding a note here can improve developer understanding. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is suggesting adding a note for clarification, which is a request for purely informative content. This violates the rule against making purely informative comments.
2. web-app/src/utils/reasoning.ts:43
- Draft comment:
Consider documenting the behavior when a chunk contains both reasoning and content tokens. The current logic always prioritizes reasoning if present, which may be intentional but could use explicit clarification. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. web-app/src/utils/reasoning.ts:36
- Draft comment:
For consistency, consider renaming the internal state variable 'isReasoningActive' to something like 'isReasoningInProgress' to match the getter method name. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_9hnq3DrnGLP5lmqx
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
LGTM @qnixsynapse @urmauur
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.
Caution
Changes requested ❌
Reviewed ff63e33 in 1 minute and 50 seconds. Click for details.
- Reviewed
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/utils/reasoning.ts:20
- Draft comment:
Refactor: Using getReasoning in extractReasoningFromMessage cleans up the logic. This improves maintainability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. web-app/src/utils/reasoning.ts:30
- Draft comment:
Consistently applying getReasoning in extractReasoningFromChunk ensures uniform handling of reasoning content across different contexts. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_7FJ6tFhJfQ8dHmC1
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 52a8fd0 in 1 minute and 22 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/utils/reasoning.ts:10
- Draft comment:
Using the nullish coalescing operator (??) here is intentional to prefer a provided empty string over falling back to 'reasoning'. Confirm that empty strings in 'reasoning_content' should be preserved rather than triggering the fallback. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_1AKB9p2GzgWg9qJ9
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Describe Your Changes
Re enable reasoning_content in the llamacpp provider extension for a single standard API for rendering reasoning content
Fixes Issues
Self Checklist
Important
Re-enable
reasoning_contentin llamacpp extension, update reasoning extraction logic, and add tests for reasoning processing.reasoning_contentinAIEngine.tsandindex.tsby removing--reasoning-format none.extractReasoningFromMessage()inreasoning.tsto preferreasoning_contentoverreasoning.reasoning.test.tsto test reasoning extraction and processing.reasoning_content,reasoning, and mixed content.getReasoning()helper function inreasoning.tsfor extracting reasoning content.This description was created by
for 52a8fd0. You can customize this summary. It will automatically update as commits are pushed.