feat: 新增 open 指令并增加相应的单元测试和集成测试#169
Conversation
Signed-off-by: Tyooughtul <yooughtul@126.com>
Signed-off-by: Tyooughtul <yooughtul@126.com>
|
@codex review |
|
Claude finished @genedna's task in 2m 2s —— View job Code Review Complete ✓
SummaryThis PR successfully implements the 🔴 Critical Issues (Must Fix)1. Command Injection Vulnerabilities (src/command/open.rs:45-56)The URL is passed directly to system commands without proper escaping, creating a command injection risk. A malicious remote URL could execute arbitrary commands on the user's system. Priority: CRITICAL - This is a severe security vulnerability that could allow arbitrary code execution. 🟡 High Priority Issues2. Silent Error Handling (src/command/open.rs:45-56)All platform-specific commands use 3. Regex Compilation in Hot Path (src/command/open.rs:64, 74)Using 🟢 Minor Issues4. Code Style (src/command/open.rs:68, 78)String formatting can be simplified using format! macro directly. 5. Test Coverage (tests/command/open_test.rs:12-34)Tests don't cover error cases like nonexistent remotes or missing remote configuration. ✅ What's Good
RecommendationsBefore merging:
Nice to have: Detailed Inline CommentsI've added 9 inline comments on specific lines with detailed explanations and code examples. Please review each one. Most critical: See inline comments on lines 45-56 of src/command/open.rs for the security vulnerabilities. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/command/open.rs
Outdated
|
|
||
| #[cfg(target_os = "windows")] | ||
| { | ||
| Command::new("cmd").args(["/C", "start", &url]).spawn().ok(); |
There was a problem hiding this comment.
Security concern: Command injection vulnerability
The URL is passed directly to the system shell without proper escaping. A malicious remote URL could potentially execute arbitrary commands.
Example attack vector:
remote.origin.url = "https://example.com; rm -rf /"
Recommendation:
Either:
- Validate and sanitize the URL before passing it to the shell command
- Use proper argument escaping/quoting
- Validate URL format with regex before execution
Example fix:
// Validate URL format
if !url.starts_with("http://") && !url.starts_with("https://") {
eprintln!("fatal: Invalid URL format");
return;
}
// On Windows, use proper argument escaping
Command::new("cmd")
.args(["/C", "start", ""]) // Empty string forces URL to be treated as argument
.arg(&url)
.spawn()
.ok();
src/command/open.rs
Outdated
| } | ||
| #[cfg(target_os = "macos")] | ||
| { | ||
| Command::new("open").arg(&url).spawn().ok(); |
There was a problem hiding this comment.
Security concern: Command injection vulnerability (macOS)
Same issue as Windows - the URL needs proper escaping. On macOS, open interprets certain characters specially.
Recommendation:
Command::new("open")
.arg("--") // Ensures URL is treated as argument, not option
.arg(&url)
.spawn()
.ok();
src/command/open.rs
Outdated
| } | ||
| #[cfg(all(not(target_os = "windows"), not(target_os = "macos")))] | ||
| { | ||
| Command::new("xdg-open").arg(&url).spawn().ok(); |
There was a problem hiding this comment.
Security concern: Command injection vulnerability (Linux)
Same issue - xdg-open can be exploited with malicious URLs.
Recommendation:
Command::new("xdg-open")
.arg(&url)
.spawn()
.ok();Note: Using .arg() separately (not in the command string) provides better protection than shell interpolation.
| { | ||
| Command::new("cmd").args(["/C", "start", &url]).spawn().ok(); | ||
| } | ||
| #[cfg(target_os = "macos")] | ||
| { | ||
| Command::new("open").arg(&url).spawn().ok(); | ||
| } | ||
| #[cfg(all(not(target_os = "windows"), not(target_os = "macos")))] | ||
| { | ||
| Command::new("xdg-open").arg(&url).spawn().ok(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Error handling: Silent failures
All three platform-specific commands use .ok() which silently ignores errors. Users won't know if:
- The command doesn't exist on their system
- The URL format is invalid
- The browser launch failed
Recommendation:
let result = Command::new("open").arg(&url).spawn();
match result {
Ok(_) => {},
Err(e) => eprintln!("warning: Failed to open browser: {}", e),
}
src/command/open.rs
Outdated
| } | ||
|
|
||
| // Handle SCP-like syntax: git@github.com:user/repo.git | ||
| let scp_re = Regex::new(r"^git@([^:]+):(.+?)(\.git)?$").unwrap(); |
There was a problem hiding this comment.
Potential panic: unwrap() on regex
While this regex is valid and won't panic in practice, using unwrap() on Regex::new() is considered a code smell.
Best practice:
Use lazy_static or once_cell to compile the regex once at startup:
use once_cell::sync::Lazy;
static SCP_RE: Lazy<Regex> = Lazy::new(|| {
Regex::new(r"^git@([^:]+):(.+?)(\.git)?$").expect("Invalid SCP regex")
});
// Then use: if let Some(caps) = SCP_RE.captures(remote) { ... }This is more efficient and makes the panic cause clearer if it happens during initialization.
src/command/open.rs
Outdated
| // Handle ssh:// syntax | ||
| // ssh://[user@]host.xz[:port]/path/to/repo.git/ | ||
| if remote.starts_with("ssh://") { | ||
| let ssh_re = Regex::new(r"^ssh://(?:[^@]+@)?([^:/]+)(?::\d+)?/(.+?)(\.git)?$").unwrap(); |
There was a problem hiding this comment.
Same issue: unwrap() on regex
Same recommendation as above - use lazy_static or once_cell for the SSH regex.
| #[tokio::test] | ||
| #[serial] | ||
| async fn test_open_remote_origin() { | ||
| let repo_dir = tempdir().unwrap(); | ||
| test::setup_with_new_libra_in(repo_dir.path()).await; | ||
| let _guard = test::ChangeDirGuard::new(repo_dir.path()); | ||
|
|
||
| // Add origin remote | ||
| remote::execute(RemoteCmds::Add { | ||
| name: "origin".into(), | ||
| url: "git@github.com:user/repo.git".into(), | ||
| }) | ||
| .await; | ||
|
|
||
| // Test explicit remote | ||
| open::open(open::OpenArgs { | ||
| remote: Some("origin".to_string()), | ||
| }) | ||
| .await; | ||
|
|
||
| // Test default remote should find origin | ||
| open::open(open::OpenArgs { remote: None }).await; | ||
| } |
There was a problem hiding this comment.
Test coverage: Limited assertions
The test doesn't verify:
- That the correct URL was opened (no way to capture what was sent to the browser)
- That the URL transformation worked correctly
- Different remote URL formats (HTTPS, SSH, SCP)
- Error cases (no remote, invalid remote name)
Recommendation:
Consider adding unit tests for the transform_url function (which you already have ✓) and integration tests for error scenarios:
#[tokio::test]
#[serial]
async fn test_open_nonexistent_remote() {
let repo_dir = tempdir().unwrap();
test::setup_with_new_libra_in(repo_dir.path()).await;
let _guard = test::ChangeDirGuard::new(repo_dir.path());
// Should print error message and not panic
open::open(open::OpenArgs {
remote: Some("nonexistent".to_string()),
})
.await;
}
#[tokio::test]
#[serial]
async fn test_open_no_remotes_configured() {
let repo_dir = tempdir().unwrap();
test::setup_with_new_libra_in(repo_dir.path()).await;
let _guard = test::ChangeDirGuard::new(repo_dir.path());
// Should print error message and not panic
open::open(open::OpenArgs { remote: None }).await;
}
| if let Some(caps) = scp_re.captures(remote) { | ||
| let host = &caps[1]; | ||
| let path = &caps[2]; | ||
| return format!("https://{}/{}", host, path); |
There was a problem hiding this comment.
Code clarity: Direct string formatting
Minor: The format! macro can be simplified when concatenating with /:
return format!("https://{host}/{path}");This is more idiomatic Rust.
src/command/open.rs
Outdated
| if let Some(caps) = ssh_re.captures(remote) { | ||
| let host = &caps[1]; | ||
| let path = &caps[2]; | ||
| return format!("https://{}/{}", host, path); |
There was a problem hiding this comment.
Same as above: Can simplify to format!("https://{host}/{path}")
![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: 6647abe2a8
ℹ️ 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".
| match Config::get_current_remote_url().await { | ||
| Some(url) => url, |
There was a problem hiding this comment.
libra open calls Config::get_current_remote_url() here when no remote is supplied; that helper unwrap()s and panics if HEAD is detached. In that scenario, the command crashes before reaching the fallback to origin/first remote, so running open in a detached HEAD state will unexpectedly abort. Consider handling the detached-HEAD error and continuing to the fallback branch instead of unwrapping.
Useful? React with 👍 / 👎.
src/command/open.rs
Outdated
| // Handle SCP-like syntax: git@github.com:user/repo.git | ||
| let scp_re = Regex::new(r"^git@([^:]+):(.+?)(\.git)?$").unwrap(); | ||
| if let Some(caps) = scp_re.captures(remote) { |
There was a problem hiding this comment.
The SCP-like parser only matches git@host:path (^git@...) so remotes like alice@github.com:org/repo.git (or enterprise setups with a different SSH user) won’t be transformed into an HTTPS URL. Those inputs fall through to xdg-open/open as a raw SSH string, which typically can’t be opened in a browser, so libra open silently fails for a common remote format. The regex should accept any user@ prefix (or parse generically) to cover these cases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR implements the libra open command, which opens a Git remote repository URL in the system's default browser. The implementation converts various Git URL formats (HTTPS, SCP-style SSH, ssh://) to standard HTTPS URLs and launches them using platform-specific commands.
Changes:
- Added new
opencommand module with URL transformation logic and cross-platform browser launching - Integrated the command into the CLI interface
- Added unit tests for URL transformation logic and integration tests for remote detection
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/command/open.rs | New command implementation with URL transformation logic, browser launching for Windows/macOS/Linux, and unit tests |
| src/command/mod.rs | Registered the new open module in the command exports |
| src/cli.rs | Added Open command variant and routing to the open command handler |
| tests/command/open_test.rs | New integration test for open command with remote origin setup |
| tests/command/mod.rs | Registered the new open_test module in test exports |
tests/command/open_test.rs
Outdated
| // Test explicit remote | ||
| open::open(open::OpenArgs { | ||
| remote: Some("origin".to_string()), | ||
| }) | ||
| .await; | ||
|
|
||
| // Test default remote should find origin | ||
| open::open(open::OpenArgs { remote: None }).await; |
There was a problem hiding this comment.
The integration test doesn't verify the expected behavior - it calls the open function but doesn't assert anything. The test should verify that:
- The correct remote URL is identified
- The URL is properly transformed
- The operation completes without errors
Consider capturing stdout/stderr or mocking the browser launch to verify the correct URL is being opened.
src/command/open.rs
Outdated
| remote.to_string() | ||
| } | ||
|
|
||
| // Uint test |
There was a problem hiding this comment.
Typo in the comment. Should be "Unit test" instead of "Uint test".
| // Uint test | |
| // Unit test |
src/command/open.rs
Outdated
| let scp_re = Regex::new(r"^git@([^:]+):(.+?)(\.git)?$").unwrap(); | ||
| if let Some(caps) = scp_re.captures(remote) { | ||
| let host = &caps[1]; | ||
| let path = &caps[2]; | ||
| return format!("https://{}/{}", host, path); | ||
| } | ||
|
|
||
| // Handle ssh:// syntax | ||
| // ssh://[user@]host.xz[:port]/path/to/repo.git/ | ||
| if remote.starts_with("ssh://") { | ||
| let ssh_re = Regex::new(r"^ssh://(?:[^@]+@)?([^:/]+)(?::\d+)?/(.+?)(\.git)?$").unwrap(); |
There was a problem hiding this comment.
The Regex compilation happens on every call to transform_url. According to Rust performance best practices and the custom guidelines emphasizing performance optimization, these regexes should be compiled once using lazy_static or std::sync::OnceLock to avoid repeated compilation overhead. This is especially important as this function is in a hot path called for every URL transformation.
src/command/open.rs
Outdated
| pub remote: Option<String>, | ||
| } | ||
|
|
||
| pub async fn open(args: OpenArgs) { |
There was a problem hiding this comment.
The function name 'open' is inconsistent with the established naming convention in this codebase. All other command modules use 'execute' as the entry point function name (see add.rs:58, blame.rs:45, branch.rs:56, etc.). For consistency and adherence to codebase conventions, this function should be renamed to 'execute' and should take 'OpenArgs' as its parameter, matching the pattern 'pub async fn execute(args: OpenArgs)'.
tests/command/open_test.rs
Outdated
| open::open(open::OpenArgs { | ||
| remote: Some("origin".to_string()), | ||
| }) | ||
| .await; | ||
|
|
||
| // Test default remote should find origin | ||
| open::open(open::OpenArgs { remote: None }).await; |
There was a problem hiding this comment.
The test calls 'open::open' but according to the codebase convention established in this PR review, the function should be named 'execute' (see src/cli.rs and other command modules). Once the function is renamed, this test call should also be updated to 'open::execute'.
src/command/open.rs
Outdated
| Command::new("cmd").args(["/C", "start", &url]).spawn().ok(); | ||
| } | ||
| #[cfg(target_os = "macos")] | ||
| { | ||
| Command::new("open").arg(&url).spawn().ok(); | ||
| } | ||
| #[cfg(all(not(target_os = "windows"), not(target_os = "macos")))] | ||
| { | ||
| Command::new("xdg-open").arg(&url).spawn().ok(); |
There was a problem hiding this comment.
The Command::spawn() calls have their errors silently discarded with .ok(). If the browser fails to open (e.g., command not found, permission denied), the user will see "Opening URL" but the browser won't actually open and they'll get no indication of what went wrong. Consider handling the error and providing user feedback. For example, you could check the Result and print a warning if the spawn fails.
| Command::new("cmd").args(["/C", "start", &url]).spawn().ok(); | |
| } | |
| #[cfg(target_os = "macos")] | |
| { | |
| Command::new("open").arg(&url).spawn().ok(); | |
| } | |
| #[cfg(all(not(target_os = "windows"), not(target_os = "macos")))] | |
| { | |
| Command::new("xdg-open").arg(&url).spawn().ok(); | |
| if let Err(err) = Command::new("cmd").args(["/C", "start", &url]).spawn() { | |
| eprintln!("warning: failed to open URL '{}' in browser: {}", url, err); | |
| } | |
| } | |
| #[cfg(target_os = "macos")] | |
| { | |
| if let Err(err) = Command::new("open").arg(&url).spawn() { | |
| eprintln!("warning: failed to open URL '{}' in browser: {}", url, err); | |
| } | |
| } | |
| #[cfg(all(not(target_os = "windows"), not(target_os = "macos")))] | |
| { | |
| if let Err(err) = Command::new("xdg-open").arg(&url).spawn() { | |
| eprintln!("warning: failed to open URL '{}' in browser: {}", url, err); | |
| } |
src/command/open.rs
Outdated
| if remotes.iter().any(|r| r.name == remote_name) { | ||
| Config::get_remote_url(&remote_name).await |
There was a problem hiding this comment.
When a remote name is specified, Config::get_remote_url can panic if the remote exists but has no URL configured (as seen in src/internal/config.rs:147). This is inconsistent with the check on line 17 which verifies the remote exists. Consider handling this potential panic case gracefully with an error message, or ensure the remote_config check on line 17 also validates that a URL is present.
| if remotes.iter().any(|r| r.name == remote_name) { | |
| Config::get_remote_url(&remote_name).await | |
| if let Some(remote) = remotes.iter().find(|r| r.name == remote_name) { | |
| if remote.url.is_empty() { | |
| eprintln!("fatal: Remote '{}' has no URL configured", remote_name); | |
| return; | |
| } | |
| remote.url.clone() |
| @@ -0,0 +1,114 @@ | |||
| use std::process::Command; | |||
There was a problem hiding this comment.
Missing module-level documentation. All other command modules in this codebase include a //! doc comment at the top describing the module's purpose (see add.rs, blame.rs, branch.rs, etc.). Add a module-level doc comment following the established convention, e.g., "//! Opens the remote repository URL in the default browser by transforming Git URLs to HTTPS format."
src/cli.rs
Outdated
| Commands::Blame(args) => command::blame::execute(args).await, | ||
| Commands::Revert(args) => command::revert::execute(args).await, | ||
| Commands::Remote(cmd) => command::remote::execute(cmd).await, | ||
| Commands::Open(args) => command::open::open(args).await, |
There was a problem hiding this comment.
The function call should be 'command::open::execute(args)' instead of 'command::open::open(args)' to match the naming convention established in this file. All other commands use the execute function (see lines 209-213: blame::execute, revert::execute, remote::execute, pull::execute, config::execute, etc.).
| // Handle SCP-like syntax: git@github.com:user/repo.git | ||
| let scp_re = Regex::new(r"^git@([^:]+):(.+?)(\.git)?$").unwrap(); | ||
| if let Some(caps) = scp_re.captures(remote) { | ||
| let host = &caps[1]; | ||
| let path = &caps[2]; | ||
| return format!("https://{}/{}", host, path); | ||
| } |
There was a problem hiding this comment.
The SCP-style regex pattern (line 64) assumes "git@" as the user, but SCP syntax can use any username. For example, "john@gitlab.com:repo/project.git" is valid Git syntax but won't match this pattern. Consider making the user part generic by changing the regex to match any user, e.g., r"^([^@]+)@([^:]+):(.+?)(\.git)?$" and capturing both user and host, or at minimum updating the regex to handle any username.
Tyooughtul
force-pushed
the
main
branch
2 times, most recently
from
17f50a6 to
891697c
Compare
|
@codex review |
|
Claude finished @genedna's task in 1m 26s —— View job Code Review Complete ✓Todo List:
SummaryExcellent work on implementing the ✅ What's Great
🟡 Issues to Address1. Naming Convention Inconsistency (src/command/open.rs:21, src/cli.rs:214)Severity: Medium - Inconsistent with codebase patterns All other command modules in this codebase use File: src/command/open.rs // Change line 21 from:
pub async fn open(args: OpenArgs) {
// To:
pub async fn execute(args: OpenArgs) {File: src/cli.rs // Change line 214 from:
Commands::Open(args) => command::open::open(args).await,
// To:
Commands::Open(args) => command::open::execute(args).await,File: tests/command/open_test.rs // Update all test calls from open::open(...) to open::execute(...)2. Typo in Comment (src/command/open.rs:128)Severity: Minor // Line 128: Change "Uint test" to "Unit test"
// Unit test3. Limited SSH User Support (src/command/open.rs:16)Severity: Low - Edge case limitation The SCP-style regex Consideration: This is a minor edge case. The current implementation handles the most common scenarios (git@). If you want broader support, you could change the regex to: static ref SCP_RE: Regex = Regex::new(r"^([^@]+)@([^:]+):(.+?)(\.git)?$").expect("Invalid Regex");And adjust the capture group indices in the transform function accordingly. However, this is not critical for the initial implementation. 4. Test Assertions (tests/command/open_test.rs:27-39)Severity: Low - Test quality The integration tests call the
However, testing browser launching is inherently difficult without mocking, so the current approach is reasonable for integration tests. The unit tests for 📋 Minor Observations
The
The current blocking behavior is actually different from git's behavior, where 🎯 RecommendationPriority fixes:
Optional improvements: 🔒 Security Assessment✅ Security is now EXCELLENT - The latest commit properly addressed the command injection vulnerability by:
Final VerdictThis is solid work! The implementation is functional, secure, and well-tested. After addressing the naming convention inconsistency (which is the only blocking issue), this PR will be ready to merge. Great job fixing the security issue! 🎉 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 891697caa1
ℹ️ 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".
| match Config::get_current_remote_url().await { | ||
| Some(url) => url, | ||
| None => { |
There was a problem hiding this comment.
When no remote is provided, this path calls Config::get_current_remote_url().await directly. That helper unwraps the result of get_current_remote_with_conn (see src/internal/config.rs around the unwrap() at lines 152-154), which panics on detached HEAD. Running libra open in a detached HEAD repo will therefore crash instead of emitting a user-friendly error. Consider checking get_current_remote_with_conn for Err (or providing a safe wrapper) before calling here so the command can report the detached HEAD condition gracefully.
Useful? React with 👍 / 👎.
|
@Tyooughtul ， 请继续依据 #169 (comment) 的提示进行修改 |
src/command/open.rs
Outdated
| remote.to_string() | ||
| } | ||
|
|
||
| // Uint test |
There was a problem hiding this comment.
Typo in comment: "Uint test" should be "Unit test".
| // Uint test | |
| // Unit test |
| async fn test_open_remote_origin() { | ||
| let repo_dir = tempdir().unwrap(); | ||
| test::setup_with_new_libra_in(repo_dir.path()).await; | ||
| let _guard = test::ChangeDirGuard::new(repo_dir.path()); | ||
|
|
||
| // Add origin remote | ||
| remote::execute(RemoteCmds::Add { | ||
| name: "origin".into(), | ||
| url: "git@github.com:user/repo.git".into(), | ||
| }) | ||
| .await; | ||
|
|
||
| // Test explicit remote | ||
| open::open(open::OpenArgs { | ||
| remote: Some("origin".to_string()), | ||
| }) | ||
| .await; | ||
|
|
||
| // Test default remote should find origin | ||
| open::open(open::OpenArgs { remote: None }).await; | ||
|
|
||
| // Test non-existent remote | ||
| open::open(open::OpenArgs { | ||
| remote: Some("nonexistent".to_string()), | ||
| }) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
This integration test lacks assertions to verify the behavior. The test calls open::open() but doesn't check:
- Whether the function succeeds or fails appropriately
- What error messages are printed for the non-existent remote case (line 35-39)
- Any state changes or side effects
Without assertions, this test will always pass even if the functionality is broken. Consider capturing stdout/stderr or having the open function return a Result that can be asserted, or at minimum verify that appropriate error messages are output for error cases.
| async fn test_open_no_remote() { | ||
| let repo_dir = tempdir().unwrap(); | ||
| test::setup_with_new_libra_in(repo_dir.path()).await; | ||
| let _guard = test::ChangeDirGuard::new(repo_dir.path()); | ||
|
|
||
| // Should handle no remote configured | ||
| open::open(open::OpenArgs { remote: None }).await; | ||
| } |
There was a problem hiding this comment.
This test lacks assertions. It calls open::open() with no remote configured, but doesn't verify:
- That an appropriate error message is printed
- That the function handles the error gracefully
Without assertions, this test will pass even if the function panics or behaves incorrectly. Add assertions to verify expected error output or return values.
src/command/open.rs
Outdated
| Command::new("cmd") | ||
| .args(["/C", "start", "", url]) | ||
| .spawn()? | ||
| .wait()?; |
There was a problem hiding this comment.
Potential security issue: The Windows command execution pattern using 'start' with an empty string for the title parameter could be exploited if the URL contains special characters. For example, a URL starting with '&' could execute arbitrary commands. While is_safe_url() validates the URL scheme, it doesn't prevent all injection vectors on Windows. Consider using proper escaping or the 'open' crate which handles cross-platform browser opening more safely. Alternatively, ensure the URL is properly quoted in the command arguments.
src/cli.rs
Outdated
| Commands::Blame(args) => command::blame::execute(args).await, | ||
| Commands::Revert(args) => command::revert::execute(args).await, | ||
| Commands::Remote(cmd) => command::remote::execute(cmd).await, | ||
| Commands::Open(args) => command::open::open(args).await, |
There was a problem hiding this comment.
The Open command should be allowed to run outside a repository when a URL is provided directly as an argument (as the code supports on lines 33-36 of open.rs). However, the CLI framework at src/cli.rs:174-180 requires all commands except Init and Clone to be run within a repository, returning GitError::RepoNotFound otherwise. This prevents the use case of "libra open https://github.com/user/repo" from outside a repository. Consider adding Commands::Open to the exception list alongside Init and Clone, or redesign the command to always require being in a repository.
| async fn test_open_remote_origin() { | ||
| let repo_dir = tempdir().unwrap(); | ||
| test::setup_with_new_libra_in(repo_dir.path()).await; | ||
| let _guard = test::ChangeDirGuard::new(repo_dir.path()); | ||
|
|
||
| // Add origin remote | ||
| remote::execute(RemoteCmds::Add { | ||
| name: "origin".into(), | ||
| url: "git@github.com:user/repo.git".into(), | ||
| }) | ||
| .await; | ||
|
|
||
| // Test explicit remote | ||
| open::open(open::OpenArgs { | ||
| remote: Some("origin".to_string()), | ||
| }) | ||
| .await; | ||
|
|
||
| // Test default remote should find origin | ||
| open::open(open::OpenArgs { remote: None }).await; | ||
|
|
||
| // Test non-existent remote | ||
| open::open(open::OpenArgs { | ||
| remote: Some("nonexistent".to_string()), | ||
| }) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
These integration tests will attempt to actually open a browser during test execution, which is problematic for several reasons:
- CI/CD environments typically don't have browsers or display servers configured
- Opening browsers during automated tests is disruptive to developers
- The tests cannot verify whether the browser opened successfully
Consider refactoring the open function to accept a dependency injection parameter for browser opening (or use a trait), so tests can mock the browser-opening behavior without actually spawning processes. Alternatively, add a feature flag or environment variable to skip browser opening during tests.
| match Config::get_current_remote_url().await { | ||
| Some(url) => url, | ||
| None => { | ||
| // Fallback, try origin | ||
| let remotes = Config::all_remote_configs().await; | ||
| if let Some(origin) = remotes.iter().find(|r| r.name == "origin") { | ||
| origin.url.clone() | ||
| } else if let Some(first) = remotes.first() { | ||
| first.url.clone() // Fallback to first available remote | ||
| } else { | ||
| eprintln!("fatal: No remote configured"); | ||
| return; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The fallback logic for finding a remote has potential issues. When Config::get_current_remote_url() returns None, the code falls back to "origin", then to the first remote. However, this behavior is not clearly communicated to the user. Consider adding informational messages like "Using remote 'origin'" or "Using remote 'X' (first available)" so users understand which remote is being opened. This is especially important when multiple remotes exist.
| fn test_transform_url() { | ||
| assert_eq!( | ||
| transform_url("git@github.com:rust-lang/rust.git"), | ||
| "https://github.com/rust-lang/rust" | ||
| ); | ||
| assert_eq!( | ||
| transform_url("git@gitlab.com:group/project.git"), | ||
| "https://gitlab.com/group/project" | ||
| ); | ||
| assert_eq!( | ||
| transform_url("https://github.com/rust-lang/rust.git"), | ||
| "https://github.com/rust-lang/rust" | ||
| ); | ||
| assert_eq!( | ||
| transform_url("ssh://git@github.com/rust-lang/rust.git"), | ||
| "https://github.com/rust-lang/rust" | ||
| ); | ||
| assert_eq!( | ||
| transform_url("ssh://user@host.com:2222/repo.git"), | ||
| "https://host.com/repo" | ||
| ); | ||
| } |
There was a problem hiding this comment.
The unit tests don't cover several important edge cases:
- URLs with usernames other than 'git' in SCP format (e.g., 'myuser@github.com:repo/path.git')
- HTTPS URLs with trailing slashes
- URLs without .git suffix
- Malformed URLs that should fail gracefully
- File:// URLs (should be rejected by is_safe_url)
- URLs with query parameters or fragments
Consider adding test cases for these scenarios to ensure robust URL handling.
引入 \`libra open\` 命令 主要功能： - 自动识别远程仓库 - 支持指定远程名称（如 \`libra open upstream\`） - 支持直接打开任意 Git URL，即使当前不在仓库目录下 - 转换 SSH/SCP 格式地址（如 git@github.com:...）为 HTTPS 链接 - 跨平台支持 - 增加 URL 安全性校验，防止命令注入风险 测试情况： - 已添加 URL 转换逻辑与安全校验的单元测试 - 已添加集成测试覆盖常见使用场景 Signed-off-by: Tyooughtul <yooughtul@126.com>
2 participants
新增libra open的功能用于打开.git配置的远程仓库
新增相应单元测试和集成测试且均已通过
#167