feat: create file/folder by new cl by 231220075 · Pull Request #1949 · gitmono-dev/mega

231220075 · 2026-02-23T11:51:56Z

Restructure the process of creating new files/folders, update files through cl method, similar to editing files.

github-actions · 2026-02-23T11:52:12Z

Claude finished @231220075's task in 3m 43s —— View job

Code Review

Gather context (PR diff, changed files)
Review mono_api_service.rs (main changes)
Review import_api_service.rs and mod.rs
Review git.rs (new model additions)
Review preview_router.rs
Compile feedback and post review

Summary

This PR refactors create_monorepo_entry to follow the same CL (change-list) workflow as file editing: instead of immediately applying ref updates, it now creates a pending commit, associates it with a CL via find_or_create_cl_for_edit, and optionally triggers a build. The core tree-building logic is extracted into a new private helper prepare_create_entry_update. The return type is enriched from a bare String (commit id) to CreateEntryResult (commit id, blob oid, entry path, CL link).

The refactor is generally well-structured, but there are several correctness, robustness, and safety issues worth addressing.

Issues Found

1. Bug: `entry_path` is wrong when intermediate directories are created

File: ceres/src/api_service/mono_api_service.rs (~line 583)

When missing_parts is non-empty (intermediate directories must be created), repo_path is set to PathBuf::from(prefix) — the existing prefix, not the full target path. The returned entry_path is then computed as {prefix}/{entry_info.name}, omitting all the intermediate missing segments.

Example: entry_info.path = "a", entry_info.name = "newfile.rs", missing_parts = ["b", "c"], prefix = "".

Actual path created: /b/c/newfile.rs
Returned entry_path: /newfile.rs ❌

The correct path should always be entry_info.path + "/" + entry_info.name (with root normalization):

let entry_path = if entry_info.path.is_empty() || entry_info.path == "/" {
    format!("/{}", entry_info.name)
} else {
    format!("{}/{}", entry_info.path.trim_end_matches('/'), entry_info.name)
};

Fix this →

2. Potential panic: `content.clone().unwrap()` without prior validation

File: ceres/src/api_service/mono_api_service.rs (in prepare_create_entry_update)

In both the missing_parts.is_empty() and else branches, file creation calls:

let blob = Blob::from_content(&entry_info.content.clone().unwrap());

If a caller sends is_directory: false but omits content, this panics. This should be validated early and return a proper error:

let content = entry_info.content.as_deref()
    .ok_or_else(|| GitError::CustomError("content is required for file creation".into()))?;
let blob = Blob::from_content(content);

Fix this →

3. Potential panic: `ObjectHash::from_str(...).unwrap()`

File: ceres/src/api_service/mono_api_service.rs (~line 520)

let base_commit = ObjectHash::from_str(&src_commit.id.to_string()).unwrap();

If src_commit.id ever fails to parse as an ObjectHash (e.g. corrupt data), this panics. Use map_err instead:

let base_commit = ObjectHash::from_str(&src_commit.id.to_string())
    .map_err(|e| GitError::CustomError(format!("Invalid commit hash: {e}")))?;

4. Potential panic: `update_chain.pop().unwrap()` and `missing_parts.first().unwrap()`

File: ceres/src/api_service/mono_api_service.rs (in prepare_create_entry_update)

update_chain.pop().unwrap() — if the search returns an empty chain (e.g. at root with no tree), this panics.
missing_parts.first().unwrap() — although this code is inside an else that guarantees !missing_parts.is_empty(), prefer .expect("guaranteed non-empty by branch condition") to make the invariant explicit and aid future maintainers.

5. Partial state on failure: commit saved before blobs

File: ceres/src/api_service/mono_api_service.rs (in create_monorepo_entry)

The current save order is:

save_mega_commits — persists the commit object
batch_save_model (trees)
save_blobs

If step 3 fails, the commit and trees exist in storage but the blobs they reference do not. This leaves the repository in an inconsistent state. Consider saving blobs before the commit, or wrapping all three operations in a transaction. At minimum, the error message from a blob-save failure should indicate that a rollback may be needed.

6. Regex compiled on every invocation (performance)

File: ceres/src/api_service/mono_api_service.rs (in prepare_create_entry_update)

let re: Regex = Regex::new(r"Path '([^']+)' not exist").unwrap();

This regex is recompiled on every call. Use once_cell::sync::Lazy (already a common pattern in Rust) or std::sync::LazyLock:

static PATH_NOT_EXIST_RE: Lazy<Regex> =
    Lazy::new(|| Regex::new(r"Path '([^']+)' not exist").unwrap());

7. Brittle error-message-based path parsing (design concern)

File: ceres/src/api_service/mono_api_service.rs (in prepare_create_entry_update)

The strategy of calling search_tree_for_update, letting it fail, then regex-parsing the error string to identify missing path segments is fragile. If the error format ever changes, the regex silently produces an empty missing_parts, causing update_chain.pop().unwrap() to panic or incorrect behavior.

This was already present in the old code and simply moved into the helper, but since you're refactoring this section it would be worth creating a dedicated error variant (e.g. GitError::PathNotFound(String)) in search_tree_for_update to carry the missing-path information structurally.

8. Commit message has a leading newline

File: ceres/src/model/git.rs (line 34–38)

format!("\n create new directory {}", self.name)
format!("\n create new file {}", self.name)

The leading \n is unusual. Git commit messages typically start with a subject line on the first line. This will display oddly in git log output.

9. Missing tests for new code paths

No tests were added for prepare_create_entry_update or the updated create_monorepo_entry. The existing test module (mod test at line 3163) contains unit tests for build_result_by_chain and other MonoServiceLogic helpers — the new prepare_create_entry_update logic for both branches (existing path / missing path, file / directory) would be good candidates for similar unit tests.

Minor Observations

mod.rs / import_api_service.rs: The trait signature change and ImportApiService stub update are clean. The removal of the return keyword in the stub is a good Rust idiom fix.
preview_router.rs: Returning CommonResult::success(Some(result)) (instead of None) is the correct fix now that the response body carries meaningful data.
CreateEntryResult.cl_link: Typed as Option<String>, but the current implementation always sets it to Some(cl.link). Unless future callers may skip CL creation, this could be a plain String.
mode field on CreateEntryInfo: The field lacks a doc comment. Adding one (similar to skip_build) would help API consumers understand when to use EditCLMode::ForceNew vs TryReuse.

Overall Assessment

The architectural direction is sound — aligning file/folder creation with the CL workflow avoids divergence. The main concerns are the incorrect entry_path return value (correctness bug), the unwrap() panics on untrusted inputs (reliability), and the save-ordering issue (data integrity). The brittle error-message parsing and missing tests are lower-priority but worth addressing before merging.

github-actions · 2026-02-23T12:41:05Z

Claude finished @231220075's task in 2m 24s —— View job

Code Review (Updated — Post-Synchronize)

This review reflects the current state of the branch after the latest push. It checks which issues from the previous review were addressed and identifies any remaining concerns.

Previous Issues — Status

#	Issue	Status
1	`entry_path` computed from `prefix` instead of full target path	✅ Fixed
2	`content.clone().unwrap()` panic on missing file content	✅ Fixed
3	`ObjectHash::from_str(...).unwrap()` panic on corrupt data	✅ Fixed
4	`update_chain.pop().unwrap()` and `missing_parts.first().unwrap()` panics	✅ Fixed
5	Partial state on failure: incorrect save ordering	⚠️ Partially fixed
6	Regex recompiled on every call	✅ Fixed
7	Brittle error-message-based path parsing	⚠️ Still present (design concern)
8	Leading `\n` in commit messages	✅ Fixed
9	Missing tests for new code paths	⚠️ Still absent

Remaining Issues

1. Save order still inconsistent: `blobs → commit → trees` (medium priority)

File: ceres/src/api_service/mono_api_service.rs (lines 547–571)

Blobs are now saved before the commit — an improvement over the previous order — but the current sequence is:

save_blobs (line 547)
save_mega_commits (line 552)
batch_save_model — trees (line 568)

A commit object contains the tree ID (Commit::from_tree_id(...)). If step 3 fails, the commit stored in step 2 references a tree that does not yet exist in the database, leaving the repository in an inconsistent state.

The correct bottom-up order (matching Git's own object model) is:

blobs → trees → commit

Fix this →

2. `new_oid` is the `.gitkeep` blob ID when creating a directory (minor)

File: ceres/src/api_service/mono_api_service.rs (line 545)

let new_oid = blob.id.to_string();

For directory creation, blob is the internally generated .gitkeep blob. The returned CreateEntryResult.new_oid is therefore the blob ID of .gitkeep, not the tree ID of the new directory. API consumers that interpret new_oid as the OID of the created entry will get a misleading value. Consider returning the new directory's tree ID instead when entry_info.is_directory is true.

3. Duplicated `upsert_binding` pattern in `create_entry` handler (minor)

File: mono/src/api/router/preview_router.rs (lines 82–96 and 383–397)

The anonymous-username normalization + upsert_binding call is copy-pasted verbatim between create_entry and save_edit. This is a minor duplication that could be extracted into a shared helper once the code stabilises, but it is not a blocker.

4. Missing tests (low priority — carry-over from previous review)

No unit tests were added for prepare_create_entry_update (either branch: existing path / missing path, file / directory) or build_entry_path. The MonoServiceLogic test module structure is already in place and both helpers are good candidates for targeted unit tests.

Summary

The critical correctness bug (entry_path) and all unwrap-based panics from the previous review have been addressed cleanly. The regex is now a LazyLock static, and commit messages no longer carry the leading newline.

The one remaining medium-priority issue is the save order (blobs → commit → trees should be blobs → trees → commit). The rest are minor or low-priority.

Signed-off-by: Ruizhi Huang <231220075@smail.nju.edu.cn> fix entry_path Signed-off-by: Ruizhi Huang <231220075@smail.nju.edu.cn> fix save order Signed-off-by: Ruizhi Huang <231220075@smail.nju.edu.cn>

github-actions · 2026-02-23T12:59:00Z

Claude finished @231220075's task in 2m 36s —— View job

Code Review (Updated — Post-Synchronize)

Gather context (PR diff, changed files)
Review mono_api_service.rs (main changes)
Review supporting files
Check previous review issues status
Compile feedback and post review

Previous Issues — Status

#	Issue	Status
1 (from review 2)	Save order `blobs → commit → trees`	✅ Fixed — now `blobs → trees → commit`
2 (from review 2)	`new_oid` was `.gitkeep` blob ID for directories	✅ Fixed — `entry_oid = new_dir_tree.id` (tree ID)
3 (from review 2)	Duplicated `upsert_binding` pattern	⚠️ Still present (minor, non-blocking)
4 (from review 2)	Missing tests	⚠️ Still absent
7 (from review 1)	Brittle error-message path parsing	⚠️ Still present (design concern)

Remaining Issues

1. Remaining `unwrap()` calls on `Tree::from_tree_items` (low priority)

File: ceres/src/api_service/mono_api_service.rs (lines 1042, 1072, 1111, 1139, 1154, 1183)

Six .unwrap() calls remain on Tree::from_tree_items(...):

let new_dir_tree = Tree::from_tree_items(vec![tree_item]).unwrap();  // line 1042
let target_tree = Tree::from_tree_items(target_items).unwrap();      // line 1072
// ... and 4 more in the else branch

These are low-risk since the inputs are constructed internally (not from user data), but they will panic if from_tree_items can ever fail with valid inputs. Using .expect("invariant: ...") would at minimum make the assumption explicit.

2. Duplicate check is mode-specific — cross-type name conflicts not caught (low priority)

File: ceres/src/api_service/mono_api_service.rs (lines 1017–1027)

In the missing_parts.is_empty() branch:

let is_tree_mode = if entry_info.is_directory { TreeItemMode::Tree } else { TreeItemMode::Blob };
if target_items.iter().any(|x| x.mode == is_tree_mode && x.name == entry_info.name) {
    return Err(GitError::CustomError("Duplicate name".to_string()));
}

This only checks for name collisions within the same mode. Creating a file named foo would not be flagged as a duplicate if a directory named foo already exists (and vice versa). Git trees do not allow two entries with the same name regardless of type, so this check should compare by name alone:

if target_items.iter().any(|x| x.name == entry_info.name) {
    return Err(GitError::CustomError("Duplicate name".to_string()));
}

The same applies in the else branch at line 1174.

Fix this →

3. `prefix` / `path` duplicated in `repo_path` computation (minor)

File: ceres/src/api_service/mono_api_service.rs (lines 1079–1092)

The path expression if prefix.is_empty() { path.clone() } else { PathBuf::from(prefix) } is computed twice — once as the first argument to build_result_by_chain and once to assign repo_path. This is harmless but could be a single let:

let chain_path = if prefix.is_empty() { path.clone() } else { PathBuf::from(prefix) };
let update_result = MonoServiceLogic::build_result_by_chain(chain_path.clone(), update_chain, target_tree.id)?;
let repo_path = chain_path;

Minor Observations

create_entry handler clones json unnecessarily: Line 99 in preview_router.rs calls json.clone() because create_monorepo_entry takes ownership, but json.author_username is still needed afterwards. This is correct but the clone of the entire struct could be avoided by extracting author_username first.
cl_link: Option<String> is always Some: CreateEntryResult.cl_link is typed Option<String> but create_monorepo_entry always sets it to Some(cl.link). If future callers won't skip CL creation, this could be a plain String. (Carry-over from review 1.)
mode field on CreateEntryInfo still lacks a doc comment: A brief note explaining ForceCreate vs TryReuse would help API consumers. (Carry-over from review 1.)

Summary

All critical correctness bugs and panics from previous reviews have been addressed. The save order is now correctly blobs → trees → commit. The new_oid for directory creation now returns the tree ID rather than the .gitkeep blob ID.

The two remaining actionable items are the cross-type duplicate name check (a real correctness gap) and the residual .unwrap() calls on Tree::from_tree_items. Missing tests and the brittle error-message path parsing continue to be the lower-priority carry-overs.

Copilot

Pull request overview

This pull request restructures the file and folder creation process to align with the Change List (CL) workflow, making it consistent with the file editing flow. The main change is transitioning from directly applying updates to the main branch to creating commits through the CL system, which enables code review, build validation, and approval workflows for newly created files and directories.

Changes:

Changed API response from returning just a commit ID string to a structured CreateEntryResult with commit ID, blob OID, file path, and CL link
Added skip_build and mode fields to CreateEntryInfo to control build triggering and CL reuse behavior
Extracted duplicate commit binding logic into a reusable upsert_commit_binding helper function
Refactored create_monorepo_entry to use the CL workflow pattern (similar to save_file_edit) instead of directly applying updates to refs

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
mono/src/api/router/preview_router.rs	Extracted `upsert_commit_binding` helper and updated `create_entry` endpoint to return structured `CreateEntryResult`
ceres/src/model/git.rs	Added `skip_build` and `mode` fields to `CreateEntryInfo`; added `CreateEntryResult` struct; removed leading newline from commit messages
ceres/src/api_service/mono_api_service.rs	Refactored `create_monorepo_entry` to use CL workflow; extracted logic into `prepare_create_entry_update` helper; improved error handling with LazyLock regex and better validation
ceres/src/api_service/mod.rs	Updated trait signature to return `CreateEntryResult` instead of `String`
ceres/src/api_service/import_api_service.rs	Updated stub implementation to match new trait signature

Copilot · 2026-02-23T13:59:09Z

    request_body = CreateEntryInfo,
    responses(
-        (status = 200, body = CommonResult<String>, content_type = "application/json")
+        (status = 200, body = CommonResult<CreateEntryResult>, content_type = "application/json")


The API response type has been changed from CommonResult<String> to CommonResult<CreateEntryResult>, but the frontend TypeScript types in moon/packages/types/generated.ts still expect CommonResultString. The frontend types need to be regenerated from the updated OpenAPI specs to include the new CreateEntryResult type and update the PostApiCreateEntryData type mapping. Without this, frontend code will have type mismatches and may break at runtime.

Copilot · 2026-02-23T13:59:09Z

+    /// if true, skip build
+    #[serde(default)]
+    pub skip_build: bool,
+    /// Controls how CL is created or reused for this change.
+    #[serde(default = "default_create_mode")]
+    pub mode: EditCLMode,


The CreateEntryInfo struct now includes skip_build and mode fields, but the frontend TypeScript types in moon/packages/types/generated.ts (lines 4080-4091) don't include these new fields. The frontend types need to be regenerated to match the updated backend schema. This could cause issues when frontend code tries to send these new optional fields.

Copilot · 2026-02-23T13:59:10Z

+            format!("create new directory {}", self.name)
        } else {
-            format!("\n create new file {}", self.name)
+            format!("create new file {}", self.name)


The commit message format has changed from having a leading newline (e.g., "\n create new directory foo") to not having one ("create new directory foo"). Verify this change is intentional and won't affect any commit message parsing or formatting elsewhere in the codebase. Consider if this affects commit history readability or any automated tooling that processes commit messages.

231220075 force-pushed the pr_branch branch from 155baed to 9be24a4 Compare February 23, 2026 12:40

feat: create file/folder by new cl

ed6b4b1

Signed-off-by: Ruizhi Huang <231220075@smail.nju.edu.cn> fix entry_path Signed-off-by: Ruizhi Huang <231220075@smail.nju.edu.cn> fix save order Signed-off-by: Ruizhi Huang <231220075@smail.nju.edu.cn>

231220075 force-pushed the pr_branch branch from 9be24a4 to ed6b4b1 Compare February 23, 2026 12:58

genedna requested review from benjamin-747 and Copilot February 23, 2026 13:53

Copilot started reviewing on behalf of genedna February 23, 2026 13:54 View session

Copilot AI reviewed Feb 23, 2026

View reviewed changes

benjamin-747 approved these changes Feb 24, 2026

View reviewed changes

benjamin-747 added this pull request to the merge queue Feb 24, 2026

Merged via the queue into gitmono-dev:main with commit 81703df Feb 24, 2026
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: create file/folder by new cl#1949

feat: create file/folder by new cl#1949
benjamin-747 merged 1 commit into
gitmono-dev:mainfrom
231220075:pr_branch

231220075 commented Feb 23, 2026

Uh oh!

github-actions Bot commented Feb 23, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Feb 23, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Feb 23, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 23, 2026

Uh oh!

Copilot AI Feb 23, 2026

Uh oh!

Copilot AI Feb 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

231220075 commented Feb 23, 2026

Uh oh!

github-actions Bot commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review

Summary

Issues Found

1. Bug: entry_path is wrong when intermediate directories are created

2. Potential panic: content.clone().unwrap() without prior validation

3. Potential panic: ObjectHash::from_str(...).unwrap()

4. Potential panic: update_chain.pop().unwrap() and missing_parts.first().unwrap()

5. Partial state on failure: commit saved before blobs

6. Regex compiled on every invocation (performance)

7. Brittle error-message-based path parsing (design concern)

8. Commit message has a leading newline

9. Missing tests for new code paths

Minor Observations

Overall Assessment

Uh oh!

github-actions Bot commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review (Updated — Post-Synchronize)

Previous Issues — Status

Remaining Issues

1. Save order still inconsistent: blobs → commit → trees (medium priority)

2. new_oid is the .gitkeep blob ID when creating a directory (minor)

3. Duplicated upsert_binding pattern in create_entry handler (minor)

4. Missing tests (low priority — carry-over from previous review)

Summary

Uh oh!

github-actions Bot commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review (Updated — Post-Synchronize)

Previous Issues — Status

Remaining Issues

1. Remaining unwrap() calls on Tree::from_tree_items (low priority)

2. Duplicate check is mode-specific — cross-type name conflicts not caught (low priority)

3. prefix / path duplicated in repo_path computation (minor)

Minor Observations

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Feb 23, 2026 •

edited

Loading

1. Bug: `entry_path` is wrong when intermediate directories are created

2. Potential panic: `content.clone().unwrap()` without prior validation

3. Potential panic: `ObjectHash::from_str(...).unwrap()`

4. Potential panic: `update_chain.pop().unwrap()` and `missing_parts.first().unwrap()`

github-actions Bot commented Feb 23, 2026 •

edited

Loading

1. Save order still inconsistent: `blobs → commit → trees` (medium priority)

2. `new_oid` is the `.gitkeep` blob ID when creating a directory (minor)

3. Duplicated `upsert_binding` pattern in `create_entry` handler (minor)

github-actions Bot commented Feb 23, 2026 •

edited

Loading

1. Remaining `unwrap()` calls on `Tree::from_tree_items` (low priority)

3. `prefix` / `path` duplicated in `repo_path` computation (minor)