[codex] stabilize repository browsing and settings workflows#12
[codex] stabilize repository browsing and settings workflows#12
Conversation
![codegraphmars[bot]](https://avatars.githubusercontent.com/u/29228178?s=60&v=4){kind=small}
| ], 'https://app.example.com')).toBe(true); | ||
| }); | ||
|
|
||
| it('supports wildcard origins', () => { |
There was a problem hiding this comment.
CRITICAL · 通配符 '*' 允许任意来源存在安全风险
测试用例验证了通配符 '' 会允许任意 origin（如 'https://preview.example.com'）。在 CORS 上下文中使用 '' 是严重的安全配置错误，会绕过 CORS 保护，使应用暴露于跨站请求伪造（CSRF）攻击风险中。生产环境绝不应使用 '*' 作为 allowed origins。
建议：1) 确认生产代码是否真的会接受 '' 作为配置值，如果是，这是一个需要立即修复的安全漏洞。2) 如果 '' 确实是有意设计的开发环境功能，应添加相应的开发环境限定测试，或添加安全注释说明其使用场景。3) 考虑在实现中添加警告日志，当检测到 '*' 配置时发出安全提示。
| } | ||
| } | ||
|
|
||
| recoverInterruptedJobs(): { recoveredCount: number; recoveredJobIds: number[] } { |
There was a problem hiding this comment.
CRITICAL · recoverInterruptedJobs 缺少事务保护，存在部分失败风险
方法遍历所有 processing 状态的作业，对每个作业依次调用 jobModel.markPending、analysisModel.markPending、analysisJobModel.markPending。这三个操作之间没有事务包装，如果在执行过程中发生错误（如数据库连接中断），会导致部分状态更新成功、部分失败，造成数据不一致：job 已标记为 pending 但关联的 analysis 或 analysisJob 仍处于 processing 状态。
建议：将整个恢复逻辑包裹在数据库事务中，或者引入补偿机制确保原子性。示例：使用 jobModel 的事务 API，或在操作前记录恢复计划、操作后验证一致性。考虑使用一个批次操作替代逐条循环。
| */ | ||
| updateStatus(id: number, status: JobStatus): AnalysisJob | null { | ||
| const fields: string[] = []; | ||
| const fields: string[] = ['status = ?']; |
There was a problem hiding this comment.
CRITICAL · 修复 updateStatus SQL 构建 bug
原代码 fields: string[] = [] 会导致 SQL 变成 SET status = ?, , updated_at = ...，出现连续逗号使 UPDATE 语句失败。这是一个会导致所有 status 更新操作直接抛出 SQL 语法错误的严重 bug。
建议：此修改已正确修复该问题，将 fields 初始化为 ['status = ?'] 确保 status 被正确包含在 SET 子句中。无需进一步操作。
| log_info "检查 API 健康状态..." | ||
|
|
||
| local backend_url="http://localhost:${BACKEND_PORT:-7900}/api/health" | ||
| local backend_url="http://localhost:${BACKEND_PORT:-7900}/health" |
There was a problem hiding this comment.
HIGH · 健康检查端点路径可能与服务端实际端点不匹配
将健康检查端点从 /api/health 改为 /health，但根据 docs/api.md:205 中 API 版本路径为 /api/v1/，且服务端路由 (server/src/auth/routes.ts:8) 使用 /api/auth/verify 前缀模式，如果服务端健康检查实际端点不是 /health，此改动将导致 verify.sh 无法正确检测后端健康状态，使得部署验证失败。
建议：确认服务端实际提供的健康检查端点路径。如果服务端使用统一 /api 前缀，应使用 /api/health；如果使用 /api/v1 版本，应使用 /api/v1/health；如果服务端确实提供无前缀的 /health，则此改动正确。
| ```bash | ||
| # 开发环境 | ||
| CORS_ORIGINS=http://localhost:3000 | ||
| CORS_ORIGINS=http://localhost:3000,http://127.0.0.1:3000 |
There was a problem hiding this comment.
HIGH · CORS 配置示例同时包含 localhost 和 127.0.0.1 可能导致认证问题
AGENTS.md:37 指出 'using 127.0.0.1 can create a false negative during login and settings verification'。虽然 localhost 和 127.0.0.1 指向同一台机器，但浏览器可能将它们视为不同的 origin（域名 origin 的判断依赖字符串比较），这可能导致 cookie/session 无法正确共享，造成认证失败。文档明确建议两者都包含，但未警告潜在风险。
建议：在开发环境配置示例后添加说明，或改为只推荐使用 localhost。例如：'注意：如果登录或设置验证遇到问题，尝试只使用 localhost 而不包含 127.0.0.1'。
e2e/dashboard-pages.spec.ts
Outdated
| await page.goto('/dashboard/jobs'); | ||
| await expect(page.getByRole('heading', { name: '作业状态' })).toBeVisible(); | ||
| await expect(page.getByText('analyze_pr')).toBeVisible(); | ||
| await expect(page.getByRole('link', { name: `Job #9301 · ${seededRepositoryName} · PR #42` })).toBeVisible(); |
There was a problem hiding this comment.
HIGH · 硬编码的 Job ID 和 PR 编号与 seed 数据耦合
测试用例在 L37 硬编码了 Job #9301 和 PR #42，这些值由 seed-e2e-data.js 脚本生成。如果 seed 脚本修改了这些 ID（时间戳增量等），测试将立即失败，且需要同步修改测试代码。这增加了维护负担和脆弱性。
建议：考虑从 seed 脚本导出这些动态值（如通过 JSON 文件或环境变量），或使用更宽松的选择器（如 getByText('codagraph-lite-e2e-fixture').first()），避免对具体编号的强依赖。
|
|
||
| const dbPath = path.join(__dirname, '..', 'server', 'data', 'codagraph-lite.db'); | ||
| const db = new Database(dbPath); | ||
| const seededTimestamp = "datetime('now', 'localtime', '+1 day')"; |
There was a problem hiding this comment.
HIGH · 未来时间戳可能导致测试断言不稳定
seededTimestamp 使用 datetime('now', 'localtime', '+1 day') 设置为未来1天的时间。如果E2E测试中有任何依赖当前时间的断言（如检查token是否过期、判断记录是否'最近'创建等），这些测试可能因为时间差而失败。特别是 oauth_installations.token_expires_at 字段虽然当前未填充值，但若应用代码未来依赖该字段与当前时间做比较，测试数据将无法覆盖这些场景。
建议：考虑使用 datetime('now', 'localtime') 获取当前时间戳，仅在明确需要测试'过期'或'未来'场景时才使用固定偏移。如果必须使用未来时间，建议在脚本顶部添加注释说明原因，并确保相关测试明确处理时间边界情况。
| .post('/login') | ||
| .send({ | ||
| username: 'admin', | ||
| password: 'wrong-password', |
There was a problem hiding this comment.
HIGH · 疑似引入敏感信息
变更中出现了密码、token 或 API key 相关字段，需要确认没有把秘密写进仓库。
建议：将密钥迁移到环境变量或密钥管理系统，并避免在源码中硬编码。
CodaGraph Review 摘要
有 2 个文件因为缺少 patch 或类型不受支持而被跳过，当前结论应按 partial review 理解。 模式：LLM + 规则；风险等级：critical；置信度：high。 共审查 28/30 个文件，发现 33 个高价值问题（critical 3 / high 19 / medium 5 / low 6）。 另外有 2 个文件未被完整审查，当前结果应按 partial review 理解。 摘要中的补充问题
建议动作
|
mars167
force-pushed
the
fix/repository-pull-requests-endpoint
branch
from
19adc5d to
949c0f8
Compare
| describe('CORS origin matching', () => { | ||
| it('allows multiple trusted origins from env-style lists', () => { | ||
| expect(isCorsOriginAllowed([ | ||
| 'http://localhost:3000', |
There was a problem hiding this comment.
CRITICAL · 测试用例覆盖不足 - 未验证拒绝不匹配的 origin
测试套件缺少验证不匹配 origin 应返回 false 的用例。如果 isCorsOriginAllowed(['https://app.example.com'], 'https://evil.com') 返回 true 或抛异常，将导致严重的安全漏洞——任何恶意网站都可以绕过 CORS 保护。
建议：添加测试用例：expect(isCorsOriginAllowed(['https://app.example.com'], 'https://evil.com')).toBe(false)
package.json
Outdated
| "build:web": "npm --prefix web run build", | ||
| "build:server": "npm --prefix server run build", | ||
| "build": "npm run build:web && npm run build:server", | ||
| "check:web": "npm --prefix web run lint -- app src next.config.ts", |
There was a problem hiding this comment.
HIGH · check:web 脚本依赖未验证的子项目配置
check:web 调用 web 目录下的 lint 脚本，但未确认 web/package.json 中是否已定义 lint 命令。如果 web 项目未配置 ESLint 或对应的 lint 脚本，此命令将失败，导致 CI/CD 流程中断。
建议：在添加此脚本前，确保 web/package.json 中已正确定义 'lint' 脚本，或考虑在 check 脚本中添加容错逻辑（如使用 || true 避免阻断，或明确记录前置条件）。
| "build:server": "npm --prefix server run build", | ||
| "build": "npm run build:web && npm run build:server", | ||
| "check:web": "npm --prefix web run lint -- app src next.config.ts", | ||
| "check:server": "npm --prefix server run typecheck", |
There was a problem hiding this comment.
HIGH · check:server 脚本依赖未验证的子项目配置
check:server 调用 server 目录下的 typecheck 命令，但未确认 server/package.json 中是否存在 typecheck 脚本。如果 server 项目未配置 TypeScript 类型检查，此命令将失败。
建议：在添加此脚本前，确保 server/package.json 中已正确定义 'typecheck' 脚本（如 tsc --noEmit）。
| updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP | ||
| ); | ||
|
|
||
| INSERT OR REPLACE INTO oauth_installations ( |
There was a problem hiding this comment.
HIGH · 新增 oauth_installations 表的 INSERT 缺少幂等性保护
repository、analysis、jobs 表都使用了 INSERT OR REPLACE 保证幂等性，但新增的 oauth_installations 表（第11-25行创建）也使用 INSERT OR REPLACE，却缺少表可能已存在数据的保护逻辑。如果该表已存在且包含其他记录，INSERT OR REPLACE 可能导致意外的数据丢失。
建议：确认 oauth_installations 表的预期状态，或考虑使用 DELETE 配合条件后再 INSERT，确保与测试预期一致。
| .post('/login') | ||
| .send({ | ||
| username: 'admin', | ||
| password: 'wrong-password', |
There was a problem hiding this comment.
HIGH · 疑似引入敏感信息
变更中出现了密码、token 或 API key 相关字段，需要确认没有把秘密写进仓库。
建议：将密钥迁移到环境变量或密钥管理系统，并避免在源码中硬编码。
|
|
||
| it('returns 401 for unknown users without trying to write an invalid activity log row', async () => { | ||
| adminModelMock.findByUsername.mockReturnValue(null); | ||
| adminModelMock.verifyPassword.mockReturnValue(false); |
There was a problem hiding this comment.
HIGH · 测试中 mock 了不会被调用的方法
第二个测试用例在 L77 设置 findByUsername.mockReturnValue(null) 返回 null 用户后，L78 又调用了 verifyPassword.mockReturnValue(false)。由于用户不存在，routes.ts 中的实现路径应该是先查用户再验证密码，所以 verifyPassword 在该场景下根本不会被调用。这个 mock 设置是无效的，不会影响测试结果，但会误导后续维护者认为该 mock 行为被验证过。
建议：移除 L78 的 verifyPassword.mockReturnValue(false) 调用，或在该行前添加注释说明该 mock 是为了与其他测试保持一致。如果 verifyPassword 确实会在 null 用户场景下被调用，则需要检查 routes.ts 的实现逻辑是否符合预期。
| @@ -0,0 +1,20 @@ | |||
| import { isCorsOriginAllowed } from './index'; | |||
There was a problem hiding this comment.
HIGH · 通配符 '*' 的安全风险未被说明
测试验证了通配符 origin 被允许，但这在生产环境中是极度危险的行为。* 会允许任何第三方网站发起跨域请求。如果这不是预期行为，应在配置层面禁止通配符 origin。
建议：确认业务需求：如果生产环境必须支持通配符，添加注释说明原因；如果不需要，应在配置验证阶段拒绝通配符 origin。
CodaGraph Review 摘要
有 2 个文件因为缺少 patch 或类型不受支持而被跳过，当前结论应按 partial review 理解。 模式：LLM + 规则；风险等级：critical；置信度：high。 共审查 28/30 个文件，发现 37 个高价值问题（critical 1 / high 21 / medium 7 / low 8）。 另外有 2 个文件未被完整审查，当前结果应按 partial review 理解。 摘要中的补充问题
建议动作
|
|
|
||
| # 检查后端 | ||
| if curl -sf http://localhost:7900/api/health >/dev/null 2>&1; then | ||
| if curl -sf "http://localhost:${BACKEND_PORT}/health" >/dev/null 2>&1; then |
There was a problem hiding this comment.
CRITICAL · 后端健康检查路径与实际 API 不匹配
后端健康检查端点从 /api/health 改为了 /health。这会导致健康检查永远失败，除非后端 server 确实暴露 /health 端点而非 /api/health。语义上下文显示后端启动命令为 BACKEND_PORT=7901 npm run start:server，需确认后端路由是否支持 /health 路径。
建议：确认 server/dist 中的路由实现。如果后端健康检查端点仍是 /api/health，应将 L134 改为：if curl -sf "http://localhost:${BACKEND_PORT}/api/health" >/dev/null 2>&1; then
|
|
||
| ### Failed Login Returning 500 | ||
|
|
||
| - Symptom: wrong admin password returns `500 {"details":"FOREIGN KEY constraint failed"}` instead of `401`. |
There was a problem hiding this comment.
HIGH · 文档中暴露了内部错误信息格式，可能被用于探测系统
错误消息 {"details":"FOREIGN KEY constraint failed"} 暴露了数据库约束细节和内部表关系。攻击者可利用此信息：(1) 确认用户账号是否存在（返回500而非401）；(2) 推断数据库表结构。虽然这是对现有 bug 的文档化记录，但文档本身无需包含完整的 JSON 响应体示例。
建议：将 L24 改为：'Symptom: wrong admin password returns 500 with internal database error details instead of 401.' 移除具体的 JSON 响应体示例，避免在文档中固化敏感的错误响应格式。
| log_info "检查 API 健康状态..." | ||
|
|
||
| local backend_url="http://localhost:${BACKEND_PORT:-7900}/api/health" | ||
| local backend_url="http://localhost:${BACKEND_PORT:-7900}/health" |
There was a problem hiding this comment.
HIGH · 健康检查端点路径变更需确认服务器端实现
健康检查路径从 /api/health 改为 /health。根据语义上下文，server/src/auth/routes.ts 定义了 /api/auth/verify 路由，但未找到 /api/health 或 /health 端点的服务器端定义。如果服务器未实现 /health 端点，部署验证将持续失败。
建议：确认服务器端健康检查端点的实际路径。如果服务器使用 Express，通常在入口文件中有 app.get('/health', ...) 或 app.get('/api/health', ...) 的路由定义。建议在修改前验证目标路径在服务器端确实存在。
e2e/dashboard-pages.spec.ts
Outdated
| await page.goto('/dashboard/jobs'); | ||
| await expect(page.getByRole('heading', { name: '作业状态' })).toBeVisible(); | ||
| await expect(page.getByText('analyze_pr')).toBeVisible(); | ||
| await expect(page.getByRole('link', { name: `Job #9301 · ${seededRepositoryName} · PR #42` })).toBeVisible(); |
There was a problem hiding this comment.
HIGH · 硬编码 Job ID 导致测试脆弱
测试断言中硬编码了 Job #9301，这是 e2e 测试数据的自增 ID。如果 seed 脚本重新运行或数据被清理后重新生成，Job ID 可能不是 9301，导致测试失败。e2e 测试的 fixture 数据通常不稳定，硬编码具体 ID 是常见失败原因。
建议：考虑两种方案：(1) 在 seed 脚本返回创建的数据信息，测试中动态获取 job ID；(2) 如果该 job 始终是最新创建的作业，使用模糊匹配如 getByRole('link', { name: new RegExp('Job #.*· .*e2e-fixture') })；或使用 last() 等定位器策略而非固定 ID。
|
|
||
| await page.goto('/dashboard/history'); | ||
| await expect(page.getByRole('heading', { name: '分析历史' })).toBeVisible(); | ||
| await expect(page.getByRole('heading', { name: '按 PR 维度回看 Review 历史' })).toBeVisible(); |
There was a problem hiding this comment.
HIGH · 页面标题变更需确认 UI 同步更新
history 页面的标题从 '分析历史' 改为 '按 PR 维度回看 Review 历史'。这是一个用户可见的文本变更，如果后端代码已更新但前端页面尚未同步此文本，测试将失败。需确认这是预期的前端 UI 变更。
建议：确认 scripts/seed-e2e-data.js 或相关前端组件是否同步更新了标题文本，确保 PR 变更与 UI 代码变更一并部署。
CodaGraph Review 摘要
有 2 个文件因为缺少 patch 或类型不受支持而被跳过，当前结论应按 partial review 理解。 模式：LLM + 规则；风险等级：critical；置信度：high。 共审查 28/30 个文件，发现 48 个高价值问题（critical 4 / high 29 / medium 6 / low 9）。 另外有 2 个文件未被完整审查，当前结果应按 partial review 理解。 摘要中的补充问题
建议动作
|
There was a problem hiding this comment.
CodaGraph Review 摘要
- PR: #12 [codex] stabilize repository browsing and settings workflows
- 风险等级: critical
- 运行模式: improve
- 置信度: high
- 覆盖情况: 28/30 文件
- 跳过文件: 2
- 报告 ID: 9266
- 已发布行级评论: 8
有 2 个文件因为缺少 patch 或类型不受支持而被跳过，当前结论应按 partial review 理解。 模式：LLM + 规则；风险等级：critical；置信度：high。 共审查 28/30 个文件，发现 37 个高价值问题（critical 1 / high 20 / medium 11 / low 5）。 另外有 2 个文件未被完整审查，当前结果应按 partial review 理解。
摘要中的补充问题
-
密码验证逻辑顺序变更可能引入时序攻击风险 (server/src/auth/routes.ts:52)
- 严重性: high
- 描述: 原逻辑先调用 verifyPassword 再检查用户是否存在，新逻辑先 findByUsername 再 verifyPassword。攻击者可能通过测量响应时间差异（用户存在但密码错误 vs 用户不存在）推断用户名是否有效。虽然当前实现中这种差异可能不明显，但这是认证系统中的已知安全反模式。
- 建议: 考虑在 verifyPassword 内部处理用户不存在的情况，使其返回一致的响应时间。或者在 findByUsername 后添加人工延迟以平衡响应时间差异。参考 timing-safe 比较的实现模式。
-
用户存在性检查变为死代码 (server/src/auth/routes.ts:64)
- 严重性: high
- 描述: diff 显示
if (!admin)检查在 L64-L67 被删除，因为新代码在 L52 已经获取了 admin 对象。这个检查现在永远不会触发，因为如果 admin 不存在，isValid 在 L53 已经是 false，会在 L55 的if (!isValid)块中提前返回。这是一个逻辑回归。 - 建议: 删除 L64-L67 的死代码块（if (!admin) return 401 '用户不存在'）。如果需要保留用户不存在时的特殊处理逻辑（如记录到单独的错误日志），需要重新设计这个分支。
-
缺少 CORS 拒绝场景测试 (server/src/config/index.test.ts:1)
- 严重性: high
- 描述: 测试仅覆盖 origin 被允许的正面场景，未验证不可信 origin 应被拒绝的行为。若
isCorsOriginAllowed错误地始终返回 true，所有现有测试仍会通过，无法发现安全回归。 - 建议: 添加测试用例验证：1) 不在允许列表中的 origin 应返回 false；2) 来自不同域的 origin 应被拒绝；3) 空数组输入应返回 false。
-
通配符 '*' 测试存在安全语义歧义 (server/src/config/index.test.ts:17)
- 严重性: high
- 描述: 测试
isCorsOriginAllowed(['*'], 'https://preview.example.com')通过，但通配符 origin 在 CORS 中允许任意来源是危险行为，需确认这是有意的测试设计而非误用。 - 建议: 若通配符是合法配置场景，明确在测试名称中说明；若为误用，应测试更严格的域名匹配逻辑。
-
失败登录场景中 adminId 语义矛盾 (server/src/models/ActivityLog.ts:115)
- 严重性: high
- 描述: logFailedLogin 用于记录未认证用户的登录失败，此时用户身份未确认，理论上不存在有效的 admin_id。原逻辑使用 0 占位，新逻辑改为传入 admin.id，这暗示调用方在认证失败时仍能获取 admin 对象。如果 admin.id 来自不可信输入（如从请求解析），可能导致数据不一致或日志被滥用记录。
- 建议: 确认 auth/routes.ts 中 admin.id 的来源：若来自已存在的 session 或数据库查询则安全；若是从请求参数直接解析则需重新审视业务逻辑。建议在此处添加断言或校验 adminId > 0，或考虑在用户身份未确认时使用 NULL 而非默认值 0。
-
测试断言与实现耦合度过高，脆弱性强 (server/src/platform/client.test.ts:189)
- 严重性: high
- 描述: 两个 GitHubApiClient 测试用例使用了完全相同的参数组合 (owner='mars167', repo='CodaGraph-lite', pr=12) 和相同的 mock fetch 行为。如果未来需要测试其他 owner/repo 组合的错误处理，或者不同 API 端点 (如 issues, commits) 的错误格式化，需要大量重复代码。
- 建议: 考虑将通用错误格式化测试提取为辅助函数，或使用 describe.each/describe.each 来参数化测试不同场景，减少重复并提高测试覆盖率的可扩展性。
-
response.text() 消费响应流导致后续无法读取 (server/src/platform/client.ts:186)
- 严重性: high
- 描述: 在 describeErrorResponse 中调用 response.text() 会消费响应体。如果调用方需要根据错误响应 body 判断错误类型（如 409 Conflict、422 Validation Error）并决定重试或恢复策略，响应流已被消费，无法再次读取。这限制了错误处理和重试逻辑的灵活性。
- 建议: 使用 response.clone() 先克隆响应对象：
const clone = response.clone();
const rawBody = await clone.text();或者只读取前 N 个字符（现代浏览器支持 ReadableStream），避免消费整个响应体。
- 疑似引入敏感信息 (server/src/services/SystemSettingsService.test.ts:64)
- 严重性: high
- 描述: 变更中出现了密码、token 或 API key 相关字段，需要确认没有把秘密写进仓库。
- 建议: 将密钥迁移到环境变量或密钥管理系统，并避免在源码中硬编码。
建议动作
-
确认后端服务是否已实现
/health端点。如果后端仍在使用/api/health（或其他路径），请将 L134 改回curl -sf "http://localhost:${BACKEND_PORT}/api/health"或确保前后端健康检查端点保持一致。也可考虑同时检查两个端点以实现平滑迁移。 -
确认后端健康检查端点的实际路径。如果后端同时支持两路径则无问题；如果只支持
/api/health，需回退此变更或同时更新后端路由配置。建议在变更前验证后端实际暴露的端点。 -
考虑从
seed-e2e-data.js导出生成的 Job ID 和 PR 编号，或在测试中通过查询接口动态获取最新创建的 Job/PR，然后在断言中使用这些动态值，而非硬编码数字。 -
建议在 web 项目的 package.json 中预先配置 lint 的默认路径参数（如 "lint": "next lint app src next.config.ts"），然后 check:web 脚本简化为 "npm --prefix web run lint"。这样更可靠且易于维护。
|
|
||
| # 检查后端 | ||
| if curl -sf http://localhost:7900/api/health >/dev/null 2>&1; then | ||
| if curl -sf "http://localhost:${BACKEND_PORT}/health" >/dev/null 2>&1; then |
There was a problem hiding this comment.
CRITICAL · 后端健康检查端点被静默修改，可能导致部署失败
diff 显示后端健康检查端点从 /api/health 变更为 /health。这是一个破坏性变更——如果后端服务没有同步实现 /health 端点，健康检查将持续失败，导致部署超时退出。当前 diff 中没有看到后端代码同步修改的证据。
建议：确认后端服务是否已实现 /health 端点。如果后端仍在使用 /api/health（或其他路径），请将 L134 改回 curl -sf "http://localhost:${BACKEND_PORT}/api/health" 或确保前后端健康检查端点保持一致。也可考虑同时检查两个端点以实现平滑迁移。
| log_info "检查 API 健康状态..." | ||
|
|
||
| local backend_url="http://localhost:${BACKEND_PORT:-7900}/api/health" | ||
| local backend_url="http://localhost:${BACKEND_PORT:-7900}/health" |
There was a problem hiding this comment.
HIGH · API 端点路径变更需与后端配置对齐
健康检查端点从 /api/health 变更为 /health。从 Impact references 可见，系统存在多个 API 端点（如 /api/v1/auth/verify、/api/auth/verify），且使用 /api/ 前缀。如果后端健康检查实际仍为 /api/health，此变更将导致健康检查永远失败，进而导致部署验证流程中断。
建议：确认后端健康检查端点的实际路径。如果后端同时支持两路径则无问题；如果只支持 /api/health，需回退此变更或同时更新后端路由配置。建议在变更前验证后端实际暴露的端点。
e2e/dashboard-pages.spec.ts
Outdated
| await page.goto('/dashboard/jobs'); | ||
| await expect(page.getByRole('heading', { name: '作业状态' })).toBeVisible(); | ||
| await expect(page.getByText('analyze_pr')).toBeVisible(); | ||
| await expect(page.getByRole('link', { name: `Job #9301 · ${seededRepositoryName} · PR #42` })).toBeVisible(); |
There was a problem hiding this comment.
HIGH · 硬编码的 Job 和 PR 标识符与 seed 数据耦合
L37 中 Job #9301 和 PR #42 是魔法数字，与 seed-e2e-data.js 脚本的输出紧密耦合。当 seed 脚本的 ID 生成逻辑发生变化（如数据库重置、时间戳变化等）时，这些测试会隐性失败，而非产生明确的错误。
建议：考虑从 seed-e2e-data.js 导出生成的 Job ID 和 PR 编号，或在测试中通过查询接口动态获取最新创建的 Job/PR，然后在断言中使用这些动态值，而非硬编码数字。
package.json
Outdated
| "build:web": "npm --prefix web run build", | ||
| "build:server": "npm --prefix server run build", | ||
| "build": "npm run build:web && npm run build:server", | ||
| "check:web": "npm --prefix web run lint -- app src next.config.ts", |
There was a problem hiding this comment.
HIGH · check:web 脚本的路径参数传递可能存在兼容性问题
使用 -- 将额外参数传递给子项目脚本的写法，在某些 npm 版本或 CI 环境中可能无法正确将 'app src next.config.ts' 作为 lint 的路径参数传递。
建议：建议在 web 项目的 package.json 中预先配置 lint 的默认路径参数（如 "lint": "next lint app src next.config.ts"），然后 check:web 脚本简化为 "npm --prefix web run lint"。这样更可靠且易于维护。
playwright.config.ts
Outdated
| timeout: 30_000, | ||
| }, | ||
| { | ||
| command: 'cd web && mkdir -p .next/standalone/.next && ln -sfn ../../static .next/standalone/.next/static && PORT=3001 HOSTNAME=127.0.0.1 NEXT_PUBLIC_API_URL=http://127.0.0.1:7901 node .next/standalone/server.js', |
There was a problem hiding this comment.
HIGH · 静态资源符号链接路径缺少存在性校验
ln -sfn ../../static .next/standalone/.next/static 假设 ../../static 目录存在。如果该目录不存在，命令会失败（创建指向不存在目标的死链接），导致前端启动异常。
建议：在 ln 命令前添加存在性检查：test -d ../../static && ln -sfn ../../static .next/standalone/.next/static || echo 'Static directory not found, skipping symlink'
playwright.config.ts
Outdated
| }, | ||
| { | ||
| command: 'cd web && mkdir -p .next/standalone/.next && ln -sfn ../../static .next/standalone/.next/static && PORT=3001 HOSTNAME=127.0.0.1 NEXT_PUBLIC_API_URL=http://127.0.0.1:7901 node .next/standalone/server.js', | ||
| url: 'http://127.0.0.1:3001/login', |
There was a problem hiding this comment.
HIGH · 健康检查端点使用非标准页面路由
使用 /login 页面作为前端 webServer 的健康检查端点不可靠。登录页面可能因路由守卫、认证逻辑或中间件问题而返回非 200 状态码，即使服务器实际已启动成功。
建议：为前端服务创建一个专门的状态端点（如 /api/health），或在 .next/standalone 中添加 health check endpoint。确保该端点仅检查进程存活状态，不依赖业务逻辑。
| 'completed', 3, 1, 3, NULL, ${seededTimestamp}, ${seededTimestamp}, ${seededTimestamp}, ${seededTimestamp} | ||
| ); | ||
| `); | ||
|
|
There was a problem hiding this comment.
HIGH · 缺少错误处理和数据库关闭保证
db.exec()执行可能因磁盘空间不足、权限问题或数据库锁而失败，当前代码没有try-catch包装。如果执行中途失败，数据库连接也不会被正确关闭（db.close()在L114），可能导致资源泄漏。
建议：使用try-finally或try-catch-finally确保db.close()始终执行：
try {
db.exec(`...`);
console.log(`Seeded E2E data into ${dbPath}`);
} finally {
db.close();
}| .post('/login') | ||
| .send({ | ||
| username: 'admin', | ||
| password: 'wrong-password', |
There was a problem hiding this comment.
HIGH · 疑似引入敏感信息
变更中出现了密码、token 或 API key 相关字段，需要确认没有把秘密写进仓库。
建议：将密钥迁移到环境变量或密钥管理系统，并避免在源码中硬编码。
There was a problem hiding this comment.
Pull request overview
This PR hardens several stability paths across the dashboard (repositories/settings/auth), adds multi-LLM profile support, improves health checks, and strengthens platform client error reporting to reduce flaky browsing/configuration workflows.
Changes:
- Introduce multi-profile LLM settings (profiles list + active selection) with modal editing and backend normalization/persistence.
- Stabilize repository browsing via safer session cache handling and optional remote sync skipping for E2E.
- Improve operational checks (frontend
/api/health, backend/health+/api/health), CORS origin normalization, and richer platform-client error messages; refresh E2E seeding + Playwright startup.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/types/index.ts | Add LlmProfile and extend SystemSettings with profiles + active profile id. |
| web/src/components/ui/Input.tsx | Add endAdornment support; refactor PasswordInput to use it. |
| web/app/login/page.tsx | Replace anchor with Next.js Link for navigation. |
| web/app/dashboard/users/page.tsx | Wrap initial data load in useCallback + effect dependency stabilization. |
| web/app/dashboard/settings/page.tsx | Add LLM profiles UI (list + modal editor) and normalize settings/profiles client-side. |
| web/app/dashboard/repositories/page.tsx | Sanitize session cache store and avoid caching empty repository pages. |
| web/app/dashboard/page.tsx | Layout tweaks to prevent overflow and improve responsive grids. |
| web/app/dashboard/connections/page.tsx | Wrap initial load in useCallback + effect dependency stabilization. |
| web/app/api/health/route.ts | Add frontend health endpoint. |
| server/src/services/SystemSettingsService.ts | Add LLM profiles to settings schema; normalize legacy flat fields into active profile; persist selection. |
| server/src/services/SystemSettingsService.test.ts | Add tests for LLM profile normalization and persistence. |
| server/src/routes/settingsRoutes.ts | Expand LLM test payload typing to include profile fields. |
| server/src/routes/repositoryRoutes.ts | Allow skipping remote repository sync via env flag for stability/E2E. |
| server/src/platform/client.ts | Centralize JSON requests; improve fetch/error diagnostics incl. body previews and cause metadata. |
| server/src/platform/client.test.ts | Add GitHub client tests for enhanced error messages. |
| server/src/models/ActivityLog.ts | Change failed-login logging to accept a valid adminId. |
| server/src/index.ts | Add /api/health alias for backend health; CORS origin check via helper. |
| server/src/config/index.ts | Normalize CORS origins; add isCorsOriginAllowed helper. |
| server/src/config/index.test.ts | Add tests for CORS origin matching/normalization. |
| server/src/auth/routes.ts | Avoid FK failures on failed login by logging only when admin exists. |
| server/src/auth/routes.test.ts | Add tests covering failed-login behavior for existing vs unknown admins. |
| scripts/seed-e2e-data.js | Improve module resolution for sqlite; create oauth table; refresh seeded records for E2E. |
| playwright.config.ts | Update baseURL/ports; add webServer orchestration for backend + standalone frontend. |
| package.json | Add check scripts and test:e2e wiring. |
| e2e/dashboard-pages.spec.ts | Update assertions to match seeded repo/job names and new UI text/flows. |
| docs/configuration.md | Document multi-LLM initialization behavior; update CORS examples. |
| deploy/verify.sh | Adjust backend health check path. |
| deploy/systemd/codagraph-lite-frontend.service | Update service type and ensure Next binds to 0.0.0.0:3000. |
| deploy/start.sh | Harden startup health checks for both frontend and backend. |
| AGENTS.md | Add project “memory” notes about known regressions and validation workflow. |
| .env.example | Update CORS docs and clarify LLM env vars as defaults for initial profile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [Service] | ||
| Type=notify | ||
| Type=simple | ||
| NotifyAccess=all |
There was a problem hiding this comment.
The unit switches to Type=simple but keeps NotifyAccess=all. NotifyAccess is only relevant for notify-based services, so this is likely a leftover and may produce confusing configuration warnings. Either remove NotifyAccess or keep Type=notify and ensure the process actually sends readiness notifications.
| NotifyAccess=all |
| const rawMaxRetries = typeof profile?.maxRetries === 'number' ? profile.maxRetries : Number.NaN; | ||
|
|
||
| return { | ||
| id: profile?.id?.trim() || defaultLlmProfile.id || `llm-profile-${index + 1}`, |
There was a problem hiding this comment.
normalizeLlmProfile() always falls back to defaultLlmProfile.id when profile.id is empty, even for index > 0. That can cause multiple profiles to collapse to the same id and be de-duplicated by the filter, effectively dropping profiles returned from the backend if any id is missing/blank. Use an index-based fallback for non-first profiles (or generate a new id) so each profile remains distinct.
| id: profile?.id?.trim() || defaultLlmProfile.id || `llm-profile-${index + 1}`, | |
| id: | |
| (typeof profile?.id === 'string' && profile.id.trim().length > 0) | |
| ? profile.id.trim() | |
| : index === 0 | |
| ? (defaultLlmProfile.id || `llm-profile-${index + 1}`) | |
| : `llm-profile-${index + 1}`, |
| <button | ||
| type="button" | ||
| onClick={() => setShowPassword((current) => !current)} | ||
| className="inline-flex items-center text-gray-500 hover:text-gray-700 dark:text-gray-400 dark:hover:text-gray-200" | ||
| tabIndex={-1} | ||
| > |
There was a problem hiding this comment.
PasswordInput’s visibility toggle button is not keyboard-focusable (tabIndex={-1}) and has no accessible name. This makes the control unusable for keyboard/screen-reader users. Consider removing tabIndex={-1} and adding an aria-label (and/or visually hidden text) describing the action (e.g., show/hide password).
| export function isCorsOriginAllowed(allowedOrigins: string[], origin: string): boolean { | ||
| const normalizedAllowedOrigins = normalizeCorsOrigins(allowedOrigins); | ||
| if (normalizedAllowedOrigins.includes('*')) { | ||
| return true; | ||
| } | ||
|
|
||
| return normalizedAllowedOrigins.includes(normalizeCorsOrigin(origin)); | ||
| } |
There was a problem hiding this comment.
isCorsOriginAllowed() re-normalizes the full allowedOrigins list on every call. Since loadConfig() already normalizes corsOrigins once, this adds unnecessary per-request work in the CORS callback. Consider normalizing allowedOrigins once (at config load) and, here, only normalizing the incoming origin before matching (or accept a pre-normalized list).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 949c0f8153
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const response = await apiClient.testLlmSettings({ | ||
| llmProvider: settings.llmProvider, | ||
| llmApiKey: settings.llmApiKey, | ||
| llmApiBaseUrl: settings.llmApiBaseUrl, | ||
| llmModel: settings.llmModel, | ||
| llmMaxRetries: settings.llmMaxRetries, | ||
| llmProvider: editingLlmProfile.provider, | ||
| llmApiKey: editingLlmProfile.apiKey, | ||
| llmApiBaseUrl: editingLlmProfile.apiBaseUrl, | ||
| llmModel: editingLlmProfile.model, |
There was a problem hiding this comment.
This test request only sends flat llm* fields, but the backend now resolves effective config from llmProfiles plus activeLlmProfileId. As a result, clicking “测试当前编辑配置” while editing a non-active (or newly added unsaved) profile can test the saved active profile instead, producing misleading pass/fail results for the wrong credentials. Include the selected profile context (e.g., llmProfiles with activeLlmProfileId set to selectedLlmProfileId) when calling the test API.
Useful? React with 👍 / 👎.
| llmProvider: activeLlmProfile.provider, | ||
| llmApiKey: activeLlmProfile.apiKey, | ||
| llmApiBaseUrl: activeLlmProfile.apiBaseUrl, | ||
| llmModel: activeLlmProfile.model, | ||
| llmMaxRetries: activeLlmProfile.maxRetries, |
There was a problem hiding this comment.
Normalization now always mirrors llmProvider/llmApiKey/llmApiBaseUrl/llmModel/llmMaxRetries from the resolved active profile, so callers that pass only legacy flat overrides are silently ignored whenever llmProfiles exists. That breaks backward-compatible update paths (and contributes to testing/saving the wrong profile unless callers also rewrite profile arrays).
Useful? React with 👍 / 👎.
| ], 'https://app.example.com')).toBe(true); | ||
| }); | ||
|
|
||
| it('supports wildcard origins', () => { |
There was a problem hiding this comment.
CRITICAL · 测试用例将通配符 CORS 配置验证为安全行为
第 17-19 行的测试用例将 ['*'] 作为可信来源列表，并期望任意来源（如 'https://preview.example.com'）都能通过验证。这是在测试一个严重的安全漏洞：通配符 Access-Control-Allow-Origin: * 允许任何恶意网站向该服务端点发起跨域请求，可能导致 CSRF 攻击和数据泄露。此测试用例将这种不安全行为规范化为预期正确的功能，没有任何警告或风险提示。
建议：如果业务上确实需要允许所有来源（例如公开 API），应使用明确的域名白名单或添加安全警告注释。如果 '' 通配符是错误配置，测试应反映这一点（期望失败或标记为危险配置）。建议添加一个专门描述通配符风险的测试用例，或在 CORS 配置模块中添加对 '' 的安全警告。
|
|
||
| # 检查后端 | ||
| if curl -sf http://localhost:7900/api/health >/dev/null 2>&1; then | ||
| if curl -sf "http://localhost:${BACKEND_PORT}/health" >/dev/null 2>&1; then |
There was a problem hiding this comment.
HIGH · 后端健康检查端点与原配置不一致
健康检查端点从 /api/health 变更为 /health。旧代码检查的是 /api/health，新代码检查 /health。这可能导致健康检查失败，如果后端服务实际只暴露了 /api/health 端点。
建议：确认 server/dist 的实际健康检查路由。如果 server 同时暴露两个端点则无问题，否则需将端点改回 /api/health 或同步更新 server 的路由配置。
| log_info "检查 API 健康状态..." | ||
|
|
||
| local backend_url="http://localhost:${BACKEND_PORT:-7900}/api/health" | ||
| local backend_url="http://localhost:${BACKEND_PORT:-7900}/health" |
There was a problem hiding this comment.
HIGH · 健康检查端点路径变更可能导致验证失败
将健康检查端点从 /api/health 改为 /health。根据语义上下文，docs/api.md 定义的 API 路径为 /api/v1/auth/verify，前端 API 客户端调用的是 /api/auth/verify。如果后端服务将所有 /api/* 路径作为业务 API 统一管理，健康检查可能实际由 /api/health 提供而非 /health。此变更可能导致部署验证脚本无法正确检测后端服务状态。
建议：确认后端实际提供的健康检查端点路径。如果使用路径前缀 /api 统一管理服务，应保持 /api/health。可通过检查 server 代码中的路由配置（如 Express/ Gin 的 health 路由定义）确认。
| @@ -19,9 +21,10 @@ test('repositories page renders seeded repository', async ({ page }) => { | |||
|
|
|||
There was a problem hiding this comment.
HIGH · 页面错误监听器注册时机晚于页面导航
在第20行 page.on('pageerror', ...) 注册错误监听器后，第22行才执行 page.goto('/dashboard/repositories')。如果页面加载过程中立即抛出错误（如 JS 执行初始化脚本失败），此时错误监听器可能尚未完全就绪，导致页面错误无法被捕获，测试无法正确验证页面稳定性。jobs 和 history 测试存在相同问题（第34行注册、L35导航；第47行注册、L49导航）。
建议：将 page.on('pageerror', ...) 调用移至 beforeEach 中，在任何 page.goto 之前统一注册一次。或者在每个测试的开头、第一行就注册监听器，再进行导航。
| await expect(page.getByText('analyze_pr')).toBeVisible(); | ||
| await expect( | ||
| page.getByRole('link', { | ||
| name: new RegExp(`Job #\\d+ · ${seededRepositoryName} · PR #\\d+`), |
There was a problem hiding this comment.
HIGH · 正则表达式硬编码 Job 链接格式，脆弱且无回退断言
正则表达式 Job #\d+ · ${seededRepositoryName} · PR #\d+ 假设前端 Job 链接的文本格式完全匹配。如果前端 UI 文本发生微小变化（如分隔符从 · 改为 -、顺序调整、翻译变更），测试会失败，即使后端数据和核心功能正常。这违反了测试金字塔原则——过度依赖 UI 文本细节使测试变得脆弱。
建议：增加一个或多个不依赖精确文本格式的断言，例如检查链接元素存在且 href 指向正确的端点：expect(page.locator('a[href*="/jobs/"]')).toBeVisible()。将正则匹配降级为可选的辅助断言。
| "build": "npm run build:web && npm run build:server", | ||
| "check:web": "npm --prefix web run lint", | ||
| "check:server": "npm --prefix server run typecheck", | ||
| "check": "npm run check:web && npm run check:server && npm run build", |
There was a problem hiding this comment.
HIGH · check 脚本的执行顺序可能导致资源浪费
check 脚本按顺序执行 check:web && check:server && npm run build。如果 lint/typecheck 失败，后续的 build 仍会被尝试执行，浪费构建时间。通常 lint/typecheck 应该在 build 之前运行，以便尽早发现问题。
建议：考虑将 check 脚本改为 npm run check:web && npm run check:server（去掉 build），将 build 交给单独的 build 命令。或者，如果确实需要 check + build 的组合，应在 check 失败时立即退出，不执行 build。
| timeout: 30_000, | ||
| }, | ||
| { | ||
| command: 'cd web && mkdir -p .next/standalone/.next && if [ -d .next/static ]; then ln -sfn ../../static .next/standalone/.next/static; fi && PORT=3001 HOSTNAME=127.0.0.1 NEXT_PUBLIC_API_URL=http://127.0.0.1:7901 node .next/standalone/server.js', |
There was a problem hiding this comment.
HIGH · 前端服务器启动命令链过长，单点故障难以诊断
webServer 命令使用 && 链式执行 5 个操作：mkdir、if 条件判断、ln 命令、PORT/HOSTNAME/NEXT_PUBLIC_API_URL 设置、node 启动。如果任一步骤失败（如 standalone 构建缺失、文件权限问题、目录不存在），Playwright 仅报告超时，开发者需要手动运行命令才能定位具体故障点。这会显著增加调试成本，特别是在 CI 环境中。
建议：考虑拆分为两步：(1) 在项目根目录添加 scripts/start-frontend-for-e2e.ts 脚本，负责预检查和准备逻辑；(2) webServer command 改为调用该脚本，使失败信息更明确。或将关键步骤的错误输出捕获并显示。
| const seededTimestamp = "datetime('now', 'localtime', '+1 day')"; | ||
| const seededAdminPasswordHash = crypto | ||
| .createHash('sha256') | ||
| .update('changeme') |
There was a problem hiding this comment.
HIGH · 硬编码测试凭证存在意外泄露风险
admin 密码和 OAuth token 使用硬编码字符串（'changeme'、'e2e-token'）。虽然这是 E2E 测试脚本，但如果错误地作为生产数据库初始化脚本运行，这些凭证可能被意外使用。
建议：考虑从环境变量读取敏感数据，例如：const adminPassword = process.env.E2E_ADMIN_PASSWORD || 'changeme';。或在脚本头部添加明确的危险警告注释。
CodaGraph Review 摘要
有 2 个文件因为缺少 patch 或类型不受支持而被跳过，当前结论应按 partial review 理解。 模式：LLM + 规则；风险等级：critical；置信度：high。 共审查 28/30 个文件，发现 30 个高价值问题（critical 1 / high 17 / medium 6 / low 6）。 另外有 2 个文件未被完整审查，当前结果应按 partial review 理解。 摘要中的补充问题
建议动作
|
Summary
This branch hardens several user-facing stability paths across repository browsing, auth/settings, deployment health checks, and LLM configuration.
Root Cause
The regressions on this branch were caused by a mix of stale repository metadata, unstable client-side state/caching behavior, and outdated assumptions between the UI, backend contracts, and deployment/runtime setup.
More concretely:
User Impact
Validation
npm run checknpm run test:e2enpm --prefix server run typechecknpm --prefix server test -- --runInBand src/auth/routes.test.ts src/services/SystemSettingsService.test.ts src/config/index.test.ts src/review/llmClient.test.tsnpm --prefix web run lint -- app/dashboard/repositories/page.tsx app/dashboard/settings/page.tsx src/components/ui/Input.tsx app/dashboard/page.tsxnpm --prefix web run buildhttp://localhost:3000for dashboard/settings/repositories critical paths