Allow creating kindra temp worktrees#5
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a new ChangesKindra Temp Worktree Feature
Release Script wisp-embers Version Bump
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant run_surface
participant KindraProvider
participant Session
User->>run_surface: types in worktree mode
run_surface->>KindraProvider: temp_worktrees_configured(repo_root)
KindraProvider-->>run_surface: configured
run_surface->>run_surface: append CreateTempWorktree row
User->>run_surface: selects CreateTempWorktree
run_surface->>KindraProvider: create_temp_worktree(repo_root, branch, trunk)
KindraProvider-->>run_surface: worktree path
run_surface->>Session: create/switch session
User->>run_surface: closes session-less temp worktree
run_surface->>KindraProvider: remove_temp_worktree(repo_root, branch)
KindraProvider-->>run_surface: removed
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/release.sh (1)
82-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate version-bump logic between workspace and embers heredocs.
The new embers python block (Lines 110-131) mirrors the existing workspace-bump block (Lines 82-108) almost line-for-line — same regex-and-write pattern, differing only in section header, expected match count, and error message. Consider extracting a shared helper (bash function or a single parameterized python script) that takes
(file_path, section_header, exact_match_required)to avoid future drift between the two blocks.♻️ Example consolidation approach
- python3 - "${CARGO_TOML}" "${version}" <<'PY' -import pathlib -import re -import sys - -path = pathlib.Path(sys.argv[1]) -version = sys.argv[2] -text = path.read_text() -updated, count = re.subn( - r'(?ms)^(\[workspace\.package\]\n.*?^version = ")([^"]+)(")$', - rf'\g<1>{version}\3', - text, - count=1, -) -if count != 1: - raise SystemExit("failed to update workspace.package.version") - -updated, count = re.subn( - r'(?m)^((?:wisp(?:-[a-z]+)?) = \{ version = ")([^"]+)(", path = "crates/[^"]+" \})$', - rf'\g<1>{version}\3', - updated, -) -if count == 0: - raise SystemExit("failed to update internal workspace dependency versions") - -path.write_text(updated) -PY - - python3 - "${EMBERS_CARGO_TOML}" "${version}" <<'PY' -import pathlib -import re -import sys - -path = pathlib.Path(sys.argv[1]) -version = sys.argv[2] -text = path.read_text() -updated, count = re.subn( - r'(?ms)^(\[package\]\n.*?^version = ")([^"]+)(")$', - rf'\g<1>{version}\3', - text, - count=1, -) -if count != 1: - raise SystemExit("failed to update wisp-embers package version") - -path.write_text(updated) -PY + python3 - "${CARGO_TOML}" "${EMBERS_CARGO_TOML}" "${version}" <<'PY' +import pathlib +import re +import sys + +def bump_section(path, header, version, expected): + text = path.read_text() + updated, count = re.subn( + rf'(?ms)^(\{header}\n.*?^version = ")([^"]+)(")$', + rf'\g<1>{version}\3', + text, + count=1, + ) + if count != expected: + raise SystemExit(f"failed to update {header} version in {path}") + path.write_text(updated) + +cargo_toml, embers_toml, version = pathlib.Path(sys.argv[1]), pathlib.Path(sys.argv[2]), sys.argv[3] +bump_section(cargo_toml, r"\[workspace\.package\]", version, 1) + +text = cargo_toml.read_text() +updated, count = re.subn( + r'(?m)^((?:wisp(?:-[a-z]+)?) = \{ version = ")([^"]+)(", path = "crates/[^"]+" \})$', + rf'\g<1>{version}\3', + text, +) +if count == 0: + raise SystemExit("failed to update internal workspace dependency versions") +cargo_toml.write_text(updated) + +bump_section(embers_toml, r"\[package\]", version, 1) +PY🤖 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 `@scripts/release.sh` around lines 82 - 131, The release script has duplicated version-update logic in the workspace and embers heredoc blocks, so consolidate the repeated regex/read/write flow into a single shared helper or parameterized Python script. Refactor the logic around the existing version bump steps in the release script so one reusable path handles both the workspace.package update and the wisp-embers package update, with parameters for the target file, section header, and required match count to keep the behavior aligned.
🤖 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 `@crates/wisp-bin/src/main.rs`:
- Around line 1079-1086: The temporary branch name helper only normalizes
whitespace, so invalid git branch names can still reach the temp-worktree flow
and fail later. Update kindra_temp_branch_name, or add a guard before
create_temp_worktree, to validate the generated branch name against git branch
rules and return None/reject the action for names like fix: crash, foo..bar, or
foo.lock before constructing the CreateTempWorktree row.
In `@crates/wisp-kindra/src/lib.rs`:
- Around line 122-138: The `Command::output()` error handling in `Kindra::run`
is misclassifying non-NotFound spawn failures as `KindraError::CommandFailed`,
even though no process stderr exists. Update the `map_err` branch in
`crates/wisp-kindra/src/lib.rs` to map IO errors other than
`ErrorKind::NotFound` to `KindraError::SpawnFailed` instead of `CommandFailed`,
and keep `CommandFailed` reserved for actual process execution failures with
real stderr from the command.
In `@README.md`:
- Line 90: Documentation for temporary worktree creation is out of sync with the
actual worktree mode behavior. Update the README section describing the Kindra
temp worktree flow so it states that the typed filter text is normalized to a
branch slug (whitespace becomes dashes) and that the trunk is resolved from the
remote default branch before falling back to main/master. Keep the wording
aligned with the worktree mode flow that appends the ＋ row and runs kin wt temp,
but remove the claim that the raw filter text is used directly as the branch
name.
---
Outside diff comments:
In `@scripts/release.sh`:
- Around line 82-131: The release script has duplicated version-update logic in
the workspace and embers heredoc blocks, so consolidate the repeated
regex/read/write flow into a single shared helper or parameterized Python
script. Refactor the logic around the existing version bump steps in the release
script so one reusable path handles both the workspace.package update and the
wisp-embers package update, with parameters for the target file, section header,
and required match count to keep the behavior aligned.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79f47d6d-5de5-4b01-a8bc-dbcac4d12b7b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlREADME.mdcrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/git.rscrates/wisp-bin/src/main.rscrates/wisp-core/src/view.rscrates/wisp-kindra/Cargo.tomlcrates/wisp-kindra/src/lib.rscrates/wisp-kindra/tests/integration.rscrates/wisp-ui/src/lib.rsscripts/release.sh
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 `@crates/wisp-bin/src/main.rs`:
- Around line 1157-1172: The CreateTempWorktree activation path in
activate_filter_selection is recomputing the trunk instead of reusing the
already-resolved value from KindraTempContext.trunk built in run_surface. Thread
the cached trunk through the SessionListItem/command_hint (or add a dedicated
field) and use that value when calling kindra.create_temp_worktree, so the
preview and activation share the same trunk, avoid duplicate git::trunk_branch
calls, and make activate_filter_selection_creates_kindra_temp_worktree test the
real resolved branch instead of the fallback.
In `@crates/wisp-kindra/src/lib.rs`:
- Around line 66-91: Add a timeout to the Kindra subprocess calls so a stalled
git/kin process cannot block the flow forever. Update the process launch in
git_common_dir and create_temp_worktree to use a bounded wait or cancellable
worker pattern instead of unbounded Command::output(), and make sure the timeout
path returns a clean None/error through the same callers that currently handle
subprocess failure.
- Around line 101-120: The create_temp_worktree boundary currently only trims
whitespace, so dash-prefixed branch names like -foo can still be treated as
flags by the git/clap command path. Update create_temp_worktree in wisp-kindra
to reject branch names that start with -, alongside the existing empty check,
and return KindraError::InvalidBranch from that path so direct callers cannot
pass a misparsed branch name.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26e573f9-91e7-4d82-8876-de71f39b80d5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlREADME.mdcrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/git.rscrates/wisp-bin/src/main.rscrates/wisp-core/src/view.rscrates/wisp-kindra/Cargo.tomlcrates/wisp-kindra/src/lib.rscrates/wisp-kindra/tests/integration.rscrates/wisp-ui/src/lib.rsscripts/release.sh
Comment on lines
+66
to
+91
| /// Resolves the absolute git common directory for `repo_root`, where shared | ||
| /// repository state (including `kindra.toml`) lives for linked worktrees. | ||
| fn git_common_dir(&self, repo_root: &Path) -> Option<PathBuf> { | ||
| let output = Command::new(&self.git_binary) | ||
| .current_dir(repo_root) | ||
| .args(["rev-parse", "--git-common-dir"]) | ||
| .output() | ||
| .ok()?; | ||
| if !output.status.success() { | ||
| return None; | ||
| } | ||
|
|
||
| let raw = String::from_utf8_lossy(&output.stdout); | ||
| let trimmed = raw.trim(); | ||
| if trimmed.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| let common_dir = PathBuf::from(trimmed); | ||
| Some(if common_dir.is_absolute() { | ||
| common_dir | ||
| } else { | ||
| repo_root.join(common_dir) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how KindraProvider methods are invoked in the picker/session flow,
# to confirm whether they run on a thread that can block the UI.
rg -n 'temp_worktrees_configured|create_temp_worktree' crates/wisp-bin/src/main.rs -B3 -A3Repository: Pajn/wisp
Length of output: 1332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files, then inspect the call sites and implementations.
ast-grep outline crates/wisp-bin/src/main.rs --view expanded | sed -n '1,220p'
printf '\n==== lib.rs outline ====\n'
ast-grep outline crates/wisp-kindra/src/lib.rs --view expanded | sed -n '1,220p'
printf '\n==== main.rs around temp_worktrees_configured ====\n'
sed -n '1620,1685p' crates/wisp-bin/src/main.rs
printf '\n==== main.rs around create_temp_worktree ====\n'
sed -n '1140,1185p' crates/wisp-bin/src/main.rs
printf '\n==== wisp-kindra lib.rs around subprocess calls ====\n'
sed -n '1,180p' crates/wisp-kindra/src/lib.rsRepository: Pajn/wisp
Length of output: 21998
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for existing timeout/cancellation patterns and other synchronous subprocess usage.
rg -n '\btimeout\b|poll\(|try_wait\(|wait_timeout|Command::output\(|Command::spawn\(' crates -g '!**/target/**'
printf '\n==== wisp-kindra tests and helpers ====\n'
sed -n '230,320p' crates/wisp-kindra/src/lib.rsRepository: Pajn/wisp
Length of output: 2516
Add a timeout around Kindra subprocesses git_common_dir and create_temp_worktree call Command::output() with no deadline, so a stalled git/kin process can block the picker/session-activation flow indefinitely. Consider a bounded wait or moving the call to a cancellable worker thread.
🤖 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 `@crates/wisp-kindra/src/lib.rs` around lines 66 - 91, Add a timeout to the
Kindra subprocess calls so a stalled git/kin process cannot block the flow
forever. Update the process launch in git_common_dir and create_temp_worktree to
use a bounded wait or cancellable worker pattern instead of unbounded
Command::output(), and make sure the timeout path returns a clean None/error
through the same callers that currently handle subprocess failure.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@crates/wisp-bin/src/git.rs`:
- Around line 220-232: The local_branch_exists helper is using
Command::status(), which inherits git stdout/stderr and can corrupt the picker
UI in raw mode. Update local_branch_exists in git.rs to capture child output the
same way remote_default_branch does, so any git warnings or hints are swallowed
instead of written directly to the terminal, while keeping the success/failure
check based on the exit status.
In `@crates/wisp-kindra/tests/integration.rs`:
- Around line 9-20: The integration tests still use a manual unique temp
directory helper, which can leave artifacts behind on panic. Replace the
`unique_root`/`UNIQUE_ROOT_COUNTER` pattern in `integration.rs` with
`tempfile::TempDir` so cleanup happens automatically via `Drop`, and update the
tests that currently take `&root` to use `root.path()` instead. Also remove any
explicit `remove_dir_all` cleanup at the end of the affected tests since
`TempDir` should own the lifecycle.
In `@scripts/release.sh`:
- Around line 124-137: The internal workspace dependency bump in the release
script only checks for a non-zero regex match, so it can partially update
Cargo.toml without failing. Update the logic around the Python re.subn block in
scripts/release.sh to assert the full expected number of wisp path-dependency
matches, similar to bump_section_version, so any layout mismatch or missed
dependency causes the release to fail instead of silently leaving an old version
behind.
- Around line 151-154: The release script is hardcoding the wisp-embers manifest
path instead of reusing the existing EMBERS_CARGO_TOML variable. Update the
cargo metadata step in scripts/release.sh to reference EMBERS_CARGO_TOML
consistently so the path stays centralized and does not drift if the crate
location changes.
- Around line 79-115: The section-scoped matcher in bump_section_version can run
past the intended TOML section and update a later version field instead of
stopping at the next header. Tighten the regex in bump_section_version so it
only searches within the target section body and explicitly stops before the
next [section] line. Use the existing bump_section_version helper and its call
sites in bump_workspace_versions as the place to apply the fix, and keep the
expected match count check so layout drift still fails the release.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e72d258d-f14d-45da-a969-1727eeea2c83
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlREADME.mdcrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/git.rscrates/wisp-bin/src/main.rscrates/wisp-core/src/view.rscrates/wisp-kindra/Cargo.tomlcrates/wisp-kindra/src/lib.rscrates/wisp-kindra/tests/integration.rscrates/wisp-ui/src/lib.rsscripts/release.sh
Comment on lines
+9
to
+20
| static UNIQUE_ROOT_COUNTER: AtomicU64 = AtomicU64::new(0); | ||
|
|
||
| fn unique_root() -> PathBuf { | ||
| let dir = std::env::temp_dir().join(format!( | ||
| "wisp-kindra-it-{}-{}", | ||
| std::process::id(), | ||
| UNIQUE_ROOT_COUNTER.fetch_add(1, Ordering::SeqCst) | ||
| )); | ||
| let _ = std::fs::remove_dir_all(&dir); | ||
| std::fs::create_dir_all(&dir).expect("create temp root"); | ||
| dir | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider tempfile for automatic cleanup on test failure.
The manual unique_root/remove_dir_all pattern leaves directories behind if an assertion panics mid-test. The tempfile crate's TempDir cleans up via Drop even on panic, removing the need for manual counters and explicit removal calls.
♻️ Suggested refactor
-static UNIQUE_ROOT_COUNTER: AtomicU64 = AtomicU64::new(0);
-
-fn unique_root() -> PathBuf {
- let dir = std::env::temp_dir().join(format!(
- "wisp-kindra-it-{}-{}",
- std::process::id(),
- UNIQUE_ROOT_COUNTER.fetch_add(1, Ordering::SeqCst)
- ));
- let _ = std::fs::remove_dir_all(&dir);
- std::fs::create_dir_all(&dir).expect("create temp root");
- dir
-}
+fn unique_root() -> tempfile::TempDir {
+ tempfile::tempdir().expect("create temp root")
+}Callers would use root.path() instead of &root, and drop the manual remove_dir_all(&root) cleanup calls at the end of each test.
🤖 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 `@crates/wisp-kindra/tests/integration.rs` around lines 9 - 20, The integration
tests still use a manual unique temp directory helper, which can leave artifacts
behind on panic. Replace the `unique_root`/`UNIQUE_ROOT_COUNTER` pattern in
`integration.rs` with `tempfile::TempDir` so cleanup happens automatically via
`Drop`, and update the tests that currently take `&root` to use `root.path()`
instead. Also remove any explicit `remove_dir_all` cleanup at the end of the
affected tests since `TempDir` should own the lifecycle.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/wisp-kindra/src/lib.rs (1)
76-98: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftStill no timeout on
kin/gitsubprocess calls.
git_common_dirandrun_kinboth callCommand::output()with no deadline. A stalledgit/kinprocess (e.g. hung network prompt, lock contention) will block the picker/session-activation flow indefinitely. This was already flagged on a previous commit and remains unaddressed.Also applies to: 172-211
🤖 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 `@crates/wisp-kindra/src/lib.rs` around lines 76 - 98, `git_common_dir` still shells out with an unbounded `Command::output()`, so stalled `git`/`kin` subprocesses can block the flow indefinitely. Update both `git_common_dir` and `run_kin` to use a timeout-aware process execution path (for example via a helper that kills the child after a deadline) and handle timeout errors cleanly by returning `None`/propagating an error. Use the existing `git_common_dir` and `run_kin` symbols to locate the two subprocess call sites and apply the same deadline behavior consistently.
🤖 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 `@crates/wisp-bin/src/git.rs`:
- Around line 199-234: The git subprocesses used by remote_default_branch and
local_branch_exists do not have any timeout, so a stalled git command can block
trunk_branch and the picker UI indefinitely. Update these helpers to run git
through the project’s existing timeout-aware subprocess pattern (the same
approach used elsewhere for wisp-kindra), and have trunk_branch propagate or
handle timeout failures cleanly when calling remote_default_branch and
local_branch_exists.
In `@crates/wisp-bin/src/main.rs`:
- Around line 2102-2134: The ConfirmDeleteWorktree branch in run_surface
currently uses `kindra.remove_temp_worktree(&repo_root, &branch)?`, which exits
the whole picker on an expected deletion failure. Change this arm to handle the
error inline: call `remove_temp_worktree` without propagating, surface the
failure through the existing in-app error reporting path used by the
picker/sidebar, and then return the user to the filter view while preserving the
current query/selection state. Use the `InputMode::ConfirmDeleteWorktree` match
arm, `run_surface`, and `kindra.remove_temp_worktree` to locate the change.
---
Duplicate comments:
In `@crates/wisp-kindra/src/lib.rs`:
- Around line 76-98: `git_common_dir` still shells out with an unbounded
`Command::output()`, so stalled `git`/`kin` subprocesses can block the flow
indefinitely. Update both `git_common_dir` and `run_kin` to use a timeout-aware
process execution path (for example via a helper that kills the child after a
deadline) and handle timeout errors cleanly by returning `None`/propagating an
error. Use the existing `git_common_dir` and `run_kin` symbols to locate the two
subprocess call sites and apply the same deadline behavior consistently.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb75f74e-dd5f-46bb-9590-ff66b62867c9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlREADME.mdcrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/git.rscrates/wisp-bin/src/main.rscrates/wisp-core/src/view.rscrates/wisp-kindra/Cargo.tomlcrates/wisp-kindra/src/lib.rscrates/wisp-kindra/tests/integration.rscrates/wisp-ui/src/lib.rsscripts/release.sh
Comment on lines
+199
to
+234
| fn remote_default_branch(repo_root: &Path) -> Option<String> { | ||
| let output = Command::new("git") | ||
| .current_dir(repo_root) | ||
| .args(["symbolic-ref", "--short", "refs/remotes/origin/HEAD"]) | ||
| .output() | ||
| .ok()?; | ||
| if !output.status.success() { | ||
| return None; | ||
| } | ||
|
|
||
| let raw = String::from_utf8_lossy(&output.stdout); | ||
| let trimmed = raw.trim(); | ||
| // `origin/main` -> `main` | ||
| let branch = trimmed.strip_prefix("origin/").unwrap_or(trimmed); | ||
| if branch.is_empty() { | ||
| None | ||
| } else { | ||
| Some(branch.to_string()) | ||
| } | ||
| } | ||
|
|
||
| fn local_branch_exists(repo_root: &Path, branch: &str) -> bool { | ||
| // Capture output rather than inheriting the terminal: `status()` would let any | ||
| // git warning/hint leak onto the raw-mode picker and corrupt the display. | ||
| Command::new("git") | ||
| .current_dir(repo_root) | ||
| .args([ | ||
| "show-ref", | ||
| "--verify", | ||
| "--quiet", | ||
| &format!("refs/heads/{branch}"), | ||
| ]) | ||
| .output() | ||
| .map(|output| output.status.success()) | ||
| .unwrap_or(false) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
No timeout on git subprocess calls used to resolve the trunk branch.
remote_default_branch and local_branch_exists (called up to 3x from trunk_branch) shell out via Command::output() with no deadline. Per the call sites in main.rs (git::trunk_branch(repo_root) during temp-worktree activation and when building KindraTempContext), a stalled git process here would block the picker UI indefinitely — the same risk already flagged for wisp-kindra's subprocess calls, but at a separate call site.
🤖 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 `@crates/wisp-bin/src/git.rs` around lines 199 - 234, The git subprocesses used
by remote_default_branch and local_branch_exists do not have any timeout, so a
stalled git command can block trunk_branch and the picker UI indefinitely.
Update these helpers to run git through the project’s existing timeout-aware
subprocess pattern (the same approach used elsewhere for wisp-kindra), and have
trunk_branch propagate or handle timeout failures cleanly when calling
remote_default_branch and local_branch_exists.
Comment on lines
+2102
to
+2134
| InputMode::ConfirmDeleteWorktree { | ||
| branch, | ||
| repo_root, | ||
| filter_query, | ||
| } => { | ||
| let branch = branch.clone(); | ||
| let repo_root = repo_root.clone(); | ||
| let filter_query = filter_query.clone(); | ||
|
|
||
| // Never forces: a dirty worktree makes kin fail and the | ||
| // error propagates rather than discarding uncommitted work. | ||
| kindra.remove_temp_worktree(&repo_root, &branch)?; | ||
|
|
||
| let reloaded_state = load_domain_state(backend)?; | ||
| apply_reloaded_state( | ||
| reloaded_state, | ||
| picker_mode, | ||
| session_sort, | ||
| &mut active_client_id, | ||
| &mut session_items, | ||
| &mut pending_branch_names, | ||
| &mut branch_status_updates, | ||
| &mut details_preview_provider, | ||
| &mut deferred_branch_status, | ||
| ); | ||
| query = filter_query; | ||
| input_mode = InputMode::Filter; | ||
| preview_session_id = None; | ||
| preview_refreshed_at = None; | ||
| if preview_enabled { | ||
| preview = Some(Vec::new()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Deleting a dirty temp worktree exits the whole picker instead of showing an error.
kindra.remove_temp_worktree(&repo_root, &branch)? propagates via ?, and this arm is inside run_surface's main loop, whose Result is returned from the function on error. Since deletion deliberately never forces (per the adjacent comment, a dirty worktree makes kin fail on purpose), this is an expected, likely-common failure path — yet it terminates the entire interactive picker/sidebar session rather than surfacing the error and returning to the filter view. The user loses their query/selection state for something as routine as trying to delete a worktree with uncommitted changes. This also doesn't match the README's description ("Kindra reports the error"), which implies in-app feedback rather than an app exit.
💡 Illustrative fix: handle the error inline instead of propagating
InputMode::ConfirmDeleteWorktree {
branch,
repo_root,
filter_query,
} => {
let branch = branch.clone();
let repo_root = repo_root.clone();
let filter_query = filter_query.clone();
// Never forces: a dirty worktree makes kin fail and the
- // error propagates rather than discarding uncommitted work.
- kindra.remove_temp_worktree(&repo_root, &branch)?;
-
- let reloaded_state = load_domain_state(backend)?;
- apply_reloaded_state(
- reloaded_state,
- picker_mode,
- session_sort,
- &mut active_client_id,
- &mut session_items,
- &mut pending_branch_names,
- &mut branch_status_updates,
- &mut details_preview_provider,
- &mut deferred_branch_status,
- );
- query = filter_query;
- input_mode = InputMode::Filter;
- preview_session_id = None;
- preview_refreshed_at = None;
- if preview_enabled {
- preview = Some(Vec::new());
- }
+ // rather than discarding uncommitted work — surface the
+ // failure instead of exiting the picker.
+ match kindra.remove_temp_worktree(&repo_root, &branch) {
+ Ok(()) => {
+ let reloaded_state = load_domain_state(backend)?;
+ apply_reloaded_state(
+ reloaded_state,
+ picker_mode,
+ session_sort,
+ &mut active_client_id,
+ &mut session_items,
+ &mut pending_branch_names,
+ &mut branch_status_updates,
+ &mut details_preview_provider,
+ &mut deferred_branch_status,
+ );
+ }
+ Err(_error) => {
+ // TODO: surface `_error` to the user (status line/preview banner).
+ }
+ }
+ query = filter_query;
+ input_mode = InputMode::Filter;
+ preview_session_id = None;
+ preview_refreshed_at = None;
+ if preview_enabled {
+ preview = Some(Vec::new());
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| InputMode::ConfirmDeleteWorktree { | |
| branch, | |
| repo_root, | |
| filter_query, | |
| } => { | |
| let branch = branch.clone(); | |
| let repo_root = repo_root.clone(); | |
| let filter_query = filter_query.clone(); | |
| // Never forces: a dirty worktree makes kin fail and the | |
| // error propagates rather than discarding uncommitted work. | |
| kindra.remove_temp_worktree(&repo_root, &branch)?; | |
| let reloaded_state = load_domain_state(backend)?; | |
| apply_reloaded_state( | |
| reloaded_state, | |
| picker_mode, | |
| session_sort, | |
| &mut active_client_id, | |
| &mut session_items, | |
| &mut pending_branch_names, | |
| &mut branch_status_updates, | |
| &mut details_preview_provider, | |
| &mut deferred_branch_status, | |
| ); | |
| query = filter_query; | |
| input_mode = InputMode::Filter; | |
| preview_session_id = None; | |
| preview_refreshed_at = None; | |
| if preview_enabled { | |
| preview = Some(Vec::new()); | |
| } | |
| } | |
| InputMode::ConfirmDeleteWorktree { | |
| branch, | |
| repo_root, | |
| filter_query, | |
| } => { | |
| let branch = branch.clone(); | |
| let repo_root = repo_root.clone(); | |
| let filter_query = filter_query.clone(); | |
| // Never forces: a dirty worktree makes kin fail and the | |
| // rather than discarding uncommitted work — surface the | |
| // failure instead of exiting the picker. | |
| match kindra.remove_temp_worktree(&repo_root, &branch) { | |
| Ok(()) => { | |
| let reloaded_state = load_domain_state(backend)?; | |
| apply_reloaded_state( | |
| reloaded_state, | |
| picker_mode, | |
| session_sort, | |
| &mut active_client_id, | |
| &mut session_items, | |
| &mut pending_branch_names, | |
| &mut branch_status_updates, | |
| &mut details_preview_provider, | |
| &mut deferred_branch_status, | |
| ); | |
| } | |
| Err(_error) => { | |
| // TODO: surface `_error` to the user (status line/preview banner). | |
| } | |
| } | |
| query = filter_query; | |
| input_mode = InputMode::Filter; | |
| preview_session_id = None; | |
| preview_refreshed_at = None; | |
| if preview_enabled { | |
| preview = Some(Vec::new()); | |
| } | |
| } |
🤖 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 `@crates/wisp-bin/src/main.rs` around lines 2102 - 2134, The
ConfirmDeleteWorktree branch in run_surface currently uses
`kindra.remove_temp_worktree(&repo_root, &branch)?`, which exits the whole
picker on an expected deletion failure. Change this arm to handle the error
inline: call `remove_temp_worktree` without propagating, surface the failure
through the existing in-app error reporting path used by the picker/sidebar, and
then return the user to the filter view while preserving the current
query/selection state. Use the `InputMode::ConfirmDeleteWorktree` match arm,
`run_surface`, and `kindra.remove_temp_worktree` to locate the change.
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
+marker.main/masterfallbacks.