-
Notifications
You must be signed in to change notification settings - Fork 625
fix: remove unnecessary escape character in regex pattern #764
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
WalkthroughA single regex in AwsBedrockProvider.fetchProviderModels was modified, changing the character class from [a-z0-9-] to [a-z0-9-] for Claude model filtering. No logic, behavior, or public API signatures were changed. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.ts (1)
93-98: Logs should be in English per repo guidelinesThe messages logged here are in Chinese. The coding guidelines say logs/comments should use English. Consider translating to keep consistency.
Example:
- console.error('获取AWS Bedrock Anthropic模型列表出错:', error) + console.error('Failed to fetch AWS Bedrock Anthropic model list:', error) - console.log('从API获取模型列表失败,使用默认模型配置') + console.log('Falling back to default model configuration: failed to fetch model list from API')Guideline reference: “Log and comments should be written in English”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use English for logs and comments
Files:
src/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Strict type checking enabled for TypeScript
**/*.{ts,tsx}: 始终使用 try-catch 处理可能的错误
提供有意义的错误信息
记录详细的错误日志
优雅降级处理
日志应包含时间戳、日志级别、错误代码、错误描述、堆栈跟踪(如适用)、相关上下文信息
日志级别应包括 ERROR、WARN、INFO、DEBUG
不要吞掉错误
提供用户友好的错误信息
实现错误重试机制
避免记录敏感信息
使用结构化日志
设置适当的日志级别
Files:
src/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.ts
src/main/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Main to Renderer: Use EventBus to broadcast events via mainWindow.webContents.send()
Use Electron's built-in APIs for file system and native dialogs
Files:
src/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.ts
src/main/presenter/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
One presenter per functional domain
Files:
src/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.ts
src/main/presenter/llmProviderPresenter/providers/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/main/presenter/llmProviderPresenter/providers/*.ts: Create provider file in src/main/presenter/llmProviderPresenter/providers/ when adding a new LLM provider
Implement coreStream method following standardized event interface in LLM provider files
src/main/presenter/llmProviderPresenter/providers/*.ts: Each file insrc/main/presenter/llmProviderPresenter/providers/*.tsshould handle interaction with a specific LLM API, including request/response formatting, tool definition conversion, native/non-native tool call management, and standardizing output streams to a common event format.
Provider implementations must use acoreStreammethod that yields standardized stream events to decouple the main loop from provider-specific details.
ThecoreStreammethod in each Provider must perform a single streaming API request per conversation round and must not contain multi-round tool call loop logic.
Provider files should implement helper methods such asformatMessages,convertToProviderTools,parseFunctionCalls, andprepareFunctionCallPromptas needed for provider-specific logic.
All provider implementations must parse provider-specific data chunks and yield standardized events for text, reasoning, tool calls, usage, errors, stop reasons, and image data.
When a provider does not support native function calling, it must prepare messages using prompt wrapping (e.g.,prepareFunctionCallPrompt) before making the API call.
When a provider supports native function calling, MCP tools must be converted to the provider's format (e.g., usingconvertToProviderTools) and included in the API request.
Provider implementations should aggregate and yield usage events as part of the standardized stream.
Provider implementations should yield image data events in the standardized format when applicable.
Provider implementations should yield reasoning events in the standardized format when applicable.
Provider implementations should yield tool call events (`tool_call_star...
Files:
src/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
**/*.{js,jsx,ts,tsx}: 使用 OxLint 进行代码检查
Log和注释使用英文书写
Files:
src/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.ts
src/{main,renderer}/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/electron-best-practices.mdc)
src/{main,renderer}/**/*.ts: Use context isolation for improved security
Implement proper inter-process communication (IPC) patterns
Optimize application startup time with lazy loading
Implement proper error handling and logging for debugging
Files:
src/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.ts
src/main/**/*.{ts,js,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
主进程代码放在
src/main
Files:
src/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-check (x64)
🔇 Additional comments (1)
src/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.ts (1)
76-76: Good fix: removing the unnecessary escape for '-' inside a character classPlacing '-' at the end of a character class makes it literal, so removing the backslash resolves the linter warning without changing behavior.
| return ( | ||
| models | ||
| ?.filter((m) => m.modelId && /^anthropic.claude-[a-z0-9\-]+(:\d+)$/g.test(m.modelId)) | ||
| ?.filter((m) => m.modelId && /^anthropic.claude-[a-z0-9-]+(:\d+)$/g.test(m.modelId)) |
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.
💡 Verification agent
🧩 Analysis chain
Escape the dot in anthropic.claude and drop the g flag; also remove the unnecessary capturing group
.matches any character; the patternanthropic.claudewill also match strings likeanthropicXclaude. Escape the dot to ensure we only match the literal..- The global
gflag is unnecessary with.test()and can be a footgun when a RegExp instance is reused; remove it. - The parentheses around
:\d+don’t serve a purpose (no back-reference); remove them.
Apply this minimal change in-place:
-?.filter((m) => m.modelId && /^anthropic.claude-[a-z0-9-]+(:\d+)$/g.test(m.modelId))
+?.filter((m) => m.modelId && /^anthropic\.claude-[a-z0-9-]+:\d+$/.test(m.modelId))Optional: hoist the regex to a top-level constant to avoid re-instantiation and improve readability:
+// Match Bedrock Anthropic Claude model ids like:
+// anthopic.claude-3-5-sonnet-20240620-v1:0
+const CLAUDE_MODEL_ID_RE = /^anthropic\.claude-[a-z0-9-]+:\d+$/;
...
-?.filter((m) => m.modelId && /^anthropic.claude-[a-z0-9-]+(:\d+)$/g.test(m.modelId))
+?.filter((m) => m.modelId && CLAUDE_MODEL_ID_RE.test(m.modelId))You can verify the mismatch caused by the unescaped dot with this quick check:
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import re
current = re.compile(r'^anthropic.claude-[a-z0-9-]+:\d+$')
fixed = re.compile(r'^anthropic\.claude-[a-z0-9-]+:\d+$')
samples = [
"anthropic.claude-3-5-sonnet-20240620-v1:0", # valid
"anthropic-claude-3-5-sonnet-20240620-v1:0", # invalid, but matches current due to dot
"anthropicXclaude-3-5-sonnet-20240620-v1:0", # invalid, but matches current due to dot
]
for s in samples:
print(f"{s:55} current={bool(current.fullmatch(s))} fixed={bool(fixed.fullmatch(s))}")
PYLength of output: 255
Fix regex to correctly match anthropic.claude model IDs
The current pattern allows invalid model IDs (e.g. anthropic-claude…, anthropicXclaude…) due to an unescaped dot, unnecessary global flag, and an unneeded capturing group. Update as follows:
• File: src/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.ts
Line 76
Suggested in-place change:
-?.filter((m) => m.modelId && /^anthropic.claude-[a-z0-9-]+(:\d+)$/g.test(m.modelId))
+?.filter((m) => m.modelId && /^anthropic\.claude-[a-z0-9-]+:\d+$/.test(m.modelId))Optional: hoist the regex to a constant for readability and to avoid re-instantiation:
// Matches Bedrock Anthropic Claude model IDs, e.g.:
// "anthropic.claude-3-5-sonnet-20240620-v1:0"
const CLAUDE_MODEL_ID_RE = /^anthropic\.claude-[a-z0-9-]+:\d+$/;
// …
?.filter((m) => m.modelId && CLAUDE_MODEL_ID_RE.test(m.modelId))🤖 Prompt for AI Agents
In src/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.ts
around line 76, the regex used to filter Anthropic Claude model IDs is incorrect
(unescaped dot, unnecessary global flag, and unneeded capturing group) causing
false positives; replace it with a stricter pattern that escapes the dot,
removes the /g flag and capturing group (e.g. use a regex matching
^anthropic\.claude-[a-z0-9-]+:\d+$) and update the filter to use that regex;
optionally hoist the regex to a named constant (CLAUDE_MODEL_ID_RE) for
readability and to avoid recreating it on every call.
fix
pnpm run lintwarning.Summary by CodeRabbit