Code review summary: usability breakages and safety/stability gaps

[dragonheartcra]

Summary

I reviewed the current codebase and found several issues that impact core usability and stability. The app is usable for basic chat flows, but some advanced workflows are currently broken or inconsistent.

Impact

• Basic chat works in most cases.
• File mention (@...) and some MCP flows are currently unreliable/broken.
• There are avoidable safety/stability risks in upload and file-serving paths.

Findings

1) @ file mention flow is effectively broken (frontend/backend contract mismatch)

• Frontend sends session_id / q to /api/files.
• Backend /api/files now requires dir and rejects missing dir with 400.
• Result: file mention suggestions fail.

Relevant code:

• src/components/chat/MessageInput.tsx (fetch to /api/files with session_id, q)
• src/app/api/files/route.ts (requires dir)

2) MCP add flow for sse/http fails due to validation mismatch

• MCP editor allows sse/http with url and empty command (expected).
• Backend POST /api/plugins/mcp still requires server.command for all types.
• Result: adding sse/http server fails despite valid form data.

Relevant code:

• src/components/plugins/McpServerEditor.tsx
• src/app/api/plugins/mcp/route.ts

3) Attachment files are written twice and can collide on filename

• Upload files are saved in /api/chat and again in claude-client path handling.
• Both paths use timestamp-based naming (Date.now() + sanitized name), which can collide under same-ms writes.
• Result: unnecessary I/O, potential overwrite/collision risk.

Relevant code:

• src/app/api/chat/route.ts
• src/lib/claude-client.ts

4) /api/uploads path check is too weak

• Current guard only checks resolved.includes('.codepilot-uploads').
• No strict base-dir boundary validation and no explicit isFile check before read.
• Result: fragile safety boundary and avoidable runtime errors.

Relevant code:

• src/app/api/uploads/route.ts

5) File security policy is inconsistent across folder picker vs file APIs

• Folder picker allows browsing arbitrary directories.
• File APIs (/api/files, /api/files/preview) enforce baseDir/home constraints.
• Result: user can select a folder that later fails with 403 in file tree/preview.

Relevant code:

• src/app/api/files/browse/route.ts
• src/app/api/files/route.ts
• src/app/api/files/preview/route.ts

6) Some API routes return stack traces in response body

• Several endpoints return error.stack || error.message directly.
• This can leak server internals/paths to clients.

Relevant code:

• src/app/api/chat/sessions/route.ts
• src/app/api/claude-sessions/route.ts
• src/app/api/claude-sessions/import/route.ts

Suggested Fix Order

1. Fix @ mention contract mismatch (/api/files request/response contract).
2. Fix MCP sse/http server validation in backend.
3. Consolidate attachment persistence into a single write path and use collision-safe IDs (UUID/nanoid).
4. Harden /api/uploads boundary checks using strict path-safe guard + file type checks.
5. Unify folder-selection and file-access trust boundary behavior.
6. Stop returning stack traces to clients; log stack server-side only.

Tests To Add

• Integration test for @ file mention suggestions (frontend request contract + API response).
• MCP API tests for stdio, sse, http create/update paths.
• Upload route tests for path traversal/boundary enforcement and isFile behavior.
• Regression test for file attachment write path (single write + unique naming).

[op7418]

Thanks, I will look into it and fix it.

[op7418]

你好！感谢非常详细的 code review，这些反馈很有价值。

逐条回复一下目前的状态：

1) @ 文件提及流程（前后端契约不匹配）
感谢指出，这个确实需要修复。/api/files 的参数契约需要和前端 MessageInput.tsx 对齐。已记录，会在后续版本中修复。

2) MCP sse/http 类型添加失败
MCP 配置读取已在 commit a5cff79 中改进（支持读取 ~/.claude.json 和 ~/.claude/settings.json）。但你提到的 sse/http 类型的 validation 问题确实还需要单独修复，后端不应该对所有类型都要求 command 字段。已记录。

3) 附件文件写入两次 + 文件名碰撞风险
感谢指出，会在后续版本中合并为单次写入路径，并使用 UUID/nanoid 替代 Date.now() 命名。

4) /api/uploads 路径检查不够严格
文件上传和预览的路径安全检查已在 commit 8f963ed 和 c21d8e4 中有所改进（修复了 Windows 跨盘符 403 问题，移除了 process.cwd() 回退）。但你提到的 isFile 检查和更严格的 base-dir 边界验证确实还可以加强。已记录。

5) 文件夹选择器 vs 文件 API 安全策略不一致
确实存在这个问题，文件夹选择器允许浏览任意目录但文件 API 有 baseDir 限制。需要统一信任边界。已记录。

6) API 路由返回 stack trace
感谢指出，生产环境不应该向客户端暴露 stack trace。会在后续版本中修复，改为只在服务端日志记录详细错误。

以上问题中 #1、#2、#6 会优先处理，其余会在后续版本中逐步修复。再次感谢详细的 review！