feat(OpenAI Node): Support custom headers for model requests #17835
Conversation
|
|
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
mgosal
changed the title
BundleMonUnchanged files (2)
No change in files bundle size Groups updated (2)
Final result: ✅ View report in BundleMon website ➡️ |
idirouhab
force-pushed
the
llm-headers
branch
from
173563a to
200a514
Compare
idirouhab
changed the title
|
Claude finished @idirouhab's task —— View job Code Review Complete ✅I've reviewed PR #17835 - Support custom headers for model requests. Here's my comprehensive analysis: Todo List:
🔍 Review SummaryThis PR adds custom header support to OpenAI API requests. The implementation is well-designed and secure with good test coverage. ✅ Positive Aspects
🛠️ Implementation QualityOpenAI Credentials (
LangChain Node (
🧪 Test Coverage AnalysisCredential Tests: Cover all scenarios including edge cases:
Node Tests: Comprehensive mocking and scenario coverage:
🔒 Security Considerations
📝 Minor Suggestions
// Example validation (optional)
if (credentials.headerName && !/^[a-zA-Z0-9\-_]+$/.test(credentials.headerName)) {
// Handle invalid header name
}📊 Code CoverageThe 71.42% patch coverage with 2 missing lines is acceptable. The missing lines appear to be in edge case handling that may be difficult to test without specific scenarios. 🎯 Final VerdictApproved ✅ - This is a well-implemented feature that:
The implementation is production-ready and follows n8n's development standards. |
idirouhab
force-pushed
the
llm-headers
branch
from
ac63aa8 to
6336773
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
|
||
| expect(result.headers).toEqual({ | ||
| Authorization: 'Bearer sk-test123456789', | ||
| 'OpenAI-Organization': undefined, |
There was a problem hiding this comment.
Avoid enforcing presence of 'OpenAI-Organization' when it’s not set; assert Authorization and separately check that the org header is undefined/absent.
Prompt for AI agents
Address the following comment on packages/nodes-base/credentials/test/OpenAiApi.credentials.test.ts at line 33:
<comment>Avoid enforcing presence of 'OpenAI-Organization' when it’s not set; assert Authorization and separately check that the org header is undefined/absent.</comment>
<file context>
@@ -0,0 +1,155 @@
+
+ expect(result.headers).toEqual({
+ Authorization: 'Bearer sk-test123456789',
+ 'OpenAI-Organization': undefined,
+ });
+ });
</file context>
| expect(result.headers?.['X-Custom-Header']).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should preserve existing headers', async () => { |
There was a problem hiding this comment.
Test name is misleading; it doesn’t verify preserving existing headers since none are provided.
Prompt for AI agents
Address the following comment on packages/nodes-base/credentials/test/OpenAiApi.credentials.test.ts at line 104:
<comment>Test name is misleading; it doesn’t verify preserving existing headers since none are provided.</comment>
<file context>
@@ -0,0 +1,155 @@
+ expect(result.headers?.['X-Custom-Header']).toBeUndefined();
+ });
+
+ it('should preserve existing headers', async () => {
+ const credentials: ICredentialDataDecryptedObject = {
+ apiKey: 'sk-test123456789',
</file context>
| requestOptions: IHttpRequestOptions, | ||
| ): Promise<IHttpRequestOptions> { | ||
| requestOptions.headers = { | ||
| Authorization: 'Bearer ' + credentials.apiKey, |
There was a problem hiding this comment.
Overwrites existing headers instead of merging, which can drop previously set headers and break requests.
Prompt for AI agents
Address the following comment on packages/nodes-base/credentials/OpenAiApi.credentials.ts at line 87:
<comment>Overwrites existing headers instead of merging, which can drop previously set headers and break requests.</comment>
<file context>
@@ -37,22 +39,57 @@ export class OpenAiApi implements ICredentialType {
+ requestOptions: IHttpRequestOptions,
+ ): Promise<IHttpRequestOptions> {
+ requestOptions.headers = {
+ Authorization: 'Bearer ' + credentials.apiKey,
+ 'OpenAI-Organization': credentials.organizationId,
+ };
</file context>
| @@ -358,6 +358,11 @@ export class LmChatOpenAi implements INodeType { | |||
| dispatcher: getProxyAgent(configuration.baseURL ?? 'https://api.openai.com/v1'), | |||
| }; | |||
| } | |||
| if (credentials.header) { | |||
| configuration.defaultHeaders = { | |||
| [credentials.headerName as string]: credentials.headerValue as string, | |||
There was a problem hiding this comment.
Rule violated: Prefer Typeguards over Type casting
Avoid as type assertions for narrowing; prefer type annotations/guards or non-assertive coercion. Replace the header key/value casts with safe string coercion to comply with the rule.
(Based on your team's feedback about avoiding unnecessary runtime type guards in performance‑critical paths, this fix uses simple string coercion without adding guards or overhead.)
Prompt for AI agents
Address the following comment on packages/@n8n/nodes-langchain/nodes/llms/LMChatOpenAi/LmChatOpenAi.node.ts at line 363:
<comment>Avoid `as` type assertions for narrowing; prefer type annotations/guards or non-assertive coercion. Replace the header key/value casts with safe string coercion to comply with the rule.
(Based on your team's feedback about avoiding unnecessary runtime type guards in performance‑critical paths, this fix uses simple string coercion without adding guards or overhead.)</comment>
<file context>
@@ -358,6 +358,11 @@ export class LmChatOpenAi implements INodeType {
}
+ if (credentials.header) {
+ configuration.defaultHeaders = {
+ [credentials.headerName as string]: credentials.headerValue as string,
+ };
+ }
</file context>
| 'OpenAI-Organization': credentials.organizationId, | ||
| }; | ||
| if (credentials.header) { | ||
| requestOptions.headers[credentials.headerName as string] = credentials.headerValue; |
There was a problem hiding this comment.
Rule violated: Prefer Typeguards over Type casting
Avoid using as for type narrowing. Replace the as string assertion with a proper type guard (e.g., check typeof credentials.headerName === 'string' and non-empty) before indexing headers, per "Prefer Typeguards over Type casting".
(Based on your team's feedback about avoiding unnecessary 'as' and using property existence checks in type guards.)
Prompt for AI agents
Address the following comment on packages/nodes-base/credentials/OpenAiApi.credentials.ts at line 91:
<comment>Avoid using `as` for type narrowing. Replace the `as string` assertion with a proper type guard (e.g., check typeof credentials.headerName === 'string' and non-empty) before indexing headers, per "Prefer Typeguards over Type casting".
(Based on your team's feedback about avoiding unnecessary 'as' and using property existence checks in type guards.)</comment>
<file context>
@@ -37,22 +39,57 @@ export class OpenAiApi implements ICredentialType {
+ 'OpenAI-Organization': credentials.organizationId,
+ };
+ if (credentials.header) {
+ requestOptions.headers[credentials.headerName as string] = credentials.headerValue;
+ }
+ return requestOptions;
</file context>
idirouhab
force-pushed
the
llm-headers
branch
from
cb90613 to
cd9bbb0
Compare
Summary
Related Linear tickets, Github issues, and Community forum posts
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)