Make state status smarter#54
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds reconciliation APIs that validate/update persisted Kindra rebase state against Git, replaces direct saved-state-file checks with passive reconciliation in command guards, updates continue/status to use reconciled state, tweaks commit autosquash state recording, and updates/adds tests. ChangesPassive rebase state reconciliation
Sequence Diagram(s)sequenceDiagram
participant User
participant ContinueCmd as continue_cmd
participant StatusCmd as status_cmd
participant Reconciler as reconcile_saved_rebase_state
participant Git as Git
User->>ContinueCmd: kin continue
ContinueCmd->>Reconciler: reconcile_saved_rebase_state(repo, Continue)
Reconciler->>Git: inspect commits, refs, active rebase dirs
alt returns Some(state)
Reconciler-->>ContinueCmd: Ok(Some(state))
ContinueCmd->>User: resume using reconciled state
else returns None
Reconciler-->>ContinueCmd: Ok(None)
ContinueCmd-->>User: "No operation in progress."
end
User->>StatusCmd: kin status
StatusCmd->>Reconciler: reconcile_saved_rebase_state(repo, Passive)
Reconciler->>Git: inspect commits, refs, active rebase dirs
alt Err(parse/mismatch)
Reconciler-->>StatusCmd: Err(...)
StatusCmd-->>User: propagate error
else Ok(Some)
StatusCmd-->>User: show operation state
else Ok(None)
StatusCmd-->>User: "No Kindra operation active."
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/status_cmd.rs`:
- Around line 32-44: The code is incorrectly treating any error from
reconcile_saved_rebase_state(&repo, ReconcileMode::Passive) (and subsequent
load_state) as equivalent to “no state” and prints "No Kindra operation
active."; instead, only the explicit Ok(None) should result in that message—any
Err(_) from reconcile_saved_rebase_state or load_state must be propagated
(returned) so callers can handle corruption/unreadable state. Update the match
in status_cmd.rs around reconcile_saved_rebase_state to: on Ok(Some(state)) use
it, on Ok(None) print "No Kindra operation active." and return Ok(()), but on
Err(e) return Err(e) (or map to the function's error type) rather than falling
back to load_state; if you still call load_state, propagate its Err instead of
treating it as no-op. Ensure you reference reconcile_saved_rebase_state,
ReconcileMode::Passive, and load_state when making the change.
In `@src/rebase_utils.rs`:
- Around line 178-182: The Err arm of the match on
reconcile_saved_rebase_state(repo, ReconcileMode::Passive) is swallowing the
error and returning Ok(true); update that branch to log or record the error
before returning so the underlying cause is visible—capture the Err(e) and call
your logger (e.g., process_logger.error or tracing::error) with a descriptive
message including e, then return Ok(true) as before; reference
reconcile_saved_rebase_state and ReconcileMode::Passive to locate the match
expression to modify.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 42e78b6e-3289-4a0b-9efc-60bd3c52ccba
📒 Files selected for processing (10)
src/commands/commit.rssrc/commands/continue_cmd.rssrc/commands/move_cmd.rssrc/commands/reorder.rssrc/commands/restack.rssrc/commands/run.rssrc/commands/status_cmd.rssrc/commands/sync.rssrc/rebase_utils.rstests/move_tests.rs
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/continue_cmd.rs`:
- Around line 45-48: The code checks state_path(&repo).exists() and may run `git
rebase --continue` before reconciliation; call
reconcile_saved_rebase_state(&repo, ReconcileMode::Continue) first and use its
returned Option to decide whether to invoke any native git rebase actions.
Specifically, remove or bypass the earlier state_path(&repo).exists() guard and
instead move the reconcile_saved_rebase_state call to the top of the flow,
ensure only when it returns Some(state) you call the logic that runs `git rebase
--continue`, and update any branches that previously assumed existence to rely
on the reconciled state; add a permanent test that writes a stale/malformed
Kindra state file and asserts that `kin continue` does not advance an unrelated
native rebase (i.e., no native `git rebase --continue` is executed) unless
reconcile_saved_rebase_state returns Some.
In `@src/rebase_utils.rs`:
- Around line 402-418: can_passively_clear_completed_state currently refuses to
clear completed state unless HEAD matches the saved
caller_branch/original_branch; change it so that when the saved state.operation
== Operation::Sync it does not require HEAD==caller_branch (i.e., allow
passively clearing if stash_ref/is_unstage_on_restore/cleanup_merged_branches
checks are already satisfied), while preserving existing guards for other
operations; update the function to inspect state.operation and bypass the
branch-name check for Operation::Sync (use the
can_passively_clear_completed_state function and the Operation::Sync enum
variant to locate where to change behavior), and add an integration test under
tests/... that reproduces the manual `git rebase --continue` + `--no-delete`
flow (start sync from a non-tip branch, simulate manual continue so
remaining_branches becomes [], and verify the repo state clears without
requiring `kin continue`) to ensure the regression is covered.
In `@tests/commit_tests.rs`:
- Around line 805-810: The test currently asserts the exact serde_json message
"key must be a string" for kin_cmd().arg("status") which is brittle; change the
assertion to avoid tying to serde_json wording by either (a) asserting failure
plus a stable Kindra-level message prefix (e.g. assert
.stderr(predicates::str::contains("failed to parse state") or "error parsing
state")) or (b) remove the exact stderr match and instead assert the command
.failure() and that the repository state file was preserved (read the state file
before/after and compare) so the test verifies behavior without depending on
serde_json's phrasing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: a64e6f84-540e-47f1-adb8-ea792a631e64
📒 Files selected for processing (11)
src/commands/commit.rssrc/commands/continue_cmd.rssrc/commands/move_cmd.rssrc/commands/reorder.rssrc/commands/restack.rssrc/commands/run.rssrc/commands/status_cmd.rssrc/commands/sync.rssrc/rebase_utils.rstests/commit_tests.rstests/move_tests.rs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/rebase_utils.rs (1)
128-130:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftValidate rebase ownership before returning saved state.
This treats any active Git rebase as Kindra-owned and returns
Some(state)without checking that the native rebase actually matches the persisted operation. A stale but parseablegits_rebase_state.jsoncan still makekin continueadvance an unrelated native rebase and then resume the wrong Kindra workflow. This needs an ownership check against Git’s rebase metadata before trusting the saved state here, plus a permanent regression test for the stale-state/native-rebase case. As per coding guidelines,tests/**/*.rs:Any identified bug or edge case (e.g., panics, incorrect state) MUST be reproduced with a permanent test case before the fix is applied; do not use temporary "repro" filenames, integrate them into the relevant test suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rebase_utils.rs` around lines 128 - 130, The current code returns Some(state) whenever git_rebase_in_progress(repo) is true without verifying that the persisted Kindra rebase state (loaded by load_state) actually corresponds to the native Git rebase metadata; update the logic in the function that calls load_state and git_rebase_in_progress to perform an ownership check (e.g., compare the persisted operation ID/branch/commit metadata in the state struct against Git's rebase metadata from the repo before returning Some(state)), and only return Some(state) when they match; if they do not match, treat it as no Kindra rebase (return None) and document/clean up the stale gits_rebase_state.json; add a permanent regression test under tests/ (following existing test patterns) that creates a stale-but-parseable gits_rebase_state.json and an unrelated native rebase, runs the continue flow (kin continue) and asserts that the Kindra state is not applied to the unrelated native rebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/restack.rs`:
- Around line 32-33: The error message returned from the guard using
passively_reconcile_rebase_state is too specific; update the Err returned where
passively_reconcile_rebase_state(&repo)? is checked to use a broader message
mentioning an active Kindra-managed operation and instructing the user to run
the recovery commands (e.g., "kin continue" or "kin abort") rather than
referring only to a "rebase operation"; modify the return in restack.rs to
produce that generalized guidance so it applies for moves, syncs, or rebases
managed by Kindra.
In `@tests/move_tests.rs`:
- Around line 300-390: The test
test_move_manual_git_continue_then_kin_continue_resumes_next_branch installs a
shell-based git wrapper but unconditionally constructs PATH with ":" which
breaks on Windows; gate the test to Unix by adding #[cfg(unix)] (or
#[cfg_attr(not(unix), ignore)] if you prefer an explicit ignore) to the test
function so the wrapper setup (git_wrapper, real_git, new_path) and the PATH
modification only run on Unix; locate the test by name and update its
declaration above fn
test_move_manual_git_continue_then_kin_continue_resumes_next_branch.
---
Duplicate comments:
In `@src/rebase_utils.rs`:
- Around line 128-130: The current code returns Some(state) whenever
git_rebase_in_progress(repo) is true without verifying that the persisted Kindra
rebase state (loaded by load_state) actually corresponds to the native Git
rebase metadata; update the logic in the function that calls load_state and
git_rebase_in_progress to perform an ownership check (e.g., compare the
persisted operation ID/branch/commit metadata in the state struct against Git's
rebase metadata from the repo before returning Some(state)), and only return
Some(state) when they match; if they do not match, treat it as no Kindra rebase
(return None) and document/clean up the stale gits_rebase_state.json; add a
permanent regression test under tests/ (following existing test patterns) that
creates a stale-but-parseable gits_rebase_state.json and an unrelated native
rebase, runs the continue flow (kin continue) and asserts that the Kindra state
is not applied to the unrelated native rebase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59d98cc6-c38b-4efb-80b6-f0714df09fd7
📒 Files selected for processing (12)
src/commands/commit.rssrc/commands/continue_cmd.rssrc/commands/move_cmd.rssrc/commands/reorder.rssrc/commands/restack.rssrc/commands/run.rssrc/commands/status_cmd.rssrc/commands/sync.rssrc/rebase_utils.rstests/commit_tests.rstests/move_tests.rstests/sync_tests.rs
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/sync.rs (1)
40-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlso block
syncwhen a run state is persisted.This guard only checks rebase state, so
kin synccan start whilekindra_run_state.jsonstill exists and mutate the repo mid-kin run.run()already treats either persisted state as exclusive, sosync()should mirror that here.Suggested fix
- if passively_reconcile_rebase_state(&repo)? { + if passively_reconcile_rebase_state(&repo)? + || crate::commands::run::run_state_exists(&repo) + { return Err(anyhow!( "A Kindra operation is already in progress. Use 'kin continue' or 'kin abort'." )); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/sync.rs` around lines 40 - 45, The sync command only checks rebase state via passively_reconcile_rebase_state(&repo) but must also guard against a persisted Kindra run state (the presence of kindra_run_state.json) the same way run() does; add a check before proceeding that detects the persisted run state (either by calling the same helper used by run() or by detecting kindra_run_state.json in the repo) and return the same Err(anyhow!("A Kindra operation is already in progress. Use 'kin continue' or 'kin abort'.")) if it exists, keeping the existing ensure_no_native_git_operation(&repo)? call after the new guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rebase_utils.rs`:
- Around line 403-410: The function active_git_rebase_matches_state currently
calls active_git_rebase_branch(repo)? and then falls back to
owned_tip_state_matches(repo, state) even when active_git_rebase_branch returns
Some(other branch), which can incorrectly claim an unrelated native rebase
matches our state; change active_git_rebase_matches_state so that if
active_git_rebase_branch(repo)? returns Some(active_branch) you only return
Ok(true) when state.in_progress_branch matches active_branch and otherwise
return Ok(false), and only call owned_tip_state_matches(repo, state) when
active_git_rebase_branch(repo)? returns None; reference
active_git_rebase_branch, owned_tip_state_matches,
active_git_rebase_matches_state, and RebaseState to locate the logic to update.
---
Outside diff comments:
In `@src/commands/sync.rs`:
- Around line 40-45: The sync command only checks rebase state via
passively_reconcile_rebase_state(&repo) but must also guard against a persisted
Kindra run state (the presence of kindra_run_state.json) the same way run()
does; add a check before proceeding that detects the persisted run state (either
by calling the same helper used by run() or by detecting kindra_run_state.json
in the repo) and return the same Err(anyhow!("A Kindra operation is already in
progress. Use 'kin continue' or 'kin abort'.")) if it exists, keeping the
existing ensure_no_native_git_operation(&repo)? call after the new guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: e973e66d-5954-4715-8f8f-1571662e6463
📒 Files selected for processing (12)
src/commands/commit.rssrc/commands/continue_cmd.rssrc/commands/move_cmd.rssrc/commands/reorder.rssrc/commands/restack.rssrc/commands/run.rssrc/commands/status_cmd.rssrc/commands/sync.rssrc/rebase_utils.rstests/commit_tests.rstests/move_tests.rstests/sync_tests.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/commit.rs (1)
230-261:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't mark autosquash as in progress before Git actually enters rebase.
Line 233 persists
in_progress_branchbeforegit rebase -i --autosquashruns. If that command fails before creating.git/rebase-*, this branch returns with a saved stash/state that now looks actively in progress even though no rebase ever started. That strands the hidden worktree changes and forces users into a bogus continue/abort flow.Mirror the pre-start failure handling in
src/commands/sync.rs: only setin_progress_branchoncegit_rebase_in_progress(&repo)is true, or unwind the saved stash/state on the non-started failure path.Suggested fix
if needs_autosquash { if autosquash_state_required { state.stash_ref = stash_non_staged_changes()?; - state.in_progress_branch = Some(target_branch.clone()); save_state(&repo, &state)?; } @@ let status = cmd.status()?; if !status.success() { - if !pre_commit_state_required && git_rebase_in_progress(&repo) { - save_state(&repo, &state)?; + if !pre_commit_state_required { + if git_rebase_in_progress(&repo) { + state.in_progress_branch = Some(target_branch.clone()); + save_state(&repo, &state)?; + } else { + if let Some(stash_ref) = state.stash_ref.clone() { + apply_stash(&stash_ref)?; + state.stash_ref = None; + if let Err(err) = drop_stash(&stash_ref) { + eprintln!("Warning: {}", err); + } + } + clear_state(&repo)?; + } } return Err(anyhow!( "git rebase --autosquash failed. Resolve conflicts and run 'kin continue', or run 'kin abort'." )); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/commit.rs` around lines 230 - 261, The code currently sets state.in_progress_branch and saves state before running git rebase --autosquash, which can leave a saved stash/state if rebase never actually starts; change the flow in the needs_autosquash branch so you only set state.in_progress_branch and call save_state() after confirming the rebase started (use git_rebase_in_progress(&repo) after running the Command), or if you must save state before running git, stash changes but do NOT persist in_progress_branch yet; on non-start failure (status not success and git_rebase_in_progress(&repo) is false) unwind the earlier stash/state by clearing state.stash_ref and state.in_progress_branch and restoring/ dropping the stash as appropriate, then save_state(&repo, &state) to persist the unwound state; refer to stash_non_staged_changes(), save_state(), in_progress_branch, stash_ref, git_rebase_in_progress(), and the autosquash Command block to locate and update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rebase_utils.rs`:
- Around line 364-401: Add a short doc comment above the branch_rebase_completed
function describing the three-stage completion check: (1) the
descendant/equality check using branch_rebase_target and
repo.graph_descendant_of/current_id == new_base_id, (2) the
original-commit-count guard that compares collect_first_parent_chain length
against state.original_commit_count_map to avoid hidden extra commits, and (3)
the first-parent verification that walks commits (repo.revwalk, hide
new_base_id) and ensures the first commit's parent (first_commit.parent_id(0))
is the expected new_base; mention the purpose of each stage and what edge case
it protects against so future maintainers understand why all three are required.
---
Outside diff comments:
In `@src/commands/commit.rs`:
- Around line 230-261: The code currently sets state.in_progress_branch and
saves state before running git rebase --autosquash, which can leave a saved
stash/state if rebase never actually starts; change the flow in the
needs_autosquash branch so you only set state.in_progress_branch and call
save_state() after confirming the rebase started (use
git_rebase_in_progress(&repo) after running the Command), or if you must save
state before running git, stash changes but do NOT persist in_progress_branch
yet; on non-start failure (status not success and git_rebase_in_progress(&repo)
is false) unwind the earlier stash/state by clearing state.stash_ref and
state.in_progress_branch and restoring/ dropping the stash as appropriate, then
save_state(&repo, &state) to persist the unwound state; refer to
stash_non_staged_changes(), save_state(), in_progress_branch, stash_ref,
git_rebase_in_progress(), and the autosquash Command block to locate and update
the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73350c9b-e5a2-4be6-a4f1-0174608503b0
📒 Files selected for processing (12)
src/commands/commit.rssrc/commands/continue_cmd.rssrc/commands/move_cmd.rssrc/commands/reorder.rssrc/commands/restack.rssrc/commands/run.rssrc/commands/status_cmd.rssrc/commands/sync.rssrc/rebase_utils.rstests/commit_tests.rstests/move_tests.rstests/sync_tests.rs
Pajn
force-pushed
the
smarter-state
branch
2 times, most recently
from
f0e2843 to
fc131b0
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/commit.rs`:
- Around line 260-267: The current sequence applies the stash via apply_stash
before persisting the state change, so if save_state fails you can re-apply the
same stash later; to fix, perform a two-phase update around state.stash_ref:
first mutate the in-memory state to mark the stash as being restored (e.g.,
clear state.stash_ref or set a "restoring" flag) and call save_state(&repo,
&state) to make that durable, then call apply_stash(&stash_ref) and, on success,
finalize by clearing any in_progress markers, calling drop_stash(&stash_ref) and
saving state again; reference apply_stash, drop_stash, save_state,
state.stash_ref and state.in_progress_branch when making this change.
In `@src/rebase_utils.rs`:
- Around line 129-135: The code currently clears Kindra state and returns
Ok(None) when git_rebase_in_progress(repo) is true but
active_git_rebase_matches_state(repo, &state) is false; instead, stop deleting
state and surface a blocking error so the foreign rebase cannot be layered over
(do not call clear_state(repo) or return Ok(None)). Modify the branch under
git_rebase_in_progress to return a suitable Err (or a Result::Err with a clear
mismatch error) and leave state intact (preserve stash_ref and cleanup fields),
so callers of this logic in commit.rs/move_cmd.rs/restack.rs will detect the
mismatch; also add a permanent test in tests/**/*.rs that reproduces this
mismatch case before applying the fix.
In `@tests/sync_tests.rs`:
- Around line 1019-1101: The test currently only asserts feature-a exists and
the Kindra state file is cleared, but must also verify the upper branch
finished; modify
sync_no_delete_manual_continue_from_non_tip_branch_clears_passively to capture
the commit id when creating the feature-b commit (assign the result of
make_commit for "refs/heads/feature-b" to a variable like b_id) and after the
manual git rebase --continue and status checks add an assertion that the local
branch "feature-b" exists and its tip equals the captured b_id (i.e., open the
Repository and verify find_branch("feature-b", BranchType::Local) points to the
expected commit) so the test fails if the upper branch was dropped or not
updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1bc3ba8-b283-46b3-933c-f6a427a01c2d
📒 Files selected for processing (14)
src/commands/commit.rssrc/commands/continue_cmd.rssrc/commands/move_cmd.rssrc/commands/reorder.rssrc/commands/restack.rssrc/commands/run.rssrc/commands/status_cmd.rssrc/commands/sync.rssrc/rebase_utils.rstests/commit_tests.rstests/move_tests.rstests/reorder_tests.rstests/split_tests.rstests/sync_tests.rs
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/commit_tests.rs (1)
1132-1155:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a valid saved-state fixture in the reentry-guard test.
commitnow goes through passive reconciliation before it decides an operation is already active. Writing{}here no longer models “operation in progress”; it models a malformed Kindra state file, so this test can fail for the wrong reason. Seed a parseableRebaseStateinstead, e.g. via the existing helper.Suggested fix
#[test] fn test_commit_reentry_guard() { - let (dir, _repo) = setup_repo(); + let (dir, repo) = setup_repo(); let state_path = dir.path().join(".git/kindra_rebase_state.json"); // Create the state file to simulate an ongoing operation - fs::write(&state_path, "{}").unwrap(); + write_commit_rebase_state_fixture(&repo, "stash@{0}"); // Attempt to run kin commit let mut cmd = kin_cmd();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/commit_tests.rs` around lines 1132 - 1155, The test currently writes an empty JSON ("{}") to state_path which is no longer interpreted as “operation in progress” but as a malformed Kindra state; replace the fs::write(&state_path, "{}").unwrap() call with code that seeds a valid, parseable RebaseState into state_path using the existing test helper for saved-state fixtures (i.e., use the project’s helper that creates/writes a valid RebaseState fixture so the reentry-guard sees an active operation), keeping the rest of the test (state_path, kin_cmd(), and assertions) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rebase_utils.rs`:
- Around line 74-76: The repo state filename was renamed in state_path (returns
".git/kindra_rebase_state.json") but code paths that check for an existing
active operation (calls that rely on state_path, lines around functions
referencing state_path at 74, 124, 179) must also look for the legacy filename
and migrate or treat it as present; update state_path or add a helper (e.g.,
find_or_migrate_state_path) to probe for the legacy path (the previous filename)
and if found either return that path or copy/rename it to the new name so the
existence checks in commit/move/reorder/restack still detect ongoing operations,
and add a permanent unit/integration test under tests/** to reproduce the
legacy-file scenario and validate migration/recognition before committing the
fix.
---
Outside diff comments:
In `@tests/commit_tests.rs`:
- Around line 1132-1155: The test currently writes an empty JSON ("{}") to
state_path which is no longer interpreted as “operation in progress” but as a
malformed Kindra state; replace the fs::write(&state_path, "{}").unwrap() call
with code that seeds a valid, parseable RebaseState into state_path using the
existing test helper for saved-state fixtures (i.e., use the project’s helper
that creates/writes a valid RebaseState fixture so the reentry-guard sees an
active operation), keeping the rest of the test (state_path, kin_cmd(), and
assertions) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb3fe96c-bb45-42b0-a2cf-4c0f7583b390
📒 Files selected for processing (14)
src/commands/commit.rssrc/commands/continue_cmd.rssrc/commands/move_cmd.rssrc/commands/reorder.rssrc/commands/restack.rssrc/commands/run.rssrc/commands/status_cmd.rssrc/commands/sync.rssrc/rebase_utils.rstests/commit_tests.rstests/move_tests.rstests/reorder_tests.rstests/split_tests.rstests/sync_tests.rs
Comment on lines
74
to
76
| pub fn state_path(repo: &Repository) -> PathBuf { | ||
| repo.path().join("gits_rebase_state.json") | ||
| repo.path().join("kindra_rebase_state.json") | ||
| } |
There was a problem hiding this comment.
Keep backward compatibility for the previous state filename.
After this rename, both reconciliation entrypoints treat the repo as clean whenever .git/kindra_rebase_state.json is absent. If a user upgrades while an older Kindra operation is still persisted under the previous filename, the guard will miss it and commit/move/reorder/restack can start a second operation on top of unfinished recovery state. Please probe or migrate the legacy state file in these existence checks before declaring the repo idle.
As per coding guidelines, tests/**/*.rs: Any identified bug or edge case (e.g., panics, incorrect state) MUST be reproduced with a permanent test case before the fix is applied; do not use temporary "repro" filenames, integrate them into the relevant test suite.
Also applies to: 124-126, 179-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rebase_utils.rs` around lines 74 - 76, The repo state filename was
renamed in state_path (returns ".git/kindra_rebase_state.json") but code paths
that check for an existing active operation (calls that rely on state_path,
lines around functions referencing state_path at 74, 124, 179) must also look
for the legacy filename and migrate or treat it as present; update state_path or
add a helper (e.g., find_or_migrate_state_path) to probe for the legacy path
(the previous filename) and if found either return that path or copy/rename it
to the new name so the existence checks in commit/move/reorder/restack still
detect ongoing operations, and add a permanent unit/integration test under
tests/** to reproduce the legacy-file scenario and validate
migration/recognition before committing the fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
New Features
Tests