-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: correct context shift flag handling in LlamaCPP extension (#6404) #6431
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
The previous implementation added the `--no-context-shift` flag when `cfg.ctx_shift` was disabled, which conflicted with the llama.cpp CLI where the presence of `--context-shift` enables the feature. The logic is updated to push `--context-shift` only when `cfg.ctx_shift` is true, ensuring the extension passes the correct argument and behaves as expected.
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 06fd2b8 in 1 minute and 4 seconds. Click for details.
- Reviewed
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
draft 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:1544
- Draft comment:
The fix correctly pushes '--context-shift' only when cfg.ctx_shift is true (omitting the flag disables the feature per the llama.cpp CLI). Consider adding an inline comment to clarify this behavior for future maintainers. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_HMTFjhEebD1fVAz3
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 29.38%Your code coverage diff: -0.01% ▾ ✅ 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 cc84180 in 1 minute and 27 seconds. Click for details.
- Reviewed
45
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
draft 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:1547
- Draft comment:
Good fix – now '--context-shift' is pushed only if cfg.ctx_shift is true, matching llama.cpp CLI behavior. Consider adding a brief inline comment for clarity. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. extensions/llamacpp-extension/src/index.ts:1747
- Draft comment:
Nice addition: checking if finish_reason === 'length' in the streaming response converts out‐of‐context conditions to a clear error. Consider extracting this duplicated error check into a helper function. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. extensions/llamacpp-extension/src/index.ts:1832
- Draft comment:
The non-streaming response now properly checks for finish_reason 'length' and throws an error. To reduce code duplication, consider refactoring this repeated logic into a shared function. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. extensions/llamacpp-extension/src/index.ts:1755
- Draft comment:
Typographical note: In the comment, 're‑throw' seems to be using a non-standard hyphen (possibly a non-breaking hyphen). Consider using a standard hyphen ('re-throw') for consistency. - 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_jXCIdRP7ZpTP3sLj
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Describe Your Changes
The previous implementation added the
--no-context-shift
flag whencfg.ctx_shift
was disabled, which conflicted with the llama.cpp CLI where the presence of--context-shift
enables the feature. The logic is updated to push--context-shift
only whencfg.ctx_shift
is true, ensuring the extension passes the correct argument and behaves as expected.Fixes Issues
Self Checklist
Important
Fixes context shift flag handling in LlamaCPP extension and adds error handling for out-of-context conditions.
llamacpp_extension
class inindex.ts
to add--context-shift
only whencfg.ctx_shift
is true.handleStreamingResponse()
andchat()
functions.OUT_OF_CONTEXT_SIZE
constant for error messaging.This description was created by
for cc84180. You can customize this summary. It will automatically update as commits are pushed.