Merged
Conversation
added 5 commits
March 5, 2026 14:27
📝 WalkthroughWalkthroughThis PR introduces transport-aware MCP server handling (HTTP vs STDIO), improves API error handling for proxy-based issues, refactors CLI mutual-exclusivity checking, adds Swift language support, and implements built-in LSP server configuration with configurable embedding backends. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
This was referenced Mar 5, 2026
Closed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
ccw/frontend/src/pages/McpManagerPage.tsx (1)
658-682: Harden stdio template normalization before populating edit state.
template.serverConfigis loosely shaped; passingcommand/args/envthrough directly can leak malformed values into dialog state. Normalize types beforesetEditingServer(...).Refactor sketch
const serverConfig = template?.serverConfig ?? {}; + const command = + typeof serverConfig.command === 'string' ? serverConfig.command : ''; + const args = Array.isArray(serverConfig.args) + ? serverConfig.args.filter((arg): arg is string => typeof arg === 'string') + : []; + const env = + serverConfig.env && typeof serverConfig.env === 'object' && !Array.isArray(serverConfig.env) + ? Object.fromEntries( + Object.entries(serverConfig.env).filter(([, v]) => typeof v === 'string') + ) + : undefined; if (isHttp) { setEditingServer({ name: template.name, transport: 'http', @@ } else { setEditingServer({ name: template.name, transport: 'stdio', - command: serverConfig.command, - args: serverConfig.args || [], - env: serverConfig.env, + command, + args, + env, scope: 'project', enabled: true, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ccw/frontend/src/pages/McpManagerPage.tsx` around lines 658 - 682, The stdio branch is passing loosely-shaped template.serverConfig fields directly into setEditingServer which can leak malformed types; update the logic around serverConfig/isHttp before calling setEditingServer to normalize and validate values: ensure serverConfig.command is coerced to a string (or ''), serverConfig.args is converted to an array of strings (default [] and filter non-strings), and serverConfig.env is normalized to a Record<string,string> (default {} and stringify values), and likewise ensure serverConfig.url is a string in the http branch; then call setEditingServer with these sanitized fields (referencing serverConfig, isHttp, setEditingServer, and template.name).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ccw/frontend/src/lib/api.ts`:
- Around line 3661-3665: The lookup after fetching servers incorrectly always
prefers project servers; modify the selection to honor config.scope: when
config.scope === 'global' search servers.global first and then servers.project,
otherwise search servers.project first then servers.global; use the existing
fetchMcpServers(options.projectPath) result and fallback to
_buildFallbackServer(serverName, config) if not found, keeping serverName and
config as the identifying symbols to locate the change.
- Around line 3555-3563: The HTTP fallback branch that constructs the returned
server state (the block where transport === 'http' returns { name: serverName,
transport: 'http', url, enabled, scope }) drops optional auth/header fields from
config; update this branch to also copy through any auth or header-related
fields present on config (for example headers, token, apiKey, authorization)
into the returned object, validating types (e.g. string for tokens/keys, object
for headers) before including them so the fallback preserves authentication
info.
In `@ccw/src/commands/view.ts`:
- Around line 25-26: The content-type check is too strict and case-sensitive:
update the isJson detection so it lowercases the header and matches any
JSON-related media type (e.g., contains "json" such as
"application/problem+json") and safely handles missing headers; replace the
current isJson = contentType.includes('application/json') logic (using the
contentType variable obtained via response.headers.get) with a case-insensitive
check that looks for the substring "json" in the contentType string.
In `@ccw/src/core/routes/mcp-routes.ts`:
- Around line 1226-1272: projectPath is currently extracted without trimming,
then later validated with projectPath.trim() and passed to addMcpServerToProject
untrimmed; trim the input immediately after reading it (e.g., normalize
projectPath to a trimmed string or undefined where you set const projectPath)
and use that trimmed value when computing resolvedScope and when calling
addMcpServerToProject so scope inference and persisted paths are consistent;
update references to projectPath in this block (including resolvedScope
computation and the addMcpServerToProject call) to use the trimmed value.
In `@codex-lens/src/codexlens/cli/commands.py`:
- Around line 2803-2806: The code sets use_centralized = not distributed but
later branches still check centralized, causing inconsistencies; fix by making a
single source-of-truth (either update all later branches to check
use_centralized or assign centralized = use_centralized immediately after
use_centralized is computed) so that all conditional logic and
success/formatting branches that reference centralized (e.g., the branches
around the symbols handling output at lines near where centralized is used,
including the blocks using centralized at the branches around 2862–2868 and
2992–3004) use the same variable; ensure any references to centralized are
replaced or synced to use_centralized so mode reporting and routing are
consistent.
In `@codex-lens/tests/test_cli_help.py`:
- Around line 27-34: Add a timeout to the subprocess invocation in
tests/test_cli_help.py by supplying a timeout (e.g. timeout=10) to the existing
subprocess.run(...) call that assigns to proc; optionally wrap the call in a
try/except catching subprocess.TimeoutExpired to fail the test with a clear
message if the CLI hangs. Ensure you modify the subprocess.run call (the one
creating proc) and handle subprocess.TimeoutExpired so the test suite cannot
hang indefinitely.
---
Nitpick comments:
In `@ccw/frontend/src/pages/McpManagerPage.tsx`:
- Around line 658-682: The stdio branch is passing loosely-shaped
template.serverConfig fields directly into setEditingServer which can leak
malformed types; update the logic around serverConfig/isHttp before calling
setEditingServer to normalize and validate values: ensure serverConfig.command
is coerced to a string (or ''), serverConfig.args is converted to an array of
strings (default [] and filter non-strings), and serverConfig.env is normalized
to a Record<string,string> (default {} and stringify values), and likewise
ensure serverConfig.url is a string in the http branch; then call
setEditingServer with these sanitized fields (referencing serverConfig, isHttp,
setEditingServer, and template.name).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f35df561-f367-4e42-9bce-a34a1ba14e0b
📒 Files selected for processing (19)
ccw/frontend/src/components/mcp/CrossCliSyncPanel.tsxccw/frontend/src/lib/api.mcp.test.tsccw/frontend/src/lib/api.tsccw/frontend/src/pages/McpManagerPage.tsxccw/src/commands/view.tsccw/src/core/routes/mcp-routes.tscodex-lens/pyproject.tomlcodex-lens/src/codexlens/api/file_context.pycodex-lens/src/codexlens/cli/commands.pycodex-lens/src/codexlens/cli/embedding_manager.pycodex-lens/src/codexlens/config.pycodex-lens/src/codexlens/lsp/lsp-servers.jsoncodex-lens/src/codexlens/lsp/standalone_manager.pycodex-lens/src/codexlens/search/chain_search.pycodex-lens/tests/lsp/test_packaging_metadata.pycodex-lens/tests/lsp/test_standalone_manager_defaults.pycodex-lens/tests/test_chain_search.pycodex-lens/tests/test_cli_help.pycodex-lens/tests/test_config.py
Comment on lines
+3555
to
+3563
| if (transport === 'http') { | ||
| const url = 'url' in config && typeof config.url === 'string' ? config.url : ''; | ||
| return { | ||
| name: serverName, | ||
| transport: 'http', | ||
| url, | ||
| enabled, | ||
| scope, | ||
| }; |
There was a problem hiding this comment.
HTTP fallback server omits optional auth/header fields.
When fallback is used, HTTP servers keep only url, dropping header/token-related fields from returned state.
💡 Proposed fix
if (transport === 'http') {
const url = 'url' in config && typeof config.url === 'string' ? config.url : '';
+ const headers =
+ 'headers' in config && config.headers && typeof config.headers === 'object'
+ ? (config.headers as Record<string, string>)
+ : undefined;
+ const bearerTokenEnvVar =
+ 'bearerTokenEnvVar' in config && typeof config.bearerTokenEnvVar === 'string'
+ ? config.bearerTokenEnvVar
+ : undefined;
+ const httpHeaders =
+ 'httpHeaders' in config && config.httpHeaders && typeof config.httpHeaders === 'object'
+ ? (config.httpHeaders as Record<string, string>)
+ : undefined;
+ const envHttpHeaders =
+ 'envHttpHeaders' in config && Array.isArray(config.envHttpHeaders)
+ ? config.envHttpHeaders
+ : undefined;
+
return {
name: serverName,
transport: 'http',
url,
+ headers,
+ bearerTokenEnvVar,
+ httpHeaders,
+ envHttpHeaders,
enabled,
scope,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (transport === 'http') { | |
| const url = 'url' in config && typeof config.url === 'string' ? config.url : ''; | |
| return { | |
| name: serverName, | |
| transport: 'http', | |
| url, | |
| enabled, | |
| scope, | |
| }; | |
| if (transport === 'http') { | |
| const url = 'url' in config && typeof config.url === 'string' ? config.url : ''; | |
| const headers = | |
| 'headers' in config && config.headers && typeof config.headers === 'object' | |
| ? (config.headers as Record<string, string>) | |
| : undefined; | |
| const bearerTokenEnvVar = | |
| 'bearerTokenEnvVar' in config && typeof config.bearerTokenEnvVar === 'string' | |
| ? config.bearerTokenEnvVar | |
| : undefined; | |
| const httpHeaders = | |
| 'httpHeaders' in config && config.httpHeaders && typeof config.httpHeaders === 'object' | |
| ? (config.httpHeaders as Record<string, string>) | |
| : undefined; | |
| const envHttpHeaders = | |
| 'envHttpHeaders' in config && Array.isArray(config.envHttpHeaders) | |
| ? config.envHttpHeaders | |
| : undefined; | |
| return { | |
| name: serverName, | |
| transport: 'http', | |
| url, | |
| headers, | |
| bearerTokenEnvVar, | |
| httpHeaders, | |
| envHttpHeaders, | |
| enabled, | |
| scope, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ccw/frontend/src/lib/api.ts` around lines 3555 - 3563, The HTTP fallback
branch that constructs the returned server state (the block where transport ===
'http' returns { name: serverName, transport: 'http', url, enabled, scope })
drops optional auth/header fields from config; update this branch to also copy
through any auth or header-related fields present on config (for example
headers, token, apiKey, authorization) into the returned object, validating
types (e.g. string for tokens/keys, object for headers) before including them so
the fallback preserves authentication info.
Comment on lines
3661
to
+3665
| const servers = await fetchMcpServers(options.projectPath); | ||
| return [...servers.project, ...servers.global].find((s) => s.name === serverName) ?? { | ||
| name: serverName, | ||
| transport: config.transport ?? 'stdio', | ||
| ...(config.transport === 'http' ? { url: config.url! } : { command: config.command! }), | ||
| args: config.args, | ||
| env: config.env, | ||
| enabled: config.enabled ?? true, | ||
| scope: config.scope, | ||
| } as McpServer; | ||
| return ( | ||
| [...servers.project, ...servers.global].find((s) => s.name === serverName) ?? | ||
| _buildFallbackServer(serverName, config) | ||
| ); |
There was a problem hiding this comment.
Return the updated server from the requested scope.
The post-update lookup searches project before global regardless of config.scope. If the same server name exists in both scopes, a global update can return the project server.
💡 Proposed fix
if (options.projectPath) {
const servers = await fetchMcpServers(options.projectPath);
- return (
- [...servers.project, ...servers.global].find((s) => s.name === serverName) ??
- _buildFallbackServer(serverName, config)
- );
+ const scopedServers = config.scope === 'global' ? servers.global : servers.project;
+ return (
+ scopedServers.find((s) => s.name === serverName) ??
+ _buildFallbackServer(serverName, config)
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const servers = await fetchMcpServers(options.projectPath); | |
| return [...servers.project, ...servers.global].find((s) => s.name === serverName) ?? { | |
| name: serverName, | |
| transport: config.transport ?? 'stdio', | |
| ...(config.transport === 'http' ? { url: config.url! } : { command: config.command! }), | |
| args: config.args, | |
| env: config.env, | |
| enabled: config.enabled ?? true, | |
| scope: config.scope, | |
| } as McpServer; | |
| return ( | |
| [...servers.project, ...servers.global].find((s) => s.name === serverName) ?? | |
| _buildFallbackServer(serverName, config) | |
| ); | |
| const servers = await fetchMcpServers(options.projectPath); | |
| const scopedServers = config.scope === 'global' ? servers.global : servers.project; | |
| return ( | |
| scopedServers.find((s) => s.name === serverName) ?? | |
| _buildFallbackServer(serverName, config) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ccw/frontend/src/lib/api.ts` around lines 3661 - 3665, The lookup after
fetching servers incorrectly always prefers project servers; modify the
selection to honor config.scope: when config.scope === 'global' search
servers.global first and then servers.project, otherwise search servers.project
first then servers.global; use the existing fetchMcpServers(options.projectPath)
result and fallback to _buildFallbackServer(serverName, config) if not found,
keeping serverName and config as the identifying symbols to locate the change.
Comment on lines
+25
to
+26
| const contentType = response.headers.get('content-type') || 'unknown'; | ||
| const isJson = contentType.includes('application/json'); |
There was a problem hiding this comment.
Broaden JSON content-type detection to avoid false non-JSON failures.
Line 26 only matches application/json case-sensitively. Valid JSON media types (for example application/problem+json) can be misclassified and trigger the wrong error path.
💡 Suggested patch
- const contentType = response.headers.get('content-type') || 'unknown';
- const isJson = contentType.includes('application/json');
+ const rawContentType = response.headers.get('content-type') || '';
+ const contentType = rawContentType || 'unknown';
+ const normalizedContentType = rawContentType.toLowerCase();
+ const isJson =
+ normalizedContentType.includes('application/json') ||
+ normalizedContentType.includes('+json');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ccw/src/commands/view.ts` around lines 25 - 26, The content-type check is too
strict and case-sensitive: update the isJson detection so it lowercases the
header and matches any JSON-related media type (e.g., contains "json" such as
"application/problem+json") and safely handles missing headers; replace the
current isJson = contentType.includes('application/json') logic (using the
contentType variable obtained via response.headers.get) with a case-insensitive
check that looks for the substring "json" in the contentType string.
Comment on lines
+1226
to
+1272
| const projectPath = typeof body.projectPath === 'string' ? body.projectPath : undefined; | ||
|
|
||
| const enableSandbox = envInput.enableSandbox === true; | ||
|
|
||
| const enabledToolsRaw = envInput.enabledTools; | ||
| let enabledToolsEnv: string; | ||
| if (enabledToolsRaw === undefined || enabledToolsRaw === null) { | ||
| enabledToolsEnv = 'write_file,edit_file,read_file,core_memory,ask_question,smart_search'; | ||
| } else if (Array.isArray(enabledToolsRaw)) { | ||
| enabledToolsEnv = enabledToolsRaw.filter((t): t is string => typeof t === 'string').join(','); | ||
| } else if (typeof enabledToolsRaw === 'string') { | ||
| enabledToolsEnv = enabledToolsRaw; | ||
| } else { | ||
| enabledToolsEnv = 'write_file,edit_file,read_file,core_memory,ask_question,smart_search'; | ||
| } | ||
|
|
||
| // Check if sandbox should be enabled | ||
| const enableSandbox = body.enableSandbox === true; | ||
| const projectRoot = typeof envInput.projectRoot === 'string' ? envInput.projectRoot : undefined; | ||
|
|
||
| // Parse enabled tools from request body | ||
| const enabledTools = Array.isArray(body.enabledTools) && body.enabledTools.length > 0 | ||
| ? (body.enabledTools as string[]).join(',') | ||
| : 'write_file,edit_file,read_file,core_memory,ask_question,smart_search'; | ||
| const allowedDirsRaw = envInput.allowedDirs; | ||
| let allowedDirsEnv: string | undefined; | ||
| if (Array.isArray(allowedDirsRaw)) { | ||
| allowedDirsEnv = allowedDirsRaw.filter((d): d is string => typeof d === 'string').join(','); | ||
| } else if (typeof allowedDirsRaw === 'string') { | ||
| allowedDirsEnv = allowedDirsRaw; | ||
| } | ||
|
|
||
| // Generate CCW MCP server config | ||
| // Use cmd /c on Windows to inherit Claude Code's working directory | ||
| // Generate CCW MCP server config using *server-side* platform detection. | ||
| // On WSL/Linux, this ensures we produce `npx -y ccw-mcp` even when the browser runs on Windows. | ||
| const isWin = process.platform === 'win32'; | ||
| const env: Record<string, string> = { CCW_ENABLED_TOOLS: enabledToolsEnv }; | ||
| if (projectRoot) env.CCW_PROJECT_ROOT = projectRoot; | ||
| if (allowedDirsEnv) env.CCW_ALLOWED_DIRS = allowedDirsEnv; | ||
| if (enableSandbox) env.CCW_ENABLE_SANDBOX = '1'; | ||
|
|
||
| const ccwMcpConfig: Record<string, any> = { | ||
| command: isWin ? "cmd" : "npx", | ||
| args: isWin ? ["/c", "npx", "-y", "ccw-mcp"] : ["-y", "ccw-mcp"], | ||
| env: { | ||
| CCW_ENABLED_TOOLS: enabledTools, | ||
| ...(enableSandbox && { CCW_ENABLE_SANDBOX: "1" }) | ||
| } | ||
| command: isWin ? 'cmd' : 'npx', | ||
| args: isWin ? ['/c', 'npx', '-y', 'ccw-mcp'] : ['-y', 'ccw-mcp'], | ||
| env | ||
| }; | ||
|
|
||
| // Use existing addMcpServerToProject to install CCW MCP | ||
| return addMcpServerToProject(projectPath, 'ccw-tools', ccwMcpConfig); | ||
| const resolvedScope: 'global' | 'project' = scope ?? (projectPath ? 'project' : 'global'); | ||
| if (resolvedScope === 'project') { | ||
| if (!projectPath || !projectPath.trim()) { | ||
| return { error: 'projectPath is required for project scope', status: 400 }; | ||
| } | ||
| return addMcpServerToProject(projectPath, 'ccw-tools', ccwMcpConfig); | ||
| } |
There was a problem hiding this comment.
Trim projectPath before scope inference and persistence.
projectPath is validated with trim() but passed downstream untrimmed. This can create invalid paths and inconsistent scope inference for whitespace input.
💡 Proposed fix
- const projectPath = typeof body.projectPath === 'string' ? body.projectPath : undefined;
+ const projectPath = typeof body.projectPath === 'string' ? body.projectPath.trim() : undefined;
@@
- if (resolvedScope === 'project') {
- if (!projectPath || !projectPath.trim()) {
+ if (resolvedScope === 'project') {
+ if (!projectPath) {
return { error: 'projectPath is required for project scope', status: 400 };
}
return addMcpServerToProject(projectPath, 'ccw-tools', ccwMcpConfig);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const projectPath = typeof body.projectPath === 'string' ? body.projectPath : undefined; | |
| const enableSandbox = envInput.enableSandbox === true; | |
| const enabledToolsRaw = envInput.enabledTools; | |
| let enabledToolsEnv: string; | |
| if (enabledToolsRaw === undefined || enabledToolsRaw === null) { | |
| enabledToolsEnv = 'write_file,edit_file,read_file,core_memory,ask_question,smart_search'; | |
| } else if (Array.isArray(enabledToolsRaw)) { | |
| enabledToolsEnv = enabledToolsRaw.filter((t): t is string => typeof t === 'string').join(','); | |
| } else if (typeof enabledToolsRaw === 'string') { | |
| enabledToolsEnv = enabledToolsRaw; | |
| } else { | |
| enabledToolsEnv = 'write_file,edit_file,read_file,core_memory,ask_question,smart_search'; | |
| } | |
| // Check if sandbox should be enabled | |
| const enableSandbox = body.enableSandbox === true; | |
| const projectRoot = typeof envInput.projectRoot === 'string' ? envInput.projectRoot : undefined; | |
| // Parse enabled tools from request body | |
| const enabledTools = Array.isArray(body.enabledTools) && body.enabledTools.length > 0 | |
| ? (body.enabledTools as string[]).join(',') | |
| : 'write_file,edit_file,read_file,core_memory,ask_question,smart_search'; | |
| const allowedDirsRaw = envInput.allowedDirs; | |
| let allowedDirsEnv: string | undefined; | |
| if (Array.isArray(allowedDirsRaw)) { | |
| allowedDirsEnv = allowedDirsRaw.filter((d): d is string => typeof d === 'string').join(','); | |
| } else if (typeof allowedDirsRaw === 'string') { | |
| allowedDirsEnv = allowedDirsRaw; | |
| } | |
| // Generate CCW MCP server config | |
| // Use cmd /c on Windows to inherit Claude Code's working directory | |
| // Generate CCW MCP server config using *server-side* platform detection. | |
| // On WSL/Linux, this ensures we produce `npx -y ccw-mcp` even when the browser runs on Windows. | |
| const isWin = process.platform === 'win32'; | |
| const env: Record<string, string> = { CCW_ENABLED_TOOLS: enabledToolsEnv }; | |
| if (projectRoot) env.CCW_PROJECT_ROOT = projectRoot; | |
| if (allowedDirsEnv) env.CCW_ALLOWED_DIRS = allowedDirsEnv; | |
| if (enableSandbox) env.CCW_ENABLE_SANDBOX = '1'; | |
| const ccwMcpConfig: Record<string, any> = { | |
| command: isWin ? "cmd" : "npx", | |
| args: isWin ? ["/c", "npx", "-y", "ccw-mcp"] : ["-y", "ccw-mcp"], | |
| env: { | |
| CCW_ENABLED_TOOLS: enabledTools, | |
| ...(enableSandbox && { CCW_ENABLE_SANDBOX: "1" }) | |
| } | |
| command: isWin ? 'cmd' : 'npx', | |
| args: isWin ? ['/c', 'npx', '-y', 'ccw-mcp'] : ['-y', 'ccw-mcp'], | |
| env | |
| }; | |
| // Use existing addMcpServerToProject to install CCW MCP | |
| return addMcpServerToProject(projectPath, 'ccw-tools', ccwMcpConfig); | |
| const resolvedScope: 'global' | 'project' = scope ?? (projectPath ? 'project' : 'global'); | |
| if (resolvedScope === 'project') { | |
| if (!projectPath || !projectPath.trim()) { | |
| return { error: 'projectPath is required for project scope', status: 400 }; | |
| } | |
| return addMcpServerToProject(projectPath, 'ccw-tools', ccwMcpConfig); | |
| } | |
| const projectPath = typeof body.projectPath === 'string' ? body.projectPath.trim() : undefined; | |
| const enableSandbox = envInput.enableSandbox === true; | |
| const enabledToolsRaw = envInput.enabledTools; | |
| let enabledToolsEnv: string; | |
| if (enabledToolsRaw === undefined || enabledToolsRaw === null) { | |
| enabledToolsEnv = 'write_file,edit_file,read_file,core_memory,ask_question,smart_search'; | |
| } else if (Array.isArray(enabledToolsRaw)) { | |
| enabledToolsEnv = enabledToolsRaw.filter((t): t is string => typeof t === 'string').join(','); | |
| } else if (typeof enabledToolsRaw === 'string') { | |
| enabledToolsEnv = enabledToolsRaw; | |
| } else { | |
| enabledToolsEnv = 'write_file,edit_file,read_file,core_memory,ask_question,smart_search'; | |
| } | |
| const projectRoot = typeof envInput.projectRoot === 'string' ? envInput.projectRoot : undefined; | |
| const allowedDirsRaw = envInput.allowedDirs; | |
| let allowedDirsEnv: string | undefined; | |
| if (Array.isArray(allowedDirsRaw)) { | |
| allowedDirsEnv = allowedDirsRaw.filter((d): d is string => typeof d === 'string').join(','); | |
| } else if (typeof allowedDirsRaw === 'string') { | |
| allowedDirsEnv = allowedDirsRaw; | |
| } | |
| // Generate CCW MCP server config using *server-side* platform detection. | |
| // On WSL/Linux, this ensures we produce `npx -y ccw-mcp` even when the browser runs on Windows. | |
| const isWin = process.platform === 'win32'; | |
| const env: Record<string, string> = { CCW_ENABLED_TOOLS: enabledToolsEnv }; | |
| if (projectRoot) env.CCW_PROJECT_ROOT = projectRoot; | |
| if (allowedDirsEnv) env.CCW_ALLOWED_DIRS = allowedDirsEnv; | |
| if (enableSandbox) env.CCW_ENABLE_SANDBOX = '1'; | |
| const ccwMcpConfig: Record<string, any> = { | |
| command: isWin ? 'cmd' : 'npx', | |
| args: isWin ? ['/c', 'npx', '-y', 'ccw-mcp'] : ['-y', 'ccw-mcp'], | |
| env | |
| }; | |
| const resolvedScope: 'global' | 'project' = scope ?? (projectPath ? 'project' : 'global'); | |
| if (resolvedScope === 'project') { | |
| if (!projectPath) { | |
| return { error: 'projectPath is required for project scope', status: 400 }; | |
| } | |
| return addMcpServerToProject(projectPath, 'ccw-tools', ccwMcpConfig); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ccw/src/core/routes/mcp-routes.ts` around lines 1226 - 1272, projectPath is
currently extracted without trimming, then later validated with
projectPath.trim() and passed to addMcpServerToProject untrimmed; trim the input
immediately after reading it (e.g., normalize projectPath to a trimmed string or
undefined where you set const projectPath) and use that trimmed value when
computing resolvedScope and when calling addMcpServerToProject so scope
inference and persisted paths are consistent; update references to projectPath
in this block (including resolvedScope computation and the addMcpServerToProject
call) to use the trimmed value.
Comment on lines
+2803
to
2806
| if centralized and distributed: | ||
| _fail_mutually_exclusive("--centralized", "--distributed", json_mode) | ||
| use_centralized = not distributed | ||
|
|
There was a problem hiding this comment.
Unify embedding mode source-of-truth (use_centralized vs centralized).
Line 2805 determines execution mode via use_centralized, but Lines 2862 and 2992 still branch on centralized. This can report/display the wrong mode and route success formatting through the wrong branch.
💡 Proposed fix
if centralized and distributed:
_fail_mutually_exclusive("--centralized", "--distributed", json_mode)
- use_centralized = not distributed
+ use_centralized = not distributed
@@
- if centralized:
+ if use_centralized:
effective_root = index_root if index_root else (index_path.parent if index_path else target_path)
console.print(f"Index root: [dim]{effective_root}[/dim]")
console.print(f"Mode: [green]Centralized[/green]")
else:
console.print(f"Index: [dim]{index_path}[/dim]")
@@
- if centralized:
+ if use_centralized:
# Centralized mode output
elapsed = data.get("elapsed_time", 0)
console.print(f"[green]v[/green] Centralized embeddings generated successfully!")
...Also applies to: 2862-2868, 2992-3004
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-lens/src/codexlens/cli/commands.py` around lines 2803 - 2806, The code
sets use_centralized = not distributed but later branches still check
centralized, causing inconsistencies; fix by making a single source-of-truth
(either update all later branches to check use_centralized or assign centralized
= use_centralized immediately after use_centralized is computed) so that all
conditional logic and success/formatting branches that reference centralized
(e.g., the branches around the symbols handling output at lines near where
centralized is used, including the blocks using centralized at the branches
around 2862–2868 and 2992–3004) use the same variable; ensure any references to
centralized are replaced or synced to use_centralized so mode reporting and
routing are consistent.
Comment on lines
+27
to
+34
| proc = subprocess.run( | ||
| [sys.executable, "-m", "codexlens", "--help"], | ||
| capture_output=True, | ||
| text=True, | ||
| encoding="utf-8", | ||
| errors="replace", | ||
| env=_subprocess_env(), | ||
| ) |
There was a problem hiding this comment.
Add a timeout to the subprocess help smoke test.
At Line 27, subprocess.run(...) has no timeout, so a hung CLI invocation can stall the full test job.
💡 Proposed fix
proc = subprocess.run(
[sys.executable, "-m", "codexlens", "--help"],
capture_output=True,
text=True,
encoding="utf-8",
errors="replace",
env=_subprocess_env(),
+ timeout=30,
)🧰 Tools
🪛 Ruff (0.15.2)
[error] 27-27: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-lens/tests/test_cli_help.py` around lines 27 - 34, Add a timeout to the
subprocess invocation in tests/test_cli_help.py by supplying a timeout (e.g.
timeout=10) to the existing subprocess.run(...) call that assigns to proc;
optionally wrap the call in a try/except catching subprocess.TimeoutExpired to
fail the test with a clear message if the CLI hangs. Ensure you modify the
subprocess.run call (the one creating proc) and handle subprocess.TimeoutExpired
so the test suite cannot hang indefinitely.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #128
Fixes #129
Fixes #130
Fixes #131
Fixes #132
Fixes #133
Fixes #134
Fixes #135
Fixes #136
Summary
Tests
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements