Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/core/src/core/openaiContentGenerator/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,9 @@ export const DEFAULT_DASHSCOPE_BASE_URL =
'https://dashscope.aliyuncs.com/compatible-mode/v1';
export const DEFAULT_DEEPSEEK_BASE_URL = 'https://api.deepseek.com/v1';
export const DEFAULT_OPEN_ROUTER_BASE_URL = 'https://openrouter.ai/api/v1';
// Consider exporting a getter function for SDK use:
export const DASHSCOPE_PROXY_BASE_URL = process.env['DASHSCOPE_PROXY_BASE_URL'];
Comment thread
HeZiGang marked this conversation as resolved.
// Or a function:
export function getDashScopeProxyBaseUrl(): string | undefined {
return process.env['DASHSCOPE_PROXY_BASE_URL'];
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,20 @@ vi.mock('../../../utils/runtimeFetchOptions.js', () => ({
buildRuntimeFetchOptions: vi.fn(),
}));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Test mock diverges from production and is fragile

The vi.mock('../constants.js') uses a getter for DASHSCOPE_PROXY_BASE_URL (dynamic read from process.env), but production uses export const (frozen at import time). Additionally, the mock duplicates all 6 other constant exports verbatim — any future constant added to constants.ts would be silently undefined in these tests. Consider using importOriginal to preserve real exports:

Suggested change
}));
vi.mock('../constants.js', async (importOriginal) => {
const actual = await importOriginal();
return {
...actual,
get DASHSCOPE_PROXY_BASE_URL() {
return process.env['DASHSCOPE_PROXY_BASE_URL'];
},
};
});

— glm-5.1 via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is already implemented at lines 60–62. Why is this recommendation showing up?


// Mock DASHSCOPE_PROXY_BASE_URL so tests can control its value
vi.mock('../constants.js', () => ({
DEFAULT_TIMEOUT: 120000,
DEFAULT_MAX_RETRIES: 3,
DEFAULT_OPENAI_BASE_URL: 'https://api.openai.com/v1',
DEFAULT_DASHSCOPE_BASE_URL:
'https://dashscope.aliyuncs.com/compatible-mode/v1',
DEFAULT_DEEPSEEK_BASE_URL: 'https://api.deepseek.com/v1',
DEFAULT_OPEN_ROUTER_BASE_URL: 'https://openrouter.ai/api/v1',
get DASHSCOPE_PROXY_BASE_URL() {
return process.env['DASHSCOPE_PROXY_BASE_URL'];
},
}));

describe('DashScopeOpenAICompatibleProvider', () => {
let provider: DashScopeOpenAICompatibleProvider;
let mockContentGeneratorConfig: ContentGeneratorConfig;
Expand Down Expand Up @@ -162,6 +176,54 @@ describe('DashScopeOpenAICompatibleProvider', () => {
expect(result).toBe(false);
});
});

it('should return true when baseUrl matches DASHSCOPE_PROXY_BASE_URL', () => {
vi.stubEnv(
'DASHSCOPE_PROXY_BASE_URL',
'https://your-proxy.com/dashscope',
);

const config = {
authType: AuthType.USE_OPENAI,
baseUrl: 'https://your-proxy.com/dashscope',
} as ContentGeneratorConfig;

const result =
DashScopeOpenAICompatibleProvider.isDashScopeProvider(config);
expect(result).toBe(true);
});

it('should return false when baseUrl does not match DASHSCOPE_PROXY_BASE_URL', () => {
vi.stubEnv(
'DASHSCOPE_PROXY_BASE_URL',
'https://your-proxy.com/dashscope',
);

const config = {
authType: AuthType.USE_OPENAI,
baseUrl: 'https://other-proxy.com/dashscope',
} as ContentGeneratorConfig;

const result =
DashScopeOpenAICompatibleProvider.isDashScopeProvider(config);
expect(result).toBe(false);
});

it('should return true when baseUrl matches DASHSCOPE_PROXY_BASE_URL with trailing slash', () => {
vi.stubEnv(
'DASHSCOPE_PROXY_BASE_URL',
'https://your-proxy.com/dashscope',
);

const config = {
authType: AuthType.USE_OPENAI,
baseUrl: 'https://your-proxy.com/dashscope/',
} as ContentGeneratorConfig;

const result =
DashScopeOpenAICompatibleProvider.isDashScopeProvider(config);
expect(result).toBe(true);
});
});

describe('buildHeaders', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
DEFAULT_TIMEOUT,
DEFAULT_MAX_RETRIES,
DEFAULT_DASHSCOPE_BASE_URL,
DASHSCOPE_PROXY_BASE_URL,
} from '../constants.js';
import type {
DashScopeRequestMetadata,
Expand All @@ -33,8 +34,24 @@ export class DashScopeOpenAICompatibleProvider extends DefaultOpenAICompatiblePr
if (authType === AuthType.QWEN_OAUTH) return true;
if (!baseUrl) return true;

const normalizedBaseUrl = baseUrl.endsWith('/')
Comment thread
HeZiGang marked this conversation as resolved.
? baseUrl.slice(0, -1)
: baseUrl;

// Matches: dashscope.aliyuncs.com, *.dashscope.aliyuncs.com, or *.dashscope-intl.aliyuncs.com
return /([\w-]+\.)?dashscope(-intl)?\.aliyuncs\.com/i.test(baseUrl);
const isDashscopeOrigin =
/([\w-]+\.)?dashscope(-intl)?\.aliyuncs\.com/i.test(normalizedBaseUrl);

// Check if proxy is configured and matches
const normalizedProxyUrl = DASHSCOPE_PROXY_BASE_URL?.endsWith('/')
? DASHSCOPE_PROXY_BASE_URL.slice(0, -1)
: DASHSCOPE_PROXY_BASE_URL;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Case-sensitivity inconsistency between origin check and proxy check

The origin-domain regex on line 43 uses the /i (case-insensitive) flag, but the proxy URL comparison here uses strict ===. If DASHSCOPE_PROXY_BASE_URL and baseUrl differ only in casing (common in CI/CD environments where env vars and config files come from different sources), the proxy match silently fails and DashScope-specific headers, cache control, and session tracking are all skipped.

Suggested change
const isProxyConfigured = Boolean(
normalizedProxyUrl &&
normalizedBaseUrl.toLowerCase() === normalizedProxyUrl.toLowerCase(),
);

— glm-5.1 via Qwen Code /review

const isProxyConfigured = Boolean(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] No diagnostic logging when proxy URL doesn't match

When DASHSCOPE_PROXY_BASE_URL is set but does not match baseUrl, the code silently falls through to the default provider. DashScope-specific headers (X-DashScope-CacheControl, X-DashScope-UserAgent, X-DashScope-AuthType), cache control, and session tracking are all skipped with zero indication of why. This is the hardest failure mode to debug — the user sees "prompt caching not working" with no error or warning. Consider adding a debug/trace-level log when the proxy URL is set but doesn't match.

— glm-5.1 via Qwen Code /review

Comment thread
HeZiGang marked this conversation as resolved.
Outdated
normalizedProxyUrl && normalizedBaseUrl === normalizedProxyUrl,
);

return isDashscopeOrigin || isProxyConfigured;
}

override buildHeaders(): Record<string, string | undefined> {
Expand Down