refactor: refactor commit-info API and GitHub history inconsistency

[AllureCurtain]

link #1653

• 单一历史遍历内核：traverse_commit_history_for_last_modification
• 统一 refs 解析：resolve_start_commit 被所有 API 复用
• 代码复用率 100%：所有 API 通过 history 模块统一调用

[Copilot]

Pull request overview

This PR refactors the commit history API to use a unified, consistent implementation across all code paths. The refactoring introduces a single traversal kernel for finding last modifications, consolidates refs resolution logic, and eliminates code duplication.

Key Changes

• Unified traversal kernel: All history lookups now use traverse_commit_history_for_last_modification which implements BFS with TREESAME pruning to match GitHub's behavior
• Consolidated refs resolution: New resolve_start_commit function handles tags, commit SHAs, and HEAD consistently
• Simplified API: Merged get_latest_commit and get_latest_commit_with_refs into a single method with an optional refs parameter
• Enhanced error handling: Added [code:xxx] prefix parsing in error handling to automatically set appropriate HTTP status codes

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
mono/src/api/router/preview_router.rs	Simplified get_latest_commit to use unified API with optional refs parameter
mono/src/api/error.rs	Added automatic HTTP status code parsing from error message prefixes like [code:404]
ceres/src/api_service/mono_api_service.rs	Removed duplicate methods and DB-based logic, delegating to unified history module
ceres/src/api_service/mod.rs	Added resolve_start_commit for unified ref resolution and updated trait signature
ceres/src/api_service/history.rs	Refactored traversal algorithm to use BFS with proper merge commit handling
ceres/src/api_service/commit_ops.rs	Updated to use unified traversal logic for both directories and files

[Copilot]

[nitpick] Potential logic issue: When the item does not exist at start_commit (line 274), the function returns the start_commit itself. Similar to the path check above, this could be misleading. If the item doesn't exist at the starting point, it may be more appropriate to return an error indicating the item was not found, rather than implying the start_commit is the last modification.

Suggested change

	return Ok((*start_commit).clone());
	return Err(GitError::NotFound(format!(
	"Item '{}' does not exist at path '{}' in commit {}",
	search_item.name,
	path.display(),
	start_commit.id
	)));

[Copilot]

[nitpick] Potential infinite loop or correctness issue: The BFS traversal only adds parents to the queue if they haven't been visited yet (line 333), but there's a scenario where all queued commits could be exhausted without finding a modification. In such cases, the function returns start_commit at line 343. However, if the traversal logic is correct and all parents have matching hashes all the way back to the root, this fallback might never be reached in practice. Consider adding a tracing log or comment explaining when this fallback case would occur, to clarify the expected behavior.

[Copilot]

Potential index out of bounds: The code extracts the status code by slicing &err_str[6..code_end] (line 73), assuming the string starts with "[code:" (6 characters). However, the filter condition only checks that the string starts with "[code:" and that a ] exists, but doesn't verify that code_end is actually greater than 6. If code_end is less than 6 (e.g., malformed string like "[code]"), this would cause a panic. While unlikely in practice, it's safer to add a check: code_end > 6 before slicing, or use safer extraction methods.

Suggested change

	if let Some(code_end) = err_str.find(']').filter(|_| err_str.starts_with("[code:")) {
	if let Some(code_end) = err_str.find(']').filter(|&idx| err_str.starts_with("[code:") && idx >= 6) {

[Copilot]

Similar potential index out of bounds: The code extracts the status code by slicing &err_str[6..code_end] (line 47), but only checks that err_msg.starts_with("[code:") and that ] exists. If code_end is less than 6 (e.g., malformed "[code]"), this would panic. Add validation that code_end > 6 before slicing to prevent potential runtime errors.

Suggested change

	if let Some(code_end) = err_msg.find(']').filter(|_| err_msg.starts_with("[code:")) {
	if let Some(code_end) = err_msg.find(']').filter(|&idx| err_msg.starts_with("[code:") && idx >= 6) {

[Copilot]

Documentation update needed: The comment on line 195 in the original trait definition mentioned "Default implementation: fallback to the version without refs", but this default implementation has been removed. The unified get_latest_commit method now requires the refs parameter. Any documentation or comments that refer to the old two-method pattern (get_latest_commit and get_latest_commit_with_refs) should be updated to reflect this consolidation.

[Copilot]

[nitpick] Potential logic issue: When the path does not exist at start_commit (line 264), the function returns the start_commit itself as the "last modification" commit. This might be misleading, as it suggests that this commit modified the non-existent path. Consider returning an error with a descriptive message like "[code:404] Path not found" instead, to provide clearer feedback that the path doesn't exist at the given reference.

Suggested change

	// Path does not exist at start_commit; return start_commit as a safe default
	return Ok((*start_commit).clone());
	// Path does not exist at start_commit; return an error
	return Err(GitError::NotFound(format!(
	"[code:404] Path not found: {:?} at commit {}",
	path, start_commit.id
	)));

[Copilot]

Error handling inconsistency: The error pattern matching changed from Some(commit) and None (old code) to Ok(commit) and Err(_) (new code). However, the Err(_) case discards all error information with the wildcard pattern. This means different errors from resolve_last_modification_by_path (e.g., "Path has no parent directory", "Item not found in directory", etc.) all get replaced with the generic "[code:404] File not found" message. Consider preserving the specific error message or at least logging it before replacing it with a generic 404, so that debugging and troubleshooting are easier.

[Copilot]

[nitpick] Missing validation: The function accepts refs as Option<&str> and passes it to resolve_start_commit, which validates commit SHAs by checking if they are exactly 40 characters and all hexadecimal. However, shorter commit SHA prefixes (7-39 characters) are commonly used in Git and may be valid in your system. If your system supports short SHAs, this validation will incorrectly reject them. Consider either supporting short SHAs or documenting that only full 40-character SHAs are accepted.

Suggested change

	// Try to resolve as commit SHA (must be 40-character hex)
	if ref_str.len() == 40 && ref_str.chars().all(|c| c.is_ascii_hexdigit()) {
	let commit = handler.get_commit_by_hash(ref_str).await;
	return Ok(Arc::new(commit));
	}
	// Failed to resolve: return descriptive error
	Err(GitError::CustomError(format!(
	"Invalid reference '{}': not a valid tag name or commit SHA",
	// Try to resolve as commit SHA (allow short SHA: 7-40 hex digits)
	if (7..=40).contains(&ref_str.len()) && ref_str.chars().all(|c| c.is_ascii_hexdigit()) {
	// Try to resolve the commit by (possibly short) hash
	let commit = handler.get_commit_by_hash(ref_str).await;
	// If get_commit_by_hash returns a sentinel/empty commit for not found, check for that here
	// (Assume get_commit_by_hash returns a valid commit or panics/errors otherwise)
	// If ambiguous or not found, get_commit_by_hash should return an error or a special value
	// If not found, return a descriptive error
	// For now, we assume get_commit_by_hash returns a valid commit or panics/errors otherwise
	return Ok(Arc::new(commit));
	}
	// Failed to resolve: return descriptive error
	Err(GitError::CustomError(format!(
	"Invalid reference '{}': not a valid tag name or commit SHA (must be a tag name or a 7-40 digit hex commit hash)",

[Copilot]

Edge case: Root directory handling may fail. If the path is the root directory (e.g., "/" or "."), then path.parent() will return Some("") or None, and path.file_name() might not return a meaningful directory name. The code at lines 35-43 extracts the parent and directory name without checking if this is the root directory. This could lead to incorrect behavior or errors when querying the latest commit for the repository root. Consider adding special handling for the root directory case.

Suggested change

	// Treat directory as a TreeItem and find its last modification
	let dir_name = path
	.file_name()
	.and_then(|n| n.to_str())
	.ok_or_else(|| GitError::CustomError("Invalid directory path".to_string()))?
	.to_string();
	let parent = path
	.parent()
	.ok_or_else(|| GitError::CustomError("Directory has no parent".to_string()))?;
	// Special handling for root directory
	let (dir_name, parent): (String, &std::path::Path) = if path.as_os_str().is_empty()
	|| path == std::path::Path::new(".")
	|| path == std::path::Path::new("/")
	{
	// For root, use "" as name and parent as root itself
	(String::from(""), std::path::Path::new(""))
	} else {
	let dir_name = path
	.file_name()
	.and_then(|n| n.to_str())
	.ok_or_else(|| GitError::CustomError("Invalid directory path".to_string()))?
	.to_string();
	let parent = path
	.parent()
	.ok_or_else(|| GitError::CustomError("Directory has no parent".to_string()))?;
	(dir_name, parent)
	};

[Copilot]

Missing documentation: The documentation comment for traverse_commit_history_for_last_modification ends abruptly on line 243 without completing the sentence "This matches the 'last modification' semantics used". This incomplete sentence should be completed or removed to ensure clear documentation.

Suggested change

	/// exist in the parent). This matches the "last modification" semantics used
	/// exist in the parent). This matches the "last modification" semantics used by tools like `git log -L` or `git blame`, which identify the most recent commit where a file or directory was changed.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

[Copilot]

[nitpick] Inconsistent error handling pattern: The error handling pattern here is convoluted with nested pattern matching. The code checks if the error is CustomError with [code:404] prefix, preserves it, and otherwise wraps it with a generic 404 message. However, this loses the original error context when wrapping.

Consider simplifying this to:

Err(e) => {
    tracing::debug!("File not found or error during traversal: {:?}", e);
    match e {
        GitError::CustomError(ref msg) if msg.starts_with("[code:404]") => Err(e),
        _ => Err(GitError::CustomError("[code:404] File not found".to_string()))
    }
}

This avoids the let-if pattern and makes the logic clearer.

Suggested change

	// Preserve the original error message for better debugging
	tracing::debug!("File not found or error during traversal: {:?}", e);
	// If it's already a CustomError with [code:404], preserve it
	if let GitError::CustomError(msg) = &e
	&& msg.starts_with("[code:404]")
	{
	return Err(e);
	}
	// Otherwise wrap with 404 code
	Err(GitError::CustomError(
	"[code:404] File not found".to_string(),
	))
	tracing::debug!("File not found or error during traversal: {:?}", e);
	match e {
	GitError::CustomError(ref msg) if msg.starts_with("[code:404]") => Err(e),
	_ => Err(GitError::CustomError("[code:404] File not found".to_string())),
	}

[Copilot]

Potential security issue: Unbounded error message exposure: The error handling code extracts and returns the full error message from anyhow::Error directly to the API response. If internal errors contain sensitive information (e.g., file paths, database connection strings, internal system details), this could leak sensitive information to API consumers.

Consider sanitizing error messages before exposing them to API responses, especially for internal server errors. Only expose safe, user-friendly messages to the client.

Suggested change

	let body = Json(CommonResult::<()> {
	req_result: false,
	data: None,
	err_message: err_msg,
	// Only expose detailed error messages for 4xx (client) errors.
	let client_error = self.status.is_client_error();
	let response_msg = if client_error {
	err_msg.clone()
	} else {
	"Internal server error".to_string()
	};
	let body = Json(CommonResult::<()> {
	req_result: false,
	data: None,
	err_message: response_msg,

[Copilot]

Potential bug with root directory handling: When handling the root directory (/, ., or empty path), the code creates a TreeItem with an empty string as the name (String::from("")). This item is then used in traverse_commit_history_for_last_modification, which searches for an item with that empty name in the tree items.

However, tree items typically don't have empty names - the root tree itself isn't contained as an item within its parent. This logic appears incorrect and may cause the traversal to fail with a 404 error when querying the root directory's last commit.

Consider either:

1. Special-casing root directory to return the start_commit directly without traversal
2. Using the root tree's hash instead of trying to find it as an item in a non-existent parent

Suggested change

	let (dir_name, parent): (String, &std::path::Path) = if path.as_os_str().is_empty()
	|| path == std::path::Path::new(".")
	|| path == std::path::Path::new("/")
	{
	// For root directory, treat it as the tree itself with empty parent
	// This commit represents the root tree's last modification
	(String::from(""), std::path::Path::new(""))
	} else {
	// For non-root directories, extract name and parent normally
	let dir_name = path
	.file_name()
	.and_then(|n| n.to_str())
	.ok_or_else(|| GitError::CustomError("Invalid directory path".to_string()))?
	.to_string();
	let parent = path
	.parent()
	.ok_or_else(|| GitError::CustomError("Directory has no parent".to_string()))?;
	(dir_name, parent)
	};
	if path.as_os_str().is_empty()
	|| path == std::path::Path::new(".")
	|| path == std::path::Path::new("/")
	{
	// For root directory, the commit that created the tree is the last modification.
	let mut commit_info: LatestCommitInfo = start_commit.clone().into();
	// If commit has a username binding, prefer showing that username
	if let Ok(Some(binding)) = handler
	.build_commit_binding_info(&start_commit.id.to_string())
	.await
	&& !binding.is_anonymous
	&& binding.matched_username.is_some()
	{
	let username = binding.matched_username.unwrap();
	commit_info.author = username.clone();
	commit_info.committer = username;
	}
	return Ok(commit_info);
	}
	// For non-root directories, extract name and parent normally
	let dir_name = path
	.file_name()
	.and_then(|n| n.to_str())
	.ok_or_else(|| GitError::CustomError("Invalid directory path".to_string()))?
	.to_string();
	let parent = path
	.parent()
	.ok_or_else(|| GitError::CustomError("Directory has no parent".to_string()))?;

[Copilot]

Potential string index panic: The code extracts a substring using err_msg[code_end + 1..] without checking if code_end + 1 is within bounds. If the error message ends with ] (e.g., "[code:404]"), then code_end + 1 would be equal to the string length, which is valid for slicing. However, if there are Unicode characters or the string is malformed, this could still panic.

Consider using .get(code_end + 1..) with proper error handling instead of direct indexing to avoid potential panics.

Suggested change

	err_msg = err_msg[code_end + 1..].trim().to_string();
	err_msg = err_msg.get(code_end + 1..)
	.map(|s| s.trim().to_string())
	.unwrap_or_else(|| err_msg.clone());

[Copilot]

Missing error handling for get_commit_by_hash: When resolving a commit SHA, the code calls handler.get_commit_by_hash(ref_str).await without checking if the commit exists. If the SHA is valid format but doesn't exist in the repository, this could lead to unclear error messages or panics depending on the implementation of get_commit_by_hash.

Consider wrapping this call with proper error handling to provide a clear error message when the commit SHA doesn't exist.

Suggested change

	let commit = handler.get_commit_by_hash(ref_str).await;
	return Ok(Arc::new(commit));
	match handler.get_commit_by_hash(ref_str).await {
	Ok(commit) => return Ok(Arc::new(commit)),
	Err(_) => {
	return Err(GitError::CustomError(format!(
	"Commit with SHA '{}' does not exist in the repository",
	ref_str
	)));
	}
	}

[Copilot]

Code duplication: Error code parsing logic is repeated: The logic to parse [code:xxx] format from error messages is duplicated between the into_response method (lines 46-51) and the From<E> implementation (lines 75-89). This creates a maintenance burden and increases the risk of inconsistencies.

Consider extracting this logic into a private helper function like:

fn parse_error_code(err_str: &str) -> Option<(&str, &str)> {
    if let Some(code_end) = err_str.find(']')
        .filter(|&idx| err_str.starts_with("[code:") && idx >= 6)
    {
        let code = &err_str[6..code_end];
        let msg = err_str.get(code_end + 1..)?.trim();
        Some((code, msg))
    } else {
        None
    }
}

[Copilot]

Potential infinite loop in BFS traversal with cycles: While the visited set prevents revisiting the same commit by ID, the queue can grow unbounded if there are many commits in the history. In a repository with complex branching and merge patterns, this could lead to memory exhaustion. Consider adding a maximum depth limit or iteration count to prevent potential DoS scenarios where an attacker could craft a repository with a very deep or complex history.

For example, add a safety limit similar to the old implementation's MAX_STEPS (which was 10,000):

const MAX_ITERATIONS: usize = 10_000;
let mut iterations = 0;
while let Some((commit, commit_hash)) = commit_queue.pop_front() {
    iterations += 1;
    if iterations > MAX_ITERATIONS {
        tracing::warn!("Exceeded maximum iterations in traverse_commit_history_for_last_modification");
        return Ok((*start_commit).clone());
    }
    // ... rest of logic
}