fix(branch-tag-reset): close review gaps#334
fix(branch-tag-reset): close review gaps#334genedna wants to merge 9 commits intoweb3infra-foundation:mainfrom
Conversation
Signed-off-by: Quanyi Ma <eli@patch.sh>
![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: 210205d877
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR closes review gaps for the branch, tag, and reset commands by tightening typed error handling, unifying run/render execution paths, and expanding regression coverage for output/help contracts.
Changes:
- Introduces/expands typed error enums and centralized
run_*+render_*flows forbranch,tag, andreset. - Adds human-mode confirmation output and JSON contract refinements (while keeping JSON-only fields stable via
skip_serializing). - Expands CLI integration tests for confirmations, JSON nullability, help
EXAMPLES, and corruption/error-code surfacing.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/command/tag_test.rs | Adds coverage for JSON message: null on lightweight tags and human confirmation lines. |
| tests/command/reset_test.rs | Adds regression test ensuring subtree corruption during pathspec reset maps to repo-corrupt error behavior. |
| tests/command/output_flags_test.rs | Ensures --help output includes an EXAMPLES: section for branch/reset/tag. |
| tests/command/branch_test.rs | Adds coverage for branch create/delete confirmations, suggestions, and config write failure surfacing (unix). |
| src/internal/branch.rs | Adds BranchStoreError + result-returning APIs; updates parsing to avoid panics on corrupt stored data. |
| src/command/tag.rs | Adds TagError, unifies execution via run_tag and rendering via render_tag_output, adds help examples. |
| src/command/reset.rs | Adds ResetError, typed propagation through helpers, warning aggregation via emit_warning, adds help examples. |
| src/command/branch.rs | Adds BranchError, unifies run/render path, adds fuzzy “did you mean” suggestions and help examples. |
| docs/improvement/tag.md | Updates status/plan narrative for tag improvements. |
| docs/improvement/reset.md | Updates status/plan narrative for reset improvements. |
| docs/improvement/README.md | Updates overall improvement roadmap status table. |
| docs/improvement/branch.md | Updates status/plan narrative for branch improvements. |
| pub async fn list_branches_with_conn<C>(db: &C, remote: Option<&str>) -> Vec<Self> | ||
| where | ||
| C: ConnectionTrait, | ||
| { | ||
| Self::list_branches_result_with_conn(db, remote) | ||
| .await | ||
| .unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
list_branches_with_conn() now converts any BranchStoreError into an empty branch list via unwrap_or_default(). This can silently mask DB/query corruption and cause callers like Branch::list_branches() (used by fetch/log/remote/switch/etc.) to behave as if there are zero branches rather than surfacing an actionable error.
Consider either (a) updating the non-*_result* APIs (list_branches / list_branches_with_conn) to return Result<Vec<Branch>, BranchStoreError> and migrating callers, or (b) at least emitting a tracing::error!/warning when falling back to default so the failure is observable.
Signed-off-by: Quanyi Ma <eli@patch.sh>
genedna
left a comment
genedna
left a comment
There was a problem hiding this comment.
Comprehensive Code Review — PR #334
Overall Assessment
This is a well-structured refactoring PR that modernizes three commands (branch, tag, reset) with typed error enums, run/render separation, and structured JSON output. The changes are consistent with the patterns established in earlier batches (switch/checkout, bisect). The internal/branch.rs fallible API additions are a welcome improvement over the old unwrap()-based code paths.
Code Quality
Strengths:
- Excellent adoption of the
thiserrorerror enum pattern across all three commands (BranchError,TagError,ResetError), with well-structuredFrom<XError> for CliErrorimpls that centralize error-to-exit-code mapping. - Clean run/render separation:
run_branch()→render_branch_output()pattern eliminates the old JSON-vs-text branching inexecute_safe. - The new
BranchStoreErrorininternal/branch.rsreplacesunwrap()calls on DB queries with proper error propagation — a significant robustness improvement. - Good use of
#[serde(skip_serializing)]for display-only fields (display_name,display_message) that shouldn't leak into JSON. - Levenshtein fuzzy matching for branch-not-found suggestions is a nice UX touch.
Issues flagged inline:
- Bug:
set_upstream_with_connidempotency check comparesconfig.mergeagainst the short branch name instead of the fullrefs/heads/...ref — the optimization never triggers. - Non-atomic rename:
rename_branch_implcreates the new branch, updates HEAD, then deletes the old branch without a transaction wrapper.
Security
- No security vulnerabilities identified. Input validation is handled via clap's argument parsing.
- The
split_once('/')upstream parsing correctly rejects inputs without a slash viaBranchError::InvalidUpstream. - No user-controlled strings are interpolated into SQL — all DB operations go through Sea-ORM's parameterized queries.
Performance
- The idempotency check in
set_upstream_with_conn(if fixed) would save unnecessary DB writes. branch_not_found_errorloads all branches to compute Levenshtein suggestions. For repositories with thousands of branches, this could be slow. Consider capping the iteration or only computing suggestions when the branch list is below a threshold.collect_branch_outputdoes separate queries for local and remote branches, then filters both — this is reasonable for the expected scale.
Testing
Strengths:
- Good coverage of new behaviors:
test_branch_create_outputs_confirmation,test_branch_not_found_suggests_similar_name,test_branch_force_delete_outputs_confirmation. - The
#[cfg(unix)]test for config-write failure via read-only DB permissions is creative and tests a real failure mode. test_reset_pathspec_surfaces_subtree_corruption_as_repo_corruptis excellent — it corrupts a real object on disk and verifies the newResetError::ObjectLoadpath.- Tag tests verify the lightweight-vs-annotated JSON contract (
messageisnullfor lightweight tags). output_flags_test.rsadditions verify--helpincludes EXAMPLES sections for all three commands.
Suggestions:
- No test for the
set_upstream_with_connidempotency path (currently broken). After the fix, add a test that sets upstream twice and verifies the second call is a no-op. - No test for
rename_branch_impl— consider adding one that verifies HEAD is updated when renaming the current branch.
Documentation
BRANCH_AFTER_HELP,RESET_EXAMPLES, andTAG_EXAMPLESconstants provide useful CLI examples including agent/JSON usage patterns.- The
docs/improvement/updates accurately reflect the landed state vs. remaining work items. - The improvement docs are thorough and well-organized with clear "landed" vs "remaining" tracking.
Summary of Action Items
| Priority | File | Issue |
|---|---|---|
| High | branch.rs:435 |
Fix idempotency check: config.merge != merge_ref instead of config.merge != remote_branch |
| Medium | branch.rs:617-630 |
Consider transactional rename or document non-atomicity |
| Low | reset.rs:270,290 |
Log get_commit_summary errors instead of silent unwrap_or_default() |
| Low | reset.rs:443 |
Log rollback warnings before discarding |
| Low | tag.rs:287 / branch.rs:330 |
Extract shared short_hash/abbreviate_hash utility |
Good work overall — the typed error approach is paying dividends in readability and testability. The main issue to address is the idempotency bug in set_upstream_with_conn.
Generated by Claude Code
| let merge_ref = format!("refs/heads/{remote_branch}"); | ||
| let should_write = branch_config | ||
| .as_ref() | ||
| .map(|config| config.remote != remote || config.merge != remote_branch) |
There was a problem hiding this comment.
Bug: Idempotency check compares against wrong value.
Line 432 constructs merge_ref = format!("refs/heads/{remote_branch}"), and line 417 correctly writes merge_ref to the DB. However, the idempotency check here compares config.merge != remote_branch (the short name) instead of config.merge != merge_ref (the full ref path that was actually stored).
Since the stored value is refs/heads/main but remote_branch is just main, this comparison always evaluates to true, meaning the write is never skipped — the idempotency optimization is dead code.
Suggested fix:
.map(|config| config.remote != remote || config.merge != merge_ref)Generated by Claude Code
|
|
||
| // delete old branch | ||
| Branch::delete_branch(&old_name, None).await; | ||
| Branch::delete_branch_result(&old_name, None) |
There was a problem hiding this comment.
Nit (pre-existing, low priority): Non-atomic rename — Branch::update_branch creates the new branch (line 617), then Head::update moves HEAD (line 625), then Branch::delete_branch_result deletes the old branch (line 630). If the delete fails, both branches exist with no rollback. Consider wrapping these operations in a transaction via _with_conn variants to make the rename atomic, or at minimum document this as a known limitation.
Generated by Claude Code
| .await; | ||
| ) -> Result<(), BranchError> { | ||
| let (remote, remote_branch) = upstream | ||
| .split_once('/') |
There was a problem hiding this comment.
Edge case: split_once('/') only splits on the first slash. For a branch like origin/feature/foo, this yields remote="origin" and remote_branch="feature/foo", which is correct. However, if someone has a remote named my/remote (unusual but valid in git), this would mis-parse. This matches git's own behavior, so it's fine for now — just flagging for awareness.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b48a0947ed
ℹ️ 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".
| if let Err(error) = Self::delete_branch_result_with_conn(db, branch_name, remote).await { | ||
| log_branch_store_error( |
There was a problem hiding this comment.
delete_branch_with_conn now swallows all deletion errors and only logs via tracing, but tracing is initialized only when LIBRA_LOG/RUST_LOG is set (see src/main.rs), so default CLI runs drop the error entirely. Callers like remote prune (src/command/remote.rs) still print *[pruned]* and increment counts even if the DB delete failed, which creates a silent false-success path and leaves stale remote-tracking refs. Return a Result here (or otherwise surface a hard error) so commands cannot report successful pruning when nothing was deleted.
Useful? React with 👍 / 👎.
Signed-off-by: Quanyi Ma <eli@patch.sh>
| // avoids holding the transaction open while doing read-only preparations. | ||
| let db = get_db_conn_instance().await; | ||
| let old_oid = Head::current_commit_with_conn(&db) | ||
| .await | ||
| .ok_or_else(|| "Cannot reset: HEAD is unborn and points to no commit.".to_string())?; | ||
| .ok_or(ResetError::HeadUnborn)?; |
There was a problem hiding this comment.
perform_reset() uses Head::current_commit_with_conn(&db) (lossy) and maps None directly to ResetError::HeadUnborn. But current_commit_with_conn now swallows DB/branch-store errors (logs and returns None), so a corrupted/failed HEAD resolution can be misreported as an unborn HEAD (RepoStateInvalid) instead of RepoCorrupt/IoReadFailed. Use Head::current_commit_result_with_conn(&db).await here and map Err(BranchStoreError::Query(_)) to an I/O read error, and other errors to repo corruption, while reserving Ok(None) for the true unborn-HEAD case.
| @@ -277,259 +487,363 @@ pub async fn create_branch_safe( | |||
| Head::Branch(name) => name, | |||
| Head::Detached(commit_hash) => commit_hash.to_string(), | |||
| }; | |||
| return Err( | |||
| CliError::fatal(format!("not a valid object name: '{}'", current)) | |||
| .with_stable_code(StableErrorCode::CliInvalidTarget), | |||
| ); | |||
| return Err(BranchError::InvalidCommit(current)); | |||
| } | |||
There was a problem hiding this comment.
create_branch_impl falls back to Head::current_commit().await (lossy) when no base is provided. Since Head::current_commit() now logs and returns None on branch-store failures, this path can turn repository corruption/DB read failures into a misleading InvalidCommit (CliInvalidTarget) error. Prefer Head::current_commit_result() so DB/query/corruption errors are surfaced with the right stable code, and reserve Ok(None) for the true unborn-HEAD case (likely RepoStateInvalid).
Signed-off-by: Quanyi Ma <eli@patch.sh>
Signed-off-by: Quanyi Ma <eli@patch.sh>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a5667bac9
ℹ️ 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".
Signed-off-by: Quanyi Ma <eli@patch.sh>
|
@codex review |
genedna
left a comment
genedna
left a comment
There was a problem hiding this comment.
Follow-up Review — PR #334 (v2, 4 new commits)
Previous Review Issues — Resolution Status
| Issue | Status | Notes |
|---|---|---|
Idempotency bug in set_upstream_with_conn (config.merge != remote_branch vs merge_ref) |
✅ Fixed | Comparison is now against the short name, with a comment explaining that branch_config_with_conn() normalizes refs/heads/<name> → <name>. Correct. |
Duplicated short_hash/abbreviate_hash |
✅ Fixed | Consolidated into utils::text::short_display_hash with Unicode-safe char_indices() slicing and unit tests. Both local copies removed from branch.rs and tag.rs. |
Silent unwrap_or_default() on get_commit_summary in reset.rs |
✅ Fixed | Replaced with load_commit_summary_or_warn() that logs at tracing::warn! before falling back. |
| Rollback warnings silently dropped in reset.rs | ✅ Fixed | Now logged via tracing::warn! with structured fields before discarding. |
Non-atomic rename_branch_impl |
Still three separate operations without a transaction wrapper. Acceptable as a known limitation for now. |
All high-priority items are resolved. Nice work on the turnaround.
New Changes in v2 — Scope Expansion
The v2 update goes well beyond fixing the flagged issues. It systematically migrates 16 command files from silent-failure Branch::find_branch / Head::current_commit / Branch::list_branches (returning Option, swallowing DB errors) to new _result-suffixed variants that return Result. This is a significant robustness improvement across the codebase.
Files touched beyond original PR scope: switch.rs, status.rs, show.rs, show_ref.rs, log.rs, checkout.rs, clone.rs, push.rs, remote.rs, restore.rs, shortlog.rs, fetch.rs, merge.rs, cloud.rs, config.rs, local_client.rs
Code Quality
Strengths:
- Consistent error-mapping pattern: each command defines a local
map_*_branch_store_errorhelper that convertsBranchStoreErrorinto the command's own error type. This keeps concerns well-separated. internal/head.rsaddscurrent_commit_result/current_commit_result_with_connfollowing the established_resultconvention, with the old non-_resultversion now delegating and logging errors.utils::textmodule is clean, well-tested, and properly exported.- The
fetch.rsfix removingunwrap_or_default()onConfigKv::all_remote_configs()closes a real silent-failure bug (the--allflag would silently skip all remotes if config reading failed).
Behavior changes to note:
- Several commands now abort on DB read failures that were previously silently degraded (e.g.,
log.rsref decorations,status.rsupstream info). This is the correct trade-off (surface errors rather than hide them), but it changes user-visible behavior in degraded environments.
Security
No new concerns. All DB operations continue to use Sea-ORM's parameterized queries. No user input reaches raw SQL.
Performance
get_commit_baseinutils/util.rsnow does a two-step remote branch resolution (fullrefs/remotes/origin/mainfirst, then short-name fallback). This adds one extra DB query in the fallback path but correctly handles both fetched remote-tracking refs and directly-stored remote branches.- No other performance regressions identified.
Testing
New tests (strong coverage):
test_branch_set_upstream_idempotent_path_skips_redundant_write— proves the fixed idempotency check works by making the DB read-only after the first write. Clever technique.test_branch_create_without_base_surfaces_corrupt_head_storage+ two siblings — corrupt-ref fault injection tests for create, delete, and show-current paths.test_branch_list_surfaces_corrupt_reference_name— tests list with a corrupt non-HEAD branch.test_remote_prune_does_not_report_success_when_delete_fails— end-to-end prune test with permission manipulation.test_reset_corrupt_head_reference_returns_repo_corrupt— with a negative regression guard against "HEAD is unborn" misclassification.tests/local_client_test.rs— verifies cwd restoration afterdiscovery_referenceerror.
Suggestions for additional coverage:
rename_branch_implhas no integration test (create → rename → verify HEAD update).--show-currentcorrupt-HEAD test may be over-broad —show-currentarguably should only read the branch name, not resolve the commit hash. Worth verifying this is intentional behavior rather than encoding a bug.
Summary of Remaining Items
| Priority | File | Issue |
|---|---|---|
| Medium | local_client.rs:73 |
with_repo_current_dir is not cancellation-safe — if the async future is dropped, cwd is not restored. Consider a Drop-based guard. |
| Low | branch.rs |
Non-atomic rename still unaddressed (known limitation). |
| Low | Test coverage | No test for rename_branch_impl; --show-current corrupt-HEAD behavior should be verified as intentional. |
Overall: This is a substantial quality improvement. The systematic migration to fallible APIs is well-executed and consistent. The previous review's high-priority items are all resolved. The remaining items are low-to-medium severity. LGTM with the local_client.rs cancellation concern as the only notable open item.
Generated by Claude Code
| } | ||
|
|
||
| impl LocalClient { | ||
| async fn with_repo_current_dir<T, E, F, Fut>(&self, operation: F) -> Result<T, E> |
There was a problem hiding this comment.
Cancellation safety concern: with_repo_current_dir changes process-global state (env::set_current_dir) without a Drop-based guard. If the operation().await future is cancelled (e.g., Tokio task abort, select! branch), the working directory is left pointing to self.repo_path with no restore.
Consider using a RAII guard struct with a Drop impl that restores the directory, similar to ChangeDirGuard in utils/test.rs. This would ensure restoration even on cancellation:
struct CwdGuard { original: PathBuf }
impl Drop for CwdGuard {
fn drop(&mut self) {
let _ = env::set_current_dir(&self.original);
}
}Alternatively, if cancellation is not expected in practice for these call sites, a brief // SAFETY: comment documenting this assumption would suffice.
Generated by Claude Code
| && let Some(branch) = Branch::find_branch(branch_name, Some(remote)).await | ||
| { | ||
| return Ok(branch.commit); | ||
| let remote_tracking_ref = format!("refs/remotes/{remote}/{branch_name}"); |
There was a problem hiding this comment.
Nice improvement: the two-step remote branch resolution (first try full refs/remotes/origin/main ref, then fall back to short main with remote=origin) correctly handles both fetched remote-tracking refs and directly-stored remote branches. The comment documenting this dual-lookup strategy is clear.
Generated by Claude Code
| )); | ||
| return Err(ResetError::PathspecWithHard); | ||
| } | ||
|
|
There was a problem hiding this comment.
Previous review issue resolved: the silent unwrap_or_default() has been replaced with load_commit_summary_or_warn() which logs at tracing::warn! before falling back. This is exactly what was suggested. The rollback warning suppression (previously let _ = ...) is also now addressed with tracing::warn! logging. Both fixes look good.
Generated by Claude Code
| pub const SHORT_HASH_LEN: usize = 7; | ||
|
|
||
| /// Return a shortened display form of a hash-like string without assuming ASCII. | ||
| pub fn short_display_hash(hash: &str) -> &str { |
There was a problem hiding this comment.
Well done: short_display_hash uses char_indices().nth() to find a safe byte boundary instead of raw byte slicing. This is strictly safer than the removed &hash[..end] pattern, even though git hashes are hex-only in practice. The unit test for UTF-8 boundaries (short_display_hash_respects_utf8_boundaries) validates this. Clean extraction from the duplicate implementations.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cedf0910b3
ℹ️ 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".
Signed-off-by: Quanyi Ma <eli@patch.sh>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/utils/util.rs:475
get_commit_base()now prefixes internal errors with"fatal: ..."(e.g."fatal: failed to resolve HEAD","fatal: HEAD does not point to a commit"). Several callers already wrap this error in their own user-facing message (e.g.CliError::fatal(format!("Cannot resolve ...: {e}"))), so this can lead to awkward/duplicated wording like"Cannot resolve 'X': fatal: ...". Consider returning plain, non-prefixed messages from this utility and letting the command layer decide whether/how to addfatal:/ stable codes / hints.
pub async fn get_commit_base(name: &str) -> Result<ObjectHash, String> {
// 1. Check for HEAD
if name.to_uppercase() == "HEAD" {
if let Some(commit_id) = Head::current_commit_result()
.await
.map_err(|error| format!("fatal: failed to resolve HEAD: {error}"))?
{
return Ok(commit_id);
} else {
return Err("fatal: HEAD does not point to a commit".to_string());
}
src/command/show_ref.rs
Outdated
| let tag_list = tag::list().await.map_err(|e| e.to_string())?; | ||
| let tag_list = tag::list() | ||
| .await | ||
| .map_err(|e| CliError::failure(e.to_string()))?; |
There was a problem hiding this comment.
collect_show_ref_entries() maps tag::list() failures to CliError::failure(e.to_string()) without an explicit StableErrorCode or context. This relies on message-based inference and can produce inconsistent error codes compared to the explicit branch/HEAD paths above. Consider wrapping with a clear context message (e.g. "failed to list tags") and setting an explicit stable code (likely RepoCorrupt or IoReadFailed, depending on how internal::tag::list() fails).
| .map_err(|e| CliError::failure(e.to_string()))?; | |
| .map_err(|e| { | |
| CliError::failure(format!("failed to list tags: {e}")) | |
| .with_stable_code(StableErrorCode::IoReadFailed) | |
| })?; |
|
@codex review |
genedna
left a comment
genedna
left a comment
There was a problem hiding this comment.
Follow-up Review — PR #334 (v3, 1 new commit)
Previous Review Issues — Resolution Status
| Issue | Status |
|---|---|
with_repo_current_dir not cancellation-safe (no Drop-based guard) |
✅ Fixed — RepoCurrentDirGuard with RAII Drop impl + explicit restore() |
Non-atomic rename_branch_impl |
Changes in v3
1. RepoCurrentDirGuard RAII guard (local_client.rs:57-97)
Exactly addresses the cancellation safety concern from the v2 review:
change_to()saves the original directory and switches to the repo pathrestore()explicitly restores on the success path, settingrestored = trueDroprestores the directory ifrestore()was never called (cancellation/panic), withtracing::error!on failurewith_repo_current_dirnow uses the guard, with clean error handling: restore failure on success path becomes the error; restore failure on error path is logged and the original error is preserved
The cancellation test is well-crafted — uses oneshot to confirm the operation entered the changed directory, pending::<()>().await to block forever, then handle.abort() + handle.await to trigger Drop and verify cwd restoration.
2. map_local_branch_state_error in clone.rs
Replaces the previous blanket error.to_string() → RepoCorrupt mapping with an exhaustive match on all four BranchStoreError variants. Notably, NotFound now correctly maps to RepoStateInvalid (not RepoCorrupt), which was flagged as a semantic imprecision in reset.rs during the v2 review. The clone.rs implementation sets the right precedent. Two unit tests cover the mapping.
3. branch -a unborn HEAD fix (branch.rs)
Fixes an edge case where branch -a on an unborn repo with remote-tracking refs would suppress the * main marker. The root cause was show_unborn_head checking entries.is_empty() (which includes remote entries) instead of local_branches_empty. The rendering order was also corrected. A new test validates this specific scenario.
Summary
All items from the v2 review are now addressed. The RepoCurrentDirGuard is the standout addition — it’s a textbook RAII implementation with proper cancellation safety and an excellent test. The exhaustive BranchStoreError matching in clone.rs and the unborn-HEAD edge case fix round out a clean revision.
No new issues found. LGTM.
Generated by Claude Code
| /// This supports an explicit `restore()` so callers can surface restore | ||
| /// failures on the success path, while `Drop` still restores the directory if | ||
| /// the surrounding future is cancelled or aborted. | ||
| struct RepoCurrentDirGuard { |
There was a problem hiding this comment.
Exactly what was suggested in the previous review. The Drop-based guard ensures cwd restoration even on async cancellation, while the explicit restore() method lets callers surface I/O failures on the success path. The restored flag avoids a redundant set_current_dir in the normal flow. Clean implementation.
The cancellation test (with_repo_current_dir_restores_current_dir_when_task_is_cancelled) is well-designed — using oneshot to confirm the operation entered the changed directory, then handle.abort() + handle.await to ensure the Drop has fully run before asserting. This directly validates the RAII guarantee.
Generated by Claude Code
| } | ||
| } | ||
|
|
||
| fn map_local_branch_state_error(source: branch::BranchStoreError) -> CliError { |
There was a problem hiding this comment.
Good: The previous review noted that reset.rs mapped BranchStoreError::NotFound to HeadCorrupt via a catch-all arm. Here in clone.rs, all four variants (Query, Corrupt, NotFound, Delete) are explicitly matched with semantically appropriate stable codes (IoReadFailed, RepoCorrupt, RepoStateInvalid, IoWriteFailed). This is the right pattern — NotFound correctly maps to RepoStateInvalid rather than RepoCorrupt. Well done.
Generated by Claude Code
| }); | ||
| } | ||
|
|
||
| let show_unborn_head = local_branches_empty |
There was a problem hiding this comment.
Good fix: The old code checked entries.is_empty() which included both local and remote branches. With branch -a on an unborn repo that has remote-tracking refs, entries would be non-empty (remote refs present), so the unborn HEAD marker * main was suppressed. Now local_branches_empty correctly decouples the unborn-HEAD display from the presence of remote entries.
The rendering change (moving the unborn-head print before the if branches.is_empty() { return Ok(()); } guard) ensures the unborn HEAD is shown even when remote branches follow. The new test test_branch_all_shows_unborn_head_even_with_remote_refs validates this precisely.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f783c2d643
ℹ️ 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".
Signed-off-by: Quanyi Ma <eli@patch.sh>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d73bea0f24
ℹ️ 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".
src/command/reset.rs
Outdated
| other => { | ||
| tracing::error!( | ||
| "rollback after reset failed: {} (primary error: {})", | ||
| rollback_error, | ||
| other | ||
| ); | ||
| other | ||
| } |
There was a problem hiding this comment.
merge_reset_failure now drops rollback errors for primary variants outside IndexSave/HeadUpdate/WorktreeRead/WorktreeRestore and only emits a tracing::error!. In normal CLI runs (without verbose tracing), a failed rollback after a partial reset (for example, an ObjectLoad failure during hard reset) is hidden, so users get only the primary error and no indication that repository recovery also failed. This can leave index/worktree state partially mutated without actionable diagnostics; include rollback context in the returned error for all variants (or wrap both errors in a dedicated composite error).
Useful? React with 👍 / 👎.
genedna
left a comment
genedna
left a comment
There was a problem hiding this comment.
Follow-up Review — PR #334 (v4, 1 new commit)
Changes in v4
A focused update addressing CLI UX and show-ref --tags error classification.
1. Tag argument validation hoisted before repo check (cli.rs, tag.rs)
validate_cli_args is now called in parse_async before the repository-existence check. This ensures that libra tag -d (missing tag name) outside a repo gives a clear usage error rather than "not a Libra repository". The validation order in run_tag is also swapped (validate_named_tag_action before require_repo) for consistency.
The test test_tag_missing_name_usage_outranks_repo_not_found_outside_repo validates this with a negative assertion — good defensive testing.
2. show_ref_tag_list_error with DbErr downcast (show_ref.rs)
Since tag::list() returns an untyped anyhow::Error, the new error handler walks the error chain looking for DbErr to distinguish I/O failures (IoReadFailed) from object corruption (RepoCorrupt). This is a pragmatic workaround — see inline comment for a minor suggestion.
The test test_show_ref_surfaces_corrupt_tag_storage validates this by injecting a corrupt tag reference and asserting LBR-REPO-002.
Code Quality
- Clean, focused changes with no unnecessary scope expansion.
- Both new tests are well-structured with proper assertions on exit code, error code, and message content.
- The dual validation (once in
cli.rs, once inrun_tag) is harmless and keepsrun_tagself-contained.
No New Issues Found
This is a clean polish pass. LGTM.
Generated by Claude Code
| } | ||
| } | ||
|
|
||
| fn show_ref_tag_list_error(error: anyhow::Error) -> CliError { |
There was a problem hiding this comment.
Interesting approach: using error.chain().any(|cause| cause.downcast_ref::<DbErr>().is_some()) to distinguish DB I/O failures from object-level corruption when tag::list() returns an untyped anyhow::Error. This is a pragmatic workaround given that tag::list() doesn't yet return a typed error enum.
One consideration: if tag::list() is ever refactored to return a typed error (like TagStoreError), this downcast heuristic becomes unnecessary. A brief // TODO: comment noting this is a workaround for untyped tag::list() errors would help future readers. Low priority — not blocking.
Generated by Claude Code
| _ => return Err(classify_parse_error(&argv, &err)), | ||
| }, | ||
| }; | ||
| if let Commands::Tag(tag_args) = &args.command { |
There was a problem hiding this comment.
Good UX improvement: hoisting validate_cli_args before the repo-existence check ensures that libra tag -d outside a repository gives a clear usage error ("tag name is required for --delete") instead of the less helpful "not a Libra repository". The test test_tag_missing_name_usage_outranks_repo_not_found_outside_repo validates this ordering explicitly, including a negative assertion that the repo error does not appear.
The corresponding reorder in run_tag (validation before require_repo) is consistent — though now the validation runs twice (once in cli.rs, once in run_tag). The duplication is harmless (it's a pure check with no side effects) and keeps run_tag self-contained for callers that bypass parse_async.
Generated by Claude Code
| Err(error) => { | ||
| tracing::error!( | ||
| repo_path = %self.repo_path.display(), | ||
| restore_dir = %guard.original_dir.display(), | ||
| error = %restore_error, | ||
| "failed to restore working directory after local protocol operation" | ||
| ); | ||
| Err(error) | ||
| } |
There was a problem hiding this comment.
with_repo_current_dir() logs when restore() fails and operation() already returned an error, but RepoCurrentDirGuard will also try to restore again in Drop and log on failure. In the common case where restore keeps failing, this produces duplicate error logs for a single operation. Consider logging in only one place (e.g., remove the explicit log here and rely on Drop, or add a restore_attempted flag so Drop can retry without re-logging).
| Err(error) => { | |
| tracing::error!( | |
| repo_path = %self.repo_path.display(), | |
| restore_dir = %guard.original_dir.display(), | |
| error = %restore_error, | |
| "failed to restore working directory after local protocol operation" | |
| ); | |
| Err(error) | |
| } | |
| Err(error) => Err(error), |
| tag::CreateTagError::CheckExisting(source) => TagError::CheckExistingFailed { | ||
| name: tag_name.to_string(), | ||
| detail: source.to_string(), | ||
| }, |
There was a problem hiding this comment.
CreateTagError::CheckExisting carries a DbErr, but map_create_tag_error() stringifies it into TagError::CheckExistingFailed { detail: String }, which discards the underlying error type/chain. That makes it hard to map query failures to the correct StableErrorCode (e.g., IoReadFailed vs RepoCorrupt) and prevents downstream helpers (like show-ref’s DbErr detection) from working. Consider storing the source error (e.g., DbErr/anyhow::Error) in TagError and mapping stable codes based on the concrete cause.
src/command/tag.rs
Outdated
| async fn collect_tags(show_lines: usize) -> Result<Vec<TagListEntry>, TagError> { | ||
| let tags = tag::list() | ||
| .await | ||
| .map_err(|e| TagError::ListFailed(e.to_string()))?; |
There was a problem hiding this comment.
collect_tags() converts the anyhow::Error from internal::tag::list() into TagError::ListFailed(String) via to_string(), losing the error chain (notably DbErr). This prevents accurate stable-code classification (I/O read vs repo corrupt) and makes debugging harder. Prefer keeping the original error as a source (e.g., ListFailed(#[source] anyhow::Error)), or classify into IoReadFailed/RepoCorrupt here before stringifying.
Signed-off-by: Eli Ma <eli@patch.sh>
|
@codex review |
genedna
left a comment
genedna
left a comment
There was a problem hiding this comment.
Follow-up Review — PR #334 (v5, 1 new commit)
Changes in v5
Three focused improvements addressing error context preservation and error classification accuracy.
1. ResetError::Rollback variant (reset.rs)
Replaces the previous merge_reset_failure approach (string-concatenation into specific variants, with tracing::error! swallowing for non-appendable variants) with a structured Rollback { primary: Box<ResetError>, rollback: Box<ResetError> } variant. This is a significant improvement:
- All error combinations are now preserved — no silent swallowing
stable_code()/hint()/is_command_usage()delegate toprimary, so exit codes and hints are always correct- Display format
"{primary}; rollback failed: {rollback}"gives full diagnostic context
The From<ResetError> for CliError is also refactored from a large per-variant match into a clean delegation through stable_code(), hint(), and is_command_usage() methods. The updated test asserts both the stable code and that the message contains both the primary and rollback error text.
2. TagError typed #[source] annotations (tag.rs)
Replaces detail: String fields with typed #[source] fields (DbErr, GitError, io::Error, anyhow::Error). This preserves the original error chain and enables the classify_tag_read_error function to use DbErr downcasting for precise IoReadFailed vs RepoCorrupt classification. Also fixes CheckExistingFailed from RepoCorrupt to IoReadFailed — correct reclassification since a DB query failure is I/O, not corruption. Three new unit tests validate the classification.
3. RepoCurrentDirGuard duplicate-logging prevention (local_client.rs)
Adds restore_failure_logged flag to prevent Drop from re-logging a restore failure that with_repo_current_dir already logged on the error path. Small but thoughtful refinement.
4. show_ref.rs TODO comment — Added per v4 review suggestion, noting the DbErr-chain heuristic is a workaround pending a typed tag::list() error.
Code Quality
- The
ResetErrorimpl methods (stable_code,hint,is_command_usage) are clean and exhaustive. TheRollbackvariant delegates correctly in all three. - The
TagErrorsource type changes are backwards-compatible —map_create_tag_errornow passes through the original source directly instead of.to_string(). render_tagsnow usesanyhow::Error::frominstead ofanyhow::anyhow!(e.to_string())for proper error chain preservation.
No New Issues Found
This is a quality-focused polish pass that improves error fidelity throughout. LGTM.
Generated by Claude Code
| #[error("pathspec '{0}' did not match any file(s) known to libra")] | ||
| PathspecNotMatched(String), | ||
|
|
||
| #[error("{primary}; rollback failed: {rollback}")] |
There was a problem hiding this comment.
Excellent refactor. The Rollback variant with Box<ResetError> for both primary and rollback errors replaces the previous merge_reset_failure which string-concatenated into specific variants or silently logged via tracing::error! for non-appendable ones. Now:
- All error combinations are preserved — no more
tracing::error!swallowing the rollback error forObjectLoador other variants. stable_code()/hint()/is_command_usage()all delegate toprimary, so the user sees the root cause's exit code and hint.- The
Displayformat"{primary}; rollback failed: {rollback}"gives full context in a single error message.
The refactored From<ResetError> for CliError is also much cleaner — stable_code(), hint(), and is_command_usage() methods eliminate the large match block with repetitive .with_stable_code() / .with_hint() chains.
Generated by Claude Code
| #[error("Cannot create tag: HEAD does not point to a commit")] | ||
| HeadUnborn, | ||
|
|
||
| #[error("failed to read existing tags before creating '{name}': {source}")] |
There was a problem hiding this comment.
Good improvement: replacing detail: String with typed #[source] fields (DbErr, GitError, io::Error, anyhow::Error) preserves the original error chain. This enables the classify_tag_read_error function to use error.chain().any(|cause| cause.downcast_ref::<DbErr>().is_some()) for precise error classification.
Also note the CheckExistingFailed stable code fix from RepoCorrupt to IoReadFailed — a DB query failure during the existence check is an I/O issue, not corruption. Correct reclassification.
Generated by Claude Code
| /// This supports an explicit `restore()` so callers can surface restore | ||
| /// failures on the success path, while `Drop` still restores the directory if | ||
| /// the surrounding future is cancelled or aborted. | ||
| struct RepoCurrentDirGuard { |
There was a problem hiding this comment.
Nice attention to detail: the restore_failure_logged flag prevents duplicate error logging when with_repo_current_dir already logged the restore failure on the error path and then Drop fires again. Without this, users would see the same tracing::error! message twice. Clean solution.
Generated by Claude Code
| let repo = create_committed_repo_via_cli(); | ||
| let db_path = repo.path().join(".libra").join("libra.db"); | ||
| let original_mode = fs::metadata(&db_path).unwrap().permissions().mode(); | ||
|
|
||
| fs::set_permissions(&db_path, std::fs::Permissions::from_mode(0o444)).unwrap(); | ||
| let output = run_libra_command(&["branch", "--set-upstream-to", "origin/main"], repo.path()); | ||
| fs::set_permissions(&db_path, std::fs::Permissions::from_mode(original_mode)).unwrap(); |
There was a problem hiding this comment.
This test relies on chmod 0444 to force a SQLite write failure, but on self-hosted CI runners the test process may run as root (or with elevated capabilities), in which case writes can still succeed and the assertion becomes flaky. Consider skipping the permission-based assertion when effective UID==0, or use a deterministic failure injection approach (e.g., read-only DB connection / mocked persistence) so the test outcome is independent of runner privileges.
| let db_path = repo.path().join(".libra").join("libra.db"); | ||
| let original_mode = fs::metadata(&db_path).unwrap().permissions().mode(); | ||
|
|
||
| fs::set_permissions(&db_path, std::fs::Permissions::from_mode(0o444)).unwrap(); | ||
| let second = run_libra_command(&["branch", "--set-upstream-to", "origin/main"], repo.path()); | ||
| fs::set_permissions(&db_path, std::fs::Permissions::from_mode(original_mode)).unwrap(); | ||
|
|
There was a problem hiding this comment.
This idempotency test again depends on chmod 0444 to ensure a redundant config write would fail. If the runner executes tests as root, the write may still succeed and the test won’t be exercising the intended failure path, which can make CI results environment-dependent. Consider guarding the test under effective UID!=0 or refactoring to assert idempotency without filesystem permission assumptions.
| let db_path = repo_dir.join(".libra").join("libra.db"); | ||
| let original_mode = fs::metadata(&db_path).unwrap().permissions().mode(); | ||
| fs::set_permissions(&db_path, std::fs::Permissions::from_mode(0o444)).unwrap(); | ||
| let output = run_libra_command(&["remote", "prune", "origin"], &repo_dir); | ||
| fs::set_permissions(&db_path, std::fs::Permissions::from_mode(original_mode)).unwrap(); | ||
|
|
There was a problem hiding this comment.
This test forces an I/O error by chmod-ing the SQLite DB to 0444. If tests run as root on self-hosted CI, the prune may still be able to write/delete and the test becomes flaky or ineffective. Consider skipping when effective UID==0 or using a deterministic failure injection that doesn’t depend on Unix permission enforcement.
No description provided.