-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat: identidy jan for openrouter #5389
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
Signed-off-by: Zhiqiang ZHOU <[email protected]>
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 7a8a38a in 1 minute and 34 seconds. Click for details.
- Reviewed
13lines 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. extensions/engine-management-extension/resources/openrouter.json:9
- Draft comment:
Ensure header entries are properly delimited. If the consuming code expects multiple headers to be separated by newlines (e.g., '\n'), update the template accordingly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment raises a valid concern about HTTP header formatting. HTTP headers are typically separated by newlines in raw HTTP requests. However, without seeing the code that consumes this template, we don't know if newlines are required - the consuming code might handle space-separated headers just fine. The comment is speculative since it starts with "Ensure" and depends on implementation details we can't see. I might be wrong about dismissing this - improper header formatting could cause API requests to fail. This could be a real issue. While header formatting is important, without seeing the consuming code, this comment is speculative. It starts with "Ensure" which violates our rules, and asks the author to verify something rather than pointing out a definite issue. The comment should be deleted because it's speculative and asks for verification rather than pointing out a definite issue. We don't have strong evidence that the current format is problematic.
2. extensions/engine-management-extension/resources/openrouter.json:9
- Draft comment:
It looks like the header template now includes multiple header fields in a single string. In particular, check the term "HTTP-Referer" – if you intend to use the standard header, it is usually just "Referer". Please verify if this is intentional or a typographical error. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a tricky case. While "HTTP-Referer" is indeed non-standard compared to just "Referer", we don't know if OpenRouter specifically requires this format. The comment suggests a change but without strong evidence that it would work. The current format might be intentional for OpenRouter's specific requirements. We don't have access to OpenRouter's API documentation to verify the correct header format. The current format might be specifically required by OpenRouter. Without clear evidence that this is incorrect and would break functionality, we shouldn't suggest changing it. Delete the comment. While it raises an interesting point about HTTP headers, we don't have strong evidence that the current format is wrong or that the suggested change would work with OpenRouter's API.
Workflow ID: wflow_5D7iYeSBEPSKW6Xs
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.
That's awesome. Thanks!!!
|
Oops! I just noticed that we don't route the request via this extension any longer. Can you help me verify the fix again? I think it should be done from |
Signed-off-by: Zhiqiang ZHOU <[email protected]>
|
Hi @louis-menlo ! I have updated code. PTAL ❤️ But I can not test on my local machine because when I execute Should I open another issue for that? Or I missed something ... thanks! |
|
Looks great now, thanks @STRRL. I'll do some tests and then proceed with the merge. |
|
Looking forward to your good news! 🤞 |
|
Hi @STRRL can you please help rebase release/v0.6.2. We are scheduling this fix into the upcoming patch release. |
|
No problem! I will take a look! |
|
@STRRL how can i help you to complete this pr ? |
|
Hi @STRRL please help resolve conflict, we will merge then |
|
sorry for the late reponse, updated PTAL @louis-menlo @samhvw8 |
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
feat: identidy jan for openrouter
|
Cherry-picked into version 0.6.5 |

Describe Your Changes
HTTP-Referer: https://jan.ai,X-Title: Janreference: https://openrouter.ai/docs/api-reference/overview#headers
Fixes Issues
Self Checklist
Important
Update
header_templateinopenrouter.jsonto includeHTTP-RefererandX-Titleheaders for OpenRouter API requests.header_templateinopenrouter.jsonto includeHTTP-Referer: https://jan.aiandX-Title: Janfor OpenRouter API requests.This description was created by
for 7a8a38a. You can customize this summary. It will automatically update as commits are pushed.