支持编辑文件后自动创建 CL 并触发构建流程

[Chen-Rong-Zi]

Summary

• 编辑文件后不再直接生成 Commit
  当用户在 UI / API 中编辑文件并保存时：

  • 改为进入 CL 流程 (通过cl与构建系统的BuildContext类型转换中实现)
  • 禁止直接创建 Git commit (mega_cl::model 定义存在from_hash以及to_hash字段, 可能需要修改该结构体)

• 自动或显式新建 CL
  编辑文件后：

  • 若当前无 open CL → 自动创建一个新的 CL (会在force_create设置为否时尝试获取当前的cl)
  • 若已有 open CL → 可选地将变更追加到该 CL (新增force_create参数)
  • CL 需包含：修改的文件列表 (由from_hash以及to_hash对应的commit动态计算)
  • CL 需包含：基于当前基线（base branch）的 diff (由from_hash以及to_hash对应的commit动态计算)

• 基于 CL 触发构建
  CL 创建或更新后：

  • 按新的构建触发模型触发构建
  • 构建状态与结果绑定到 CL (cl -> task -> {builds})
  • 构建不再与“孤立 commit”绑定 (cl -> task -> {builds})
  • UI 可在 CL 维度展示构建状态 (cl -> task -> {builds})
    新增:
  • 新增skip_build参数, 抑制构建可选
    - [x] 构建前MonoApiService::should_trigger_build可根据changed_files动态判断是否构建, 目前总是返回字面常量true 相关代码已删除, 对应功能应该在新构建系统内实现
    feat: implement build trigger system #1880

related issue

#1872

[genedna]

@codex review

[genedna]

@claude

[github-actions]

Claude finished @genedna's task in 1m 35s —— View job

---

Code Review Complete ✓

I've completed a comprehensive review of PR #1877. Here are my findings:

---

Overview

This PR successfully implements automatic CL (Change List) creation and build triggering after file edits, replacing the previous direct commit approach. The implementation is generally well-structured with good separation of concerns.

---

Strengths

1. Architecture: Clean separation between the web edit handler, CL management, and build triggering
2. Comments: All code comments are properly in English (previous review concern addressed ✓)
3. Code Quality: The refactor from sort_by to sort_by_key in commit_ops.rs:457 is a good improvement
4. Extensibility: The new EditCLMode enum provides flexibility for CL reuse strategies

---

Issues & Recommendations

🔴 Critical Issues

1. Error Handling - Silent Failure in Background Task (mono_api_service.rs:1174-1180)

   tokio::spawn(async move {
       let _ = BuildTriggerService::build_by_context(
           storage,
           git_cache,
           bellatrix.into(),
           cl.into(),
       )
       .await;
   });

   Issue: The let _ = pattern silently discards build errors. Users won't know if their build failed to trigger.

   Recommendation: Add error logging at minimum:

   tokio::spawn(async move {
       if let Err(e) = BuildTriggerService::build_by_context(...).await {
           tracing::error!("Failed to trigger build for CL {}: {}", cl_link, e);
       }
   });

   Fix this →

2. Breaking API Change (model/git.rs:200)

   pub struct EditFilePayload {
       // ...
       pub mode: EditCLMode,  // ← Not optional!
   }

   Issue: Adding a required field breaks backward compatibility with existing API clients.

   Recommendation: Make it optional with a sensible default:

   #[serde(default = "default_edit_mode")]
   pub mode: EditCLMode,
   
   fn default_edit_mode() -> EditCLMode {
       EditCLMode::TryReuse(None)
   }

   Fix this →

🟡 Important Issues

3. Incomplete Error Context (mono_api_service.rs:1085-1090)

   let from_hash = self
       .storage
       .mono_storage()
       .get_main_ref(repo_path)
       .await?
       .ok_or_else(|| MegaError::Other("Main ref not found".to_string()))?

   Issue: Error message doesn't include which repo path failed, making debugging difficult.

   Recommendation:

   .ok_or_else(|| MegaError::Other(format!("Main ref not found for repo: {}", repo_path)))?

   Fix this →

4. Potential Path Parsing Issue (mono_api_service.rs:1139-1142)

   let repo_path = path
       .parent()
       .map(|p| p.to_string_lossy().to_string())
       .unwrap_or_else(|| "/".to_string());

   Issue: If file_path is just a filename (no path separators), parent() returns Some(""), not None. This could create an empty string repo path instead of "/".

   Recommendation: Add explicit empty string handling:

   let repo_path = path
       .parent()
       .and_then(|p| {
           let s = p.to_string_lossy().to_string();
           if s.is_empty() { None } else { Some(s) }
       })
       .unwrap_or_else(|| "/".to_string());

5. CL Update Race Condition (mono_api_service.rs:1155-1161)

   if let Some(existing_cl) = storage.get_open_cl_by_path(&repo_path, username).await... {
       storage.update_cl_to_hash(existing_cl.clone(), to_hash).await?;
       Ok(existing_cl)
   }

   Issue: Between fetching and updating, another request could modify the same CL, leading to inconsistent state.

   Recommendation: Implement optimistic locking or a transaction to ensure atomicity.

🟢 Minor Issues

6. Missing field documentation (model/git.rs:201)
   The mode field lacks a doc comment explaining when to use ForceCreate vs TryReuse.

7. Inconsistent naming (web_edit_handler.rs:32)
   The fallback CL link format "webedit-{timestamp}-{hash}" doesn't match other CL link generation patterns. Should use generate_link() consistently.

8. Test Coverage: No tests included for the new CL workflow. Consider adding:

   • Unit tests for find_or_create_cl_for_edit logic
   • Integration tests for the build trigger flow
   • Edge case tests (empty paths, concurrent edits, etc.)

---

Security Considerations

✅ Good: No SQL injection risks (using ORM)
✅ Good: No obvious authentication bypasses
⚠️ Consider: Add rate limiting for CL creation to prevent abuse

---

Performance Considerations

✅ Good: Background task spawning prevents blocking the edit response
⚠️ Consider: The get_builds_for_commit call in web_edit_handler.rs:34 could be expensive for large changes. Consider adding caching or limits.

---

Documentation

• Missing field documentation for new EditFilePayload.mode and EditFilePayload.skip_build
• The PR description checkboxes are well-maintained ✓
• Consider adding inline examples in the doc comments

---

Summary

The PR implements the required functionality correctly, but has 2 critical issues that should be addressed before merging:

1. Silent error handling in background build tasks
2. Breaking API change with required mode field

The code is otherwise well-structured with good English documentation. After addressing these issues, this will be a solid implementation of the CL workflow.

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c501d92f69

ℹ️ 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".

[chatgpt-codex-connector]

Resolve repo root before looking up main ref

Here repo_path is derived from the immediate parent directory of the edited file. For edits under nested subdirectories (e.g., /repo/src/file.rs), this becomes /repo/src, but get_main_ref is keyed by the repository root path (e.g., /repo). That makes create_new_cl fail with “Main ref not found” for typical edits and blocks CL creation/builds. The repo root should be resolved before calling get_main_ref rather than using the file’s parent directory.

Useful? React with 👍 / 👎.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

[Copilot]

The new CL creation/reuse flow for web edits (find_or_create_cl_for_edit, create_new_cl) and build triggering via trigger_build_for_cl introduce non-trivial behavior (different EditCLMode paths, skip_build flag, background build trigger) but there are no targeted tests covering these code paths, even though MonoApiService already has a test module. Please add tests that exercise at least ForceCreate, TryReuse (with/without link), and skip_build behavior to guard against regressions in the CL/build workflow.