Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (23)
📝 WalkthroughWalkthroughWisp adds optional Embers backend support across config, core data, client libraries, CLI routing, docs, workflows, and tests. Backend selection now flows through config and runtime abstractions, with a new ChangesEmbers Backend Support
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-app/src/lib.rs`:
- Around line 479-481: Rename the misleading SessionRecord::tmux_id field to a
generic name (e.g., native_id or backend_id) and update all places that
construct or access it: change the struct definition in wisp-core and update the
constructor call in crates/wisp-app/src/lib.rs (the SessionRecord literal where
tmux_id: Some(session.native_id.clone()) is set), adapters that populate/read
this field from backends (tmux and Embers), any serialization/deserialization or
DB mapping code, and tests/examples; ensure matching Option types (Some/None)
and update imports/usages to the new symbol name so compilation and runtime
behavior remain the same.
In `@crates/wisp-bin/src/main.rs`:
- Around line 1339-1405: The UI loop contains fallible backend calls
(sidebar_requires_handoff, persist_sidebar_ui_state,
reconcile_sidebar_for_current_context,
client.join_buffer_to_current_session_root, runtime.rename_session,
reload_sidebar_runtime_state) that can early-return and skip terminal teardown;
refactor so terminal raw-mode/alternate-screen cleanup is always executed on any
early exit by moving these backend operations into a fallible helper that
returns Result and ensuring the outer loop uses an RAII guard or explicit
finally-style cleanup (e.g., a TerminalTeardown guard constructed before
entering the loop or calling a restore function in a match on the helper's
Result) so that even when those functions return Err the terminal teardown runs.
Ensure references to sidebar_runtime, backend, and runtime/session mutation
remain valid across this change.
- Around line 1141-1154: The buffer is created with client.create_buffer and
then a follow-up call like client.create_floating_for_buffer_in_current_session
may fail and leave an orphaned buffer; wrap the follow-up call so that on error
you clean up the newly created buffer (use the client method that removes
buffers such as client.delete_buffer or the repo's equivalent) before returning
the error, and apply the same rollback pattern to the other two occurrences (the
sidebar-popup float path and the sidebar-pane join path referenced in the
comment); make sure to reference buffer_id from the create_buffer call and call
the remove/delete API with that id in the error arm.
- Line 2520: The worker loop currently uses queue.lock().ok() which swallows a
poisoned mutex and causes the worker to exit silently; change the lock handling
in the loop that reads from queue (the while let Some(work_item) =
queue.lock().ok().and_then(|mut queue| queue.pop_front()) { ... }) to explicitly
handle PoisonError: if lock() returns Ok(guard) use it; if Err(poison) recover
with poison.into_inner() (or otherwise obtain the inner guard), log the poison
error with context (e.g., which git worker and a descriptive message) and
continue processing so pending git status work is not dropped; ensure you still
call queue.pop_front() on the recovered guard and keep the rest of the loop
logic unchanged.
In `@rust-toolchain.toml`:
- Line 2: The pinned Rust toolchain in rust-toolchain.toml (channel = "1.95.0")
is inconsistent with the workspace MSRV declared as rust-version = "1.90" in the
root Cargo.toml; decide which MSRV to use and make them match: either update the
root Cargo.toml rust-version to "1.95" (or "1.95.0") if the code/deps require
newer Rust, or change rust-toolchain.toml channel to "1.90" (or "1.90.0") if
1.90 is intended; ensure both the rust-toolchain.toml channel and the root
Cargo.toml rust-version fields are identical after the change.
🪄 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: 0d46f70c-eb23-404a-9a83-f7431bc3c155
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlREADME.mdcrates/wisp-app/Cargo.tomlcrates/wisp-app/src/lib.rscrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/main.rscrates/wisp-bin/tests/smoke.rscrates/wisp-config/src/lib.rscrates/wisp-core/src/view.rscrates/wisp-embers/Cargo.tomlcrates/wisp-embers/src/lib.rscrates/wisp-embers/tests/integration.rscrates/wisp-preview/src/lib.rsdocs/config.schema.tomldocs/configuration.mdrust-toolchain.toml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/wisp-embers/src/lib.rs (1)
405-426:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t detach the buffer before the cross-session join is known to succeed.
If
JoinBufferAtNodefails afterdetach_buffer_record, the buffer is left unattached and disappears from the original session. This needs either an atomic server-side move or explicit rollback using the original location.🤖 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-embers/src/lib.rs` around lines 405 - 426, The code detaches the buffer via detach_buffer_record(runtime, state, buffer_id) before issuing the cross-session request JoinBufferAtNode, which can leave the buffer unattached if the request fails; change the flow in the join logic so you first record the original location (using buffer_location), then perform the remote request (runtime.block_on(state.client.request_message(...)) calling NodeRequest::JoinBufferAtNode) and only call detach_buffer_record when the request and apply_session_layout_response(...) succeed; alternatively, if you must detach first implement an explicit rollback that reattaches the original location on any failure by using the saved original location to restore the buffer record.crates/wisp-bin/src/main.rs (1)
1336-1337:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop hard-coding
defaultfor Embers projections.
build_embers_statestores client focus undersnapshot.context.client_id, but this path always rebuilds session items withSome(DEFAULT_CLIENT_ID). On a real Embers snapshot that meansstate.current_session_id(...)andstate.previous_session_id(...)never resolve, so current/previous markers break. The same hard-coded key is reused in the later reload paths too.🤖 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 1336 - 1337, The call to rebuild_session_items_for_picker_mode is hard-coded with Some(DEFAULT_CLIENT_ID), which prevents session lookup from using the actual Embers snapshot client id (snapshot.context.client_id) and breaks state.current_session_id/previous_session_id resolution; change the call site(s) to pass the real client id from the state/snapshot (e.g., use the client id stored at snapshot.context.client_id via the current state object) instead of DEFAULT_CLIENT_ID, and update any other reload paths that reuse DEFAULT_CLIENT_ID to similarly forward the snapshot.context.client_id so current/previous markers resolve correctly..github/workflows/ci.yml (1)
47-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the Embers tests in CI.
This workflow only executes default-feature tests, so the new
#[cfg(feature = "embers")]coverage added in this PR never runs.cargo clippy --all-featuresonly proves the code compiles; it does not execute those tests.Suggested change
- - name: Run tests - run: cargo test --workspace --all-targets + - name: Run tests + run: cargo test --workspace --all-targets --all-features🤖 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 @.github/workflows/ci.yml around lines 47 - 48, The CI step named "Run tests" currently runs tests without feature flags; update its command so embers-enabled tests execute by running cargo test with all features and targets (e.g. replace `cargo test --workspace --all-targets` with `cargo test --workspace --all-targets --all-features`) so that `#[cfg(feature = "embers")]` tests are exercised in CI.
♻️ Duplicate comments (1)
crates/wisp-embers/src/lib.rs (1)
667-695:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClean up the temporary root buffer when
AddRootTabfails.This creates a buffer and then performs a second request. If the second step fails, the buffer is orphaned in the mux and in the local cache.
Suggested rollback pattern
let buffer_id = create_buffer_record( runtime, state, &command, &title, Some(directory), &BTreeMap::new(), )?; let response = runtime.block_on(state.client.request_message(ClientMessage::Session( SessionRequest::AddRootTab { request_id: state.client.next_request_id(), session_id, title, buffer_id: Some(buffer_id), child_node_id: None, }, )))?; match response { ServerResponse::SessionSnapshot(response) => { state .client .state_mut() .apply_session_snapshot(response.snapshot); Ok(()) } - _ => Err(EmbersError::UnexpectedResponse("add root tab")), + _ => { + let _ = runtime.block_on(state.client.request_message(ClientMessage::Buffer( + BufferRequest::Kill { + request_id: state.client.next_request_id(), + buffer_id, + force: true, + }, + ))); + Err(EmbersError::UnexpectedResponse("add root tab")) + } }
🤖 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.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 47-48: The CI step named "Run tests" currently runs tests without
feature flags; update its command so embers-enabled tests execute by running
cargo test with all features and targets (e.g. replace `cargo test --workspace
--all-targets` with `cargo test --workspace --all-targets --all-features`) so
that `#[cfg(feature = "embers")]` tests are exercised in CI.
In `@crates/wisp-bin/src/main.rs`:
- Around line 1336-1337: The call to rebuild_session_items_for_picker_mode is
hard-coded with Some(DEFAULT_CLIENT_ID), which prevents session lookup from
using the actual Embers snapshot client id (snapshot.context.client_id) and
breaks state.current_session_id/previous_session_id resolution; change the call
site(s) to pass the real client id from the state/snapshot (e.g., use the client
id stored at snapshot.context.client_id via the current state object) instead of
DEFAULT_CLIENT_ID, and update any other reload paths that reuse
DEFAULT_CLIENT_ID to similarly forward the snapshot.context.client_id so
current/previous markers resolve correctly.
In `@crates/wisp-embers/src/lib.rs`:
- Around line 405-426: The code detaches the buffer via
detach_buffer_record(runtime, state, buffer_id) before issuing the cross-session
request JoinBufferAtNode, which can leave the buffer unattached if the request
fails; change the flow in the join logic so you first record the original
location (using buffer_location), then perform the remote request
(runtime.block_on(state.client.request_message(...)) calling
NodeRequest::JoinBufferAtNode) and only call detach_buffer_record when the
request and apply_session_layout_response(...) succeed; alternatively, if you
must detach first implement an explicit rollback that reattaches the original
location on any failure by using the saved original location to restore the
buffer record.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7fb83e26-3343-47df-aa50-f3bc219750af
📒 Files selected for processing (14)
.github/workflows/ci.yml.github/workflows/prepare-release.yml.github/workflows/release.ymlCargo.tomlcrates/wisp-app/src/lib.rscrates/wisp-bin/src/main.rscrates/wisp-core/benches/projections.rscrates/wisp-core/src/domain.rscrates/wisp-core/src/reduce.rscrates/wisp-core/src/view.rscrates/wisp-embers/Cargo.tomlcrates/wisp-embers/src/lib.rscrates/wisp-preview/src/lib.rsrust-toolchain.toml
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/wisp-app/src/lib.rs (1)
349-349: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueInconsistent index conversion between backends.
build_tmux_stateuses an uncheckedas i32cast here, whilebuild_embers_state(line 458) usesi32::try_from(...).expect(...)for the same conversion. For consistency and defensive coding, consider using the checked conversion in both places.♻️ Suggested fix for consistency
- index: window.index as i32, + index: i32::try_from(window.index) + .expect("window index should fit in i32"),🤖 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-app/src/lib.rs` at line 349, The index conversion in build_tmux_state uses an unchecked as i32 cast while build_embers_state uses a checked i32::try_from conversion with expect. Replace the unchecked cast of window.index to i32 in build_tmux_state with the same defensive checked conversion pattern using i32::try_from that is already used in build_embers_state for consistency and to prevent potential overflow issues.
🤖 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 @.github/workflows/ci.yml:
- Around line 55-56: The workflow currently uses floating version tags for three
GitHub Actions (actions/checkout using `@v4`, dtolnay/rust-toolchain using
`@stable`, and swatinem/rust-cache using `@v2`), which poses a supply chain security
risk since the action definitions can change without explicit updates. Replace
each floating version tag with its corresponding full commit SHA: update
actions/checkout@v4 to its commit SHA, dtolnay/rust-toolchain@stable to its
commit SHA, and swatinem/rust-cache@v2 to its commit SHA. This pins the exact
version of each action being used and prevents unexpected changes.
In @.github/workflows/prepare-release.yml:
- Line 12: The `actions: write` permission is currently granted at the workflow
level (line 12), giving every job and step unnecessary workflow control
privileges. Move the `actions: write` permission from the top-level permissions
section to be scoped only to the specific job that handles workflow dispatch
operations. Create a dedicated job with its own permissions block that includes
only `actions: write`, and remove this permission from the workflow-wide
permissions to reduce the security blast radius.
- Around line 65-68: The "Dispatch release workflow" step injects inputs.version
directly into the gh workflow run shell command without validation, which
creates a shell injection vulnerability. Add validation to ensure inputs.version
matches an expected semantic version pattern (like v1.2.3 with only alphanumeric
characters, dots, hyphens, and the v prefix) before using it in the command, or
assign it to an environment variable in the env section and reference that
variable in the run command instead of inline interpolation to prevent shell
metacharacters from being interpreted as commands.
In @.github/workflows/release.yml:
- Around line 14-15: The workflow-level permissions configuration grants
unnecessary write access to all jobs. Instead, change the workflow-level
permissions to set contents as read-only by default, and then add a job-specific
permissions override in the create-release job (or whichever job actually needs
to create releases or upload artifacts) to grant it contents: write permission.
This follows the principle of least privilege and ensures only the jobs that
actually require write access have it.
In `@crates/wisp-embers/src/lib.rs`:
- Around line 732-745: In the session_has_root_window function, the logic at the
end where tabs.is_none_or(...) is called incorrectly treats a missing tabs
payload as an existing root window. Change the logic to only return true when
tabs actually exists AND contains windows by replacing is_none_or with
is_some_and, so the condition properly validates that both the tabs payload
exists and it contains non-empty tabs before confirming a root window exists.
In `@docs/configuration.md`:
- Around line 28-32: The documentation for embers.socket_path claims it accepts
an "absolute socket path" but doesn't clarify when this validation occurs.
Investigate where absolute path validation actually happens (either during
config parsing with PathBuf::from or later at runtime in MuxClient::connect),
then update the documentation at lines 29 and 64 to explicitly state whether the
absolute path requirement is enforced at parse-time or at runtime, or clarify if
no validation occurs and the requirement is merely a convention.
---
Outside diff comments:
In `@crates/wisp-app/src/lib.rs`:
- Line 349: The index conversion in build_tmux_state uses an unchecked as i32
cast while build_embers_state uses a checked i32::try_from conversion with
expect. Replace the unchecked cast of window.index to i32 in build_tmux_state
with the same defensive checked conversion pattern using i32::try_from that is
already used in build_embers_state for consistency and to prevent potential
overflow issues.
🪄 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: 050bfba3-e8bd-4c37-8b32-a694e6964b81
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcrates/wisp-embers/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/ci.yml.github/workflows/prepare-release.yml.github/workflows/release.yml.gitignoreCargo.tomlREADME.mdcrates/wisp-app/Cargo.tomlcrates/wisp-app/src/lib.rscrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/main.rscrates/wisp-bin/tests/smoke.rscrates/wisp-config/src/lib.rscrates/wisp-core/benches/projections.rscrates/wisp-core/src/domain.rscrates/wisp-core/src/reduce.rscrates/wisp-core/src/view.rscrates/wisp-embers/Cargo.tomlcrates/wisp-embers/src/lib.rscrates/wisp-embers/tests/integration.rscrates/wisp-preview/src/lib.rsdocs/config.schema.tomldocs/configuration.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/wisp-app/src/lib.rs (1)
349-349: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueInconsistent index conversion between backends.
build_tmux_stateuses an uncheckedas i32cast here, whilebuild_embers_state(line 458) usesi32::try_from(...).expect(...)for the same conversion. For consistency and defensive coding, consider using the checked conversion in both places.♻️ Suggested fix for consistency
- index: window.index as i32, + index: i32::try_from(window.index) + .expect("window index should fit in i32"),🤖 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-app/src/lib.rs` at line 349, The index conversion in build_tmux_state uses an unchecked as i32 cast while build_embers_state uses a checked i32::try_from conversion with expect. Replace the unchecked cast of window.index to i32 in build_tmux_state with the same defensive checked conversion pattern using i32::try_from that is already used in build_embers_state for consistency and to prevent potential overflow issues.
🤖 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 @.github/workflows/ci.yml:
- Around line 55-56: The workflow currently uses floating version tags for three
GitHub Actions (actions/checkout using `@v4`, dtolnay/rust-toolchain using
`@stable`, and swatinem/rust-cache using `@v2`), which poses a supply chain security
risk since the action definitions can change without explicit updates. Replace
each floating version tag with its corresponding full commit SHA: update
actions/checkout@v4 to its commit SHA, dtolnay/rust-toolchain@stable to its
commit SHA, and swatinem/rust-cache@v2 to its commit SHA. This pins the exact
version of each action being used and prevents unexpected changes.
In @.github/workflows/prepare-release.yml:
- Line 12: The `actions: write` permission is currently granted at the workflow
level (line 12), giving every job and step unnecessary workflow control
privileges. Move the `actions: write` permission from the top-level permissions
section to be scoped only to the specific job that handles workflow dispatch
operations. Create a dedicated job with its own permissions block that includes
only `actions: write`, and remove this permission from the workflow-wide
permissions to reduce the security blast radius.
- Around line 65-68: The "Dispatch release workflow" step injects inputs.version
directly into the gh workflow run shell command without validation, which
creates a shell injection vulnerability. Add validation to ensure inputs.version
matches an expected semantic version pattern (like v1.2.3 with only alphanumeric
characters, dots, hyphens, and the v prefix) before using it in the command, or
assign it to an environment variable in the env section and reference that
variable in the run command instead of inline interpolation to prevent shell
metacharacters from being interpreted as commands.
In @.github/workflows/release.yml:
- Around line 14-15: The workflow-level permissions configuration grants
unnecessary write access to all jobs. Instead, change the workflow-level
permissions to set contents as read-only by default, and then add a job-specific
permissions override in the create-release job (or whichever job actually needs
to create releases or upload artifacts) to grant it contents: write permission.
This follows the principle of least privilege and ensures only the jobs that
actually require write access have it.
In `@crates/wisp-embers/src/lib.rs`:
- Around line 732-745: In the session_has_root_window function, the logic at the
end where tabs.is_none_or(...) is called incorrectly treats a missing tabs
payload as an existing root window. Change the logic to only return true when
tabs actually exists AND contains windows by replacing is_none_or with
is_some_and, so the condition properly validates that both the tabs payload
exists and it contains non-empty tabs before confirming a root window exists.
In `@docs/configuration.md`:
- Around line 28-32: The documentation for embers.socket_path claims it accepts
an "absolute socket path" but doesn't clarify when this validation occurs.
Investigate where absolute path validation actually happens (either during
config parsing with PathBuf::from or later at runtime in MuxClient::connect),
then update the documentation at lines 29 and 64 to explicitly state whether the
absolute path requirement is enforced at parse-time or at runtime, or clarify if
no validation occurs and the requirement is merely a convention.
---
Outside diff comments:
In `@crates/wisp-app/src/lib.rs`:
- Line 349: The index conversion in build_tmux_state uses an unchecked as i32
cast while build_embers_state uses a checked i32::try_from conversion with
expect. Replace the unchecked cast of window.index to i32 in build_tmux_state
with the same defensive checked conversion pattern using i32::try_from that is
already used in build_embers_state for consistency and to prevent potential
overflow issues.
🪄 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: 050bfba3-e8bd-4c37-8b32-a694e6964b81
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcrates/wisp-embers/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/ci.yml.github/workflows/prepare-release.yml.github/workflows/release.yml.gitignoreCargo.tomlREADME.mdcrates/wisp-app/Cargo.tomlcrates/wisp-app/src/lib.rscrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/main.rscrates/wisp-bin/tests/smoke.rscrates/wisp-config/src/lib.rscrates/wisp-core/benches/projections.rscrates/wisp-core/src/domain.rscrates/wisp-core/src/reduce.rscrates/wisp-core/src/view.rscrates/wisp-embers/Cargo.tomlcrates/wisp-embers/src/lib.rscrates/wisp-embers/tests/integration.rscrates/wisp-preview/src/lib.rsdocs/config.schema.tomldocs/configuration.md
🛑 Comments failed to post (6)
.github/workflows/ci.yml (1)
55-56:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify unpinned action refs in this workflow rg -n '^\s*uses:\s*[^@]+@(?![0-9a-fA-F]{40}$).+$' .github/workflows/ci.ymlRepository: Pajn/wisp
Length of output: 316
🏁 Script executed:
# Read the relevant section of the workflow file around the specified lines sed -n '45,75p' .github/workflows/ci.yml | cat -nRepository: Pajn/wisp
Length of output: 1231
🏁 Script executed:
# Read the entire workflow file to check other job definitions cat .github/workflows/ci.yml | head -100Repository: Pajn/wisp
Length of output: 2439
Pin action versions to commit SHAs in the Embers job.
Lines 56, 59, and 70 use floating version tags (
@v4,@stable,@v2). Floating tags introduce supply-chain risk—the action definition can change unexpectedly without updating the workflow. Pin each to its full commit SHA instead.
- Line 56:
actions/checkout@v4→actions/checkout@<commit-sha>- Line 59:
dtolnay/rust-toolchain@stable→dtolnay/rust-toolchain@<commit-sha>- Line 70:
swatinem/rust-cache@v2→swatinem/rust-cache@<commit-sha>🧰 Tools
🪛 zizmor (1.25.2)
[warning] 55-56: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 56-56: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/ci.yml around lines 55 - 56, The workflow currently uses floating version tags for three GitHub Actions (actions/checkout using `@v4`, dtolnay/rust-toolchain using `@stable`, and swatinem/rust-cache using `@v2`), which poses a supply chain security risk since the action definitions can change without explicit updates. Replace each floating version tag with its corresponding full commit SHA: update actions/checkout@v4 to its commit SHA, dtolnay/rust-toolchain@stable to its commit SHA, and swatinem/rust-cache@v2 to its commit SHA. This pins the exact version of each action being used and prevents unexpected changes.Source: Linters/SAST tools
.github/workflows/prepare-release.yml (2)
12-12:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReduce
actions: writeblast radius by scoping it to a dedicated dispatch job.Line 12 grants
actions: writeworkflow-wide, so every step in this job inherits unnecessary workflow-control privileges.🧰 Tools
🪛 zizmor (1.25.2)
[error] 12-12: overly broad permissions (excessive-permissions): actions: write is overly broad at the workflow level
(excessive-permissions)
[warning] 12-12: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 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 @.github/workflows/prepare-release.yml at line 12, The `actions: write` permission is currently granted at the workflow level (line 12), giving every job and step unnecessary workflow control privileges. Move the `actions: write` permission from the top-level permissions section to be scoped only to the specific job that handles workflow dispatch operations. Create a dedicated job with its own permissions block that includes only `actions: write`, and remove this permission from the workflow-wide permissions to reduce the security blast radius.Source: Linters/SAST tools
65-68:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate and sanitize
inputs.versionbefore shell interpolation.Line 68 injects
${{ inputs.version }}into a shell command. A crafted value containing shell metacharacters/quotes can alter command execution.Suggested hardening
+ - name: Validate version input + run: | + [[ "${{ inputs.version }}" =~ ^[0-9]+\.[0-9]+\.[0-9]+([.-][0-9A-Za-z.-]+)?$ ]] || { + echo "Invalid version format"; exit 1; + } + - name: Dispatch release workflow env: GH_TOKEN: ${{ github.token }} + RELEASE_VERSION: ${{ inputs.version }} run: gh workflow run release.yml --ref main -f tag="v${{ inputs.version }}"🧰 Tools
🪛 zizmor (1.25.2)
[error] 68-68: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/prepare-release.yml around lines 65 - 68, The "Dispatch release workflow" step injects inputs.version directly into the gh workflow run shell command without validation, which creates a shell injection vulnerability. Add validation to ensure inputs.version matches an expected semantic version pattern (like v1.2.3 with only alphanumeric characters, dots, hyphens, and the v prefix) before using it in the command, or assign it to an environment variable in the env section and reference that variable in the run command instead of inline interpolation to prevent shell metacharacters from being interpreted as commands.Source: Linters/SAST tools
.github/workflows/release.yml (1)
14-15:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope
contents: writeper job instead of workflow-wide.Line 15 gives write access to all jobs, including ones that only need read. Set workflow default to read and grant write only to
create-release/upload steps that require it.🧰 Tools
🪛 zizmor (1.25.2)
[error] 15-15: overly broad permissions (excessive-permissions): contents: write is overly broad at the workflow level
(excessive-permissions)
[warning] 15-15: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 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 @.github/workflows/release.yml around lines 14 - 15, The workflow-level permissions configuration grants unnecessary write access to all jobs. Instead, change the workflow-level permissions to set contents as read-only by default, and then add a job-specific permissions override in the create-release job (or whichever job actually needs to create releases or upload artifacts) to grant it contents: write permission. This follows the principle of least privilege and ensures only the jobs that actually require write access have it.Source: Linters/SAST tools
crates/wisp-embers/src/lib.rs (1)
732-745:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not treat missing tabs payload as an existing root window.
On Lines 743-744,
tabs.is_none_or(...)returnstruewhen the root is a tabs node butroot.tabsis missing. That makesensure_root_windowskip repair and leaves later snapshot projection to fail on invalid state.Proposed fix
fn session_has_root_window( state: &ClientState, session_id: SessionId, ) -> Result<bool, EmbersError> { @@ - let tabs = root.tabs.as_ref(); - Ok(tabs.is_none_or(|tabs| !tabs.tabs.is_empty())) + match root.kind { + NodeRecordKind::Tabs => { + let tabs = root.tabs.as_ref().ok_or_else(|| { + EmbersError::InvalidState(format!( + "tabs node {} is missing tabs payload", + session.root_node_id + )) + })?; + Ok(!tabs.tabs.is_empty()) + } + NodeRecordKind::Split => { + let split = root.split.as_ref().ok_or_else(|| { + EmbersError::InvalidState(format!( + "split node {} is missing split payload", + session.root_node_id + )) + })?; + Ok(!split.child_ids.is_empty()) + } + NodeRecordKind::BufferView => Ok(true), + } }🤖 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-embers/src/lib.rs` around lines 732 - 745, In the session_has_root_window function, the logic at the end where tabs.is_none_or(...) is called incorrectly treats a missing tabs payload as an existing root window. Change the logic to only return true when tabs actually exists AND contains windows by replacing is_none_or with is_some_and, so the condition properly validates that both the tabs payload exists and it contains non-empty tabs before confirming a root window exists.docs/configuration.md (1)
28-32:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Verify if socket_path is validated as absolute during config parsing or only at runtime. # Search for PathBuf validation or socket connection logic that validates absolute paths. # Check config parsing for socket_path validation rg -n 'socket_path' crates/wisp-config/src/lib.rs -A 5 -B 2 # Check embers client for absolute path validation rg -n 'socket_path|absolute' crates/wisp-embers/src/lib.rs -A 3 -B 1Repository: Pajn/wisp
Length of output: 3894
🏁 Script executed:
#!/bin/bash # Check the actual documentation file cat -n docs/configuration.md | sed -n '20,70p'Repository: Pajn/wisp
Length of output: 3193
🏁 Script executed:
#!/bin/bash # Search for absolute path validation logic rg -n 'is_absolute|absolute' --type md --type rs -iRepository: Pajn/wisp
Length of output: 80
🏁 Script executed:
#!/bin/bash # Check if MuxClient or any socket validation happens rg -n 'MuxClient|is_absolute' crates/ -A 2 -B 1Repository: Pajn/wisp
Length of output: 1747
Clarify when socket_path absolute-path validation is enforced.
Documentation (lines 29 and 64) states
embers.socket_pathaccepts an "absolute socket path", but config parsing only performsPathBuf::from(value)without validation. Validation either happens later (in MuxClient::connect at runtime) or doesn't occur at all. Update the docs to explicitly state whether this is a parse-time requirement or a runtime expectation.🤖 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 `@docs/configuration.md` around lines 28 - 32, The documentation for embers.socket_path claims it accepts an "absolute socket path" but doesn't clarify when this validation occurs. Investigate where absolute path validation actually happens (either during config parsing with PathBuf::from or later at runtime in MuxClient::connect), then update the documentation at lines 29 and 64 to explicitly state whether the absolute path requirement is enforced at parse-time or at runtime, or clarify if no validation occurs and the requirement is merely a convention.
972d665 to
04e5717
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 @.github/workflows/ci.yml:
- Around line 37-41: The flatc_version variable is extracted using sed without
validation, which means if sed fails to match the pattern or extract a version,
the variable remains empty and causes a malformed download URL in the curl
command. Add an explicit check immediately after the sed command that verifies
flatc_version is not empty and exits with a clear error message if the
extraction fails. Apply this same validation fix to both occurrences mentioned:
the initial flatc_version extraction around line 37 and the similar extraction
pattern around line 79-83.
- Around line 63-67: Replace the mutable action version tags with pinned commit
SHAs in the workflow. For the Checkout action, change from `@v4` to the full
commit SHA (eef61447b9ff4aafe5dcd4e0bbf5d482be7e7871) and add a with section
containing persist-credentials: false, followed by a comment showing the
version. For the Install Rust toolchain action, change from `@stable` to its
commit SHA (1482605baf5caebd8b2feb1143f460cce50e0a94) and add a comment showing
the version. Repeat this pattern for any other mutable tags referenced on lines
86 and elsewhere in the workflow file.
In @.github/workflows/prepare-release.yml:
- Around line 53-56: The flatc_version extraction via sed on line 53 can
silently fail and result in an empty variable, which would then cause the curl
command to download from a malformed URL. Add a validation check immediately
after the sed command extraction to verify that flatc_version is not empty, and
have the script exit with an error message if the version extraction fails. This
prevents the subsequent curl and unzip commands from executing with an invalid
or missing version number.
In @.github/workflows/release.yml:
- Around line 42-45: The publish-crates job in release.yml installs
flatbuffers-compiler via apt on line 44, but this version may not match the
flatbuffers crate version in Cargo.lock. Replace the apt-based installation of
flatbuffers-compiler with the Cargo.lock-matched approach already implemented in
prepare-release.yml (lines 53-60), which extracts the flatc version from
Cargo.lock and downloads the corresponding release binary. Apply the same
pattern to the release.yml workflow's system dependencies step to ensure the
flatc binary is compatible with the generated flatbuffers code.
In `@crates/wisp-bin/src/main.rs`:
- Around line 883-890: The existing_session_names function is silently
swallowing errors from list_sessions() by using unwrap_or_default(), which
converts any error into an empty vector instead of propagating it. Replace the
unwrap_or_default() call with the ? operator to properly propagate errors from
list_sessions() to the caller, matching the pattern used in the Embers
implementation, which ensures that any tmux communication errors are surfaced
rather than hidden.
In `@crates/wisp-embers/src/lib.rs`:
- Around line 511-526: The kill_buffer method currently hardcodes force: true
when creating the BufferRequest::Kill message. Add a force parameter to the
kill_buffer method signature to allow callers to control this behavior, then use
the provided parameter value instead of the hardcoded true in the
BufferRequest::Kill construction. This aligns the public API with caller needs
and matches the pattern of similar methods like kill_session that expose the
force parameter.
🪄 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: fbe97243-21da-4b03-94a1-71c888f3c67f
⛔ Files ignored due to path filters (1)
crates/wisp-embers/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/ci.yml.github/workflows/prepare-release.yml.github/workflows/release.yml.gitignoreCargo.tomlcrates/wisp-app/src/lib.rscrates/wisp-bin/src/main.rscrates/wisp-core/benches/projections.rscrates/wisp-core/src/domain.rscrates/wisp-core/src/reduce.rscrates/wisp-core/src/view.rscrates/wisp-embers/Cargo.tomlcrates/wisp-embers/src/lib.rscrates/wisp-preview/src/lib.rsdocs/configuration.md
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Install Rust toolchain | ||
| uses: dtolnay/rust-toolchain@stable |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unpinned action refs and checkout credential persistence in workflows.
rg -n 'uses:\s*[^@]+@(?:v[0-9]+|stable|main|master)$' .github/workflows
rg -n -A4 -B2 'uses:\s*actions/checkout@' .github/workflowsRepository: Pajn/wisp
Length of output: 3576
🏁 Script executed:
cat -n .github/workflows/ci.yml | sed -n '50,95p'Repository: Pajn/wisp
Length of output: 2214
Pin third-party actions to commit SHAs and disable checkout credential persistence in the embers job.
Lines 64, 67, and 86 use mutable tags (@v4, @stable, @v2). The checkout action should pin to a commit SHA and set persist-credentials: false unless a later step requires git authentication. For example:
Example fix
- name: Checkout
uses: actions/checkout@eef61447b9ff4aafe5dcd4e0bbf5d482be7e7871 # v4.2.1
with:
persist-credentials: false
- name: Install Rust toolchain
uses: dtolnay/rust-toolchain@1482605baf5caebd8b2feb1143f460cce50e0a94 # stable🧰 Tools
🪛 zizmor (1.25.2)
[warning] 63-64: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 64-64: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 67-67: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 67-67: action functionality is already included by the runner (superfluous-actions): use rustup and/or cargo in a script step
(superfluous-actions)
🤖 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 @.github/workflows/ci.yml around lines 63 - 67, Replace the mutable action
version tags with pinned commit SHAs in the workflow. For the Checkout action,
change from `@v4` to the full commit SHA
(eef61447b9ff4aafe5dcd4e0bbf5d482be7e7871) and add a with section containing
persist-credentials: false, followed by a comment showing the version. For the
Install Rust toolchain action, change from `@stable` to its commit SHA
(1482605baf5caebd8b2feb1143f460cce50e0a94) and add a comment showing the
version. Repeat this pattern for any other mutable tags referenced on lines 86
and elsewhere in the workflow file.
Source: Linters/SAST tools
| pub fn kill_buffer(&self, buffer_id: &str) -> Result<(), EmbersError> { | ||
| self.with_state(|runtime, state| { | ||
| let buffer_id = parse_buffer_id(buffer_id)?; | ||
| let response = runtime.block_on(state.client.request_message( | ||
| ClientMessage::Buffer(BufferRequest::Kill { | ||
| request_id: state.client.next_request_id(), | ||
| buffer_id, | ||
| force: true, | ||
| }), | ||
| ))?; | ||
| match response { | ||
| ServerResponse::Ok(_) => Ok(()), | ||
| _ => Err(EmbersError::UnexpectedResponse("buffer kill")), | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how kill_buffer is called downstream to understand if force:true is appropriate
rg -n -C3 'kill_buffer' --type rustRepository: Pajn/wisp
Length of output: 1453
🏁 Script executed:
# View the kill_session method to understand force: false usage
sed -n '250,275p' crates/wisp-embers/src/lib.rsRepository: Pajn/wisp
Length of output: 1100
🏁 Script executed:
# Get broader context around the kill_buffer calls in main.rs
sed -n '1100,1140p' crates/wisp-bin/src/main.rsRepository: Pajn/wisp
Length of output: 1124
🏁 Script executed:
# Search for any documentation or comments about the force flag
rg -n -B2 -A2 'force.*:' crates/wisp-embers/src/lib.rs | head -50Repository: Pajn/wisp
Length of output: 440
🏁 Script executed:
# Search for BufferRequest enum definition to understand Kill operation semantics
rg -n 'enum BufferRequest\|BufferRequest::Kill' --type rust -A3Repository: Pajn/wisp
Length of output: 35
🏁 Script executed:
# Check if there are any other callers of kill_buffer beyond the two found
rg -n 'kill_buffer' --type rustRepository: Pajn/wisp
Length of output: 331
🏁 Script executed:
# Search for any comments or documentation about force flag in the codebase
rg -n 'force.*buffer\|buffer.*force' --type rust -iRepository: Pajn/wisp
Length of output: 35
🏁 Script executed:
# Check the broader context of kill_buffer method - any comments or API documentation
sed -n '505,530p' crates/wisp-embers/src/lib.rsRepository: Pajn/wisp
Length of output: 1011
🏁 Script executed:
# Check if there's any public API documentation or module comments about kill_buffer
rg -n 'kill_buffer' crates/wisp-embers/src/lib.rs -B10 | head -30Repository: Pajn/wisp
Length of output: 427
Current usage of force: true is appropriate for error recovery cleanup, but consider adding a force parameter for API flexibility.
The method is only called in error handling paths (lines 1112, 1129 in main.rs) where automatic cleanup is needed after failed buffer operations. In these contexts, forcing the kill unconditionally makes sense. However, as a public API method, hardcoding force: true removes caller flexibility. If this method is intended for general use beyond internal cleanup, exposing the force parameter would align the API with caller needs and match the pattern of similar methods like kill_session (which uses force: false for session termination).
🤖 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-embers/src/lib.rs` around lines 511 - 526, The kill_buffer method
currently hardcodes force: true when creating the BufferRequest::Kill message.
Add a force parameter to the kill_buffer method signature to allow callers to
control this behavior, then use the provided parameter value instead of the
hardcoded true in the BufferRequest::Kill construction. This aligns the public
API with caller needs and matches the pattern of similar methods like
kill_session that expose the force parameter.
|
@coderabbitai review |
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/configuration.md (1)
75-76: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winClarify Embers server prerequisite.
The phrase "expects the local Embers checkout used by the adapter crate" is vague and uses implementation jargon. Users need to understand that a running Embers server/instance is required. Consider rephrasing to: "...when Wisp is built with that feature and
backend.kind = "embers", provided a running Embers server is available at the configured socket path, ..."🤖 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 `@docs/configuration.md` around lines 75 - 76, The documentation in docs/configuration.md contains vague implementation jargon in the Embers runtime support section. Replace the phrase "expects the local Embers checkout used by the adapter crate" with clearer language that explicitly states the user-facing prerequisite of a running Embers server. Rephrase to something like "provided a running Embers server is available at the configured socket path" to make it immediately clear to users that an Embers server instance must be running for the feature to work, rather than describing an internal implementation detail about checkouts and adapter crates.
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
64-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin embers job actions to commit SHAs and disable persisted checkout credentials.
Line 65, Line 68, and Line 88 still use mutable action refs, and checkout does not set
persist-credentials: false. This weakens CI supply-chain and token handling guarantees.Suggested patch
- name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@<pinned_commit_sha> + with: + persist-credentials: false - name: Install Rust toolchain - uses: dtolnay/rust-toolchain@stable + uses: dtolnay/rust-toolchain@<pinned_commit_sha> with: toolchain: 1.95.0 components: clippy @@ - name: Cache cargo artifacts - uses: swatinem/rust-cache@v2 + uses: swatinem/rust-cache@<pinned_commit_sha>Also applies to: 87-88
🤖 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 @.github/workflows/ci.yml around lines 64 - 68, Pin both the actions/checkout and dtolnay/rust-toolchain action references to specific commit SHAs instead of using version tags (v4 and stable respectively). For the actions/checkout action, additionally add the parameter persist-credentials: false to disable persisted checkout credentials. Apply these changes to the instances at lines 65 and 68, and also to the similar action references at line 88 mentioned in the "Also applies to" section.Source: Linters/SAST tools
🤖 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 `@Cargo.toml`:
- Line 19: The rust-version field in Cargo.toml is currently set to 1.95.0, but
the codebase uses Rust 2024 edition which only requires 1.85.0. Review the
dependencies and language features used in the codebase to determine if 1.95.0
is actually the true minimum required version. If no specific features from
versions between 1.85.0 and 1.95.0 are being used, lower the rust-version field
to 1.85.0 to reduce the MSRV requirement and its impact on developers.
In `@crates/wisp-bin/src/main.rs`:
- Around line 118-125: The Drop implementation for TerminalTeardown currently
only disables raw mode and leaves the alternate screen, but does not restore
cursor visibility. If the process exits abnormally without calling restore(),
the cursor will remain hidden. In the drop method of the TerminalTeardown Drop
impl, add a call to show the cursor (using the appropriate crossterm cursor
command, similar to how LeaveAlternateScreen is executed) alongside the existing
disable_raw_mode and execute calls, wrapping it with let _ = to ignore potential
errors, ensuring complete terminal cleanup in all exit paths.
---
Outside diff comments:
In `@docs/configuration.md`:
- Around line 75-76: The documentation in docs/configuration.md contains vague
implementation jargon in the Embers runtime support section. Replace the phrase
"expects the local Embers checkout used by the adapter crate" with clearer
language that explicitly states the user-facing prerequisite of a running Embers
server. Rephrase to something like "provided a running Embers server is
available at the configured socket path" to make it immediately clear to users
that an Embers server instance must be running for the feature to work, rather
than describing an internal implementation detail about checkouts and adapter
crates.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 64-68: Pin both the actions/checkout and dtolnay/rust-toolchain
action references to specific commit SHAs instead of using version tags (v4 and
stable respectively). For the actions/checkout action, additionally add the
parameter persist-credentials: false to disable persisted checkout credentials.
Apply these changes to the instances at lines 65 and 68, and also to the similar
action references at line 88 mentioned in the "Also applies to" section.
🪄 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: 95acffde-bb83-4c84-92a8-10262f808ea8
⛔ Files ignored due to path filters (1)
crates/wisp-embers/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/ci.yml.github/workflows/prepare-release.yml.github/workflows/release.yml.gitignoreCargo.tomlcrates/wisp-app/src/lib.rscrates/wisp-bin/src/main.rscrates/wisp-core/benches/projections.rscrates/wisp-core/src/domain.rscrates/wisp-core/src/reduce.rscrates/wisp-core/src/view.rscrates/wisp-embers/Cargo.tomlcrates/wisp-embers/src/lib.rscrates/wisp-preview/src/lib.rsdocs/configuration.md
| version = "0.2.0" | ||
| edition = "2024" | ||
| rust-version = "1.90" | ||
| rust-version = "1.95.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify Rust 1.95.0 is a valid released version and check for any syntax/features requiring it
# Check if Rust 1.95.0 is mentioned in release notes or is valid
rg -i "rust.*1\.95" --type toml --type md
# Check for any edition2024 or recent language features that might require newer Rust
rg -n "edition.*=.*\"202[4-9]\"" --type tomlRepository: Pajn/wisp
Length of output: 254
🌐 Web query:
Rust 1.95.0 release date version
💡 Result:
Rust version 1.95.0 was released on April 16, 2026 [1][2][3].
Citations:
- 1: https://doc.rust-lang.org/stable/releases.html
- 2: https://blog.rust-lang.org/2026/04/16/Rust-1.95.0/
- 3: https://releases.rs/docs/1.95.0/
🌐 Web query:
Rust edition 2024 release status
💡 Result:
Rust 2024 is already released and stable as of today (2026-06-20). It was stabilized and released with Rust 1.85.0 on 2025-02-20.[1][2][3] Status timeline (official sources): - Entered beta: 2025-01-09, and stayed in beta until 2025-02-20.[2] - Released to stable (Rust 1.85 / edition 2024 stable): 2025-02-20.[1][2] The edition guide also lists the Rust 2024 release version as 1.85.0.[3]
Citations:
- 1: https://blog.rust-lang.org/2025/02/20/Rust-1.85.0/
- 2: https://blog.rust-lang.org/2025/01/22/rust-2024-beta/
- 3: https://doc.rust-lang.org/beta/edition-guide/rust-2024/index.html
Verify that Rust 1.95.0 is the minimum required version.
Rust 1.95.0 is a valid released version (released April 16, 2026). However, Rust 2024 edition (which the codebase uses) was stabilized with Rust 1.85.0 (released February 20, 2025). Setting the rust-version to 1.95.0 bumps the MSRV significantly beyond what's needed for edition 2024 compatibility. Confirm that specific language features or dependencies in the codebase actually require Rust 1.95.0 rather than 1.85.0, or consider lowering the MSRV to reduce the breaking change impact on developers.
🤖 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 `@Cargo.toml` at line 19, The rust-version field in Cargo.toml is currently set
to 1.95.0, but the codebase uses Rust 2024 edition which only requires 1.85.0.
Review the dependencies and language features used in the codebase to determine
if 1.95.0 is actually the true minimum required version. If no specific features
from versions between 1.85.0 and 1.95.0 are being used, lower the rust-version
field to 1.85.0 to reduce the MSRV requirement and its impact on developers.
a8ae16d to
54dd82d
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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-app/src/lib.rs`:
- Around line 517-524: The previous-session map built in lib.rs currently skips
entries when `snapshot.context.client_id` is missing, unlike the `clients`
fallback and `snapshot_client_id()` in main.rs. Update the
`previous_session_by_client` construction to use the same synthetic `"default"`
client id when `snapshot.context.client_id` is absent, while still pairing it
with `snapshot.context.previous_session_name` so clientless Embers snapshots
preserve the mapping.
In `@crates/wisp-bin/src/main.rs`:
- Around line 611-615: BackendKind::Auto currently hard-fails when
resolve_embers_socket_path returns a path but EmbersClient::connect cannot
establish the connection, instead of degrading to RuntimeBackend::Tmux. Update
the BackendKind::Auto branch in the runtime backend selection logic to treat a
connect failure like a missed detection path: attempt EmbersClient::connect, but
on error fall back to Tmux rather than propagating the failure, while still
keeping the successful Embers path unchanged.
- Around line 661-665: The Embers status check in BackendKind::Embers is using
resolve_embers_socket_path as a proxy for availability, which can report
“available” even when the socket file is missing or the server is dead. Update
this branch to perform a real connection/health check before printing the
available message, and only fall back to “socket not configured” or an
unavailable message when the connection attempt fails.
In `@crates/wisp-config/src/lib.rs`:
- Around line 635-638: The Embers socket resolution in the config loading path
currently treats an empty string as a configured socket because
`PathBuf::from("")` still becomes `Some(_)`. Update the resolution/validation
logic in `wisp-config` around `config.embers.socket_path` so
`WISP_EMBERS_SOCKET` and `embers.socket_path` reject empty values or normalize
them to `None`, and make sure the same check is applied in the other referenced
resolution path as well.
In `@crates/wisp-embers/src/lib.rs`:
- Around line 409-415: The sidebar handoff logic in buffer handling is only
detaching when the buffer belongs to a different session, which leaves
same-session buffers still attached before `JoinBufferAtNode` reuses them at the
root. Update the `buffer_location`/`location.session_id()` check so the existing
attachment is detached whenever the buffer is being rejoined through this
handoff path, including when `location.session_id() == Some(session_id)`, and
keep the fix localized to the branch around `detach_buffer_record` in
`crates/wisp-embers/src/lib.rs`.
In `@docs/configuration.md`:
- Around line 149-157: Update the documentation entry for WISP_NO_ZOXIDE so it
matches the actual boolean parsing and negation behavior in the zoxide
configuration handling. In the configuration docs table, clarify that only
truthy values disable zoxide, while 0/false/off keep zoxide enabled, and make
sure the wording aligns with the existing zoxide.enabled override semantics.
In `@README.md`:
- Around line 136-140: The README quality-gate commands still omit the new
wisp-embers crate because the workspace-wide cargo invocations do not cover
crates excluded from Cargo.toml. Update the documented commands near the
test/lint section to explicitly include crates/wisp-embers for clippy and unit
tests, alongside the existing integration test entry, so the crate is validated
by all relevant quality gates.
🪄 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: 510ea491-5caa-49e4-9b76-8416f83962f4
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcrates/wisp-embers/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.github/workflows/ci.yml.github/workflows/prepare-release.yml.github/workflows/release.yml.gitignoreCargo.tomlREADME.mdcrates/wisp-app/Cargo.tomlcrates/wisp-app/src/lib.rscrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/main.rscrates/wisp-bin/tests/smoke.rscrates/wisp-config/src/lib.rscrates/wisp-core/benches/projections.rscrates/wisp-core/src/domain.rscrates/wisp-core/src/reduce.rscrates/wisp-core/src/view.rscrates/wisp-embers/Cargo.tomlcrates/wisp-embers/src/lib.rscrates/wisp-embers/tests/integration.rscrates/wisp-preview/src/lib.rscrates/wisp-zoxide/tests/integration.rsdocs/config.schema.tomldocs/configuration.md
53bfe09 to
4378c5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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-app/src/lib.rs`:
- Around line 394-405: The tmux snapshot focus IDs are being stored in a
different namespace than the projected pane IDs, so focused-pane lookups cannot
match entries in WindowRecord.panes. Update the client construction in the
clients mapping that builds ClientFocus so pane_id uses the same synthetic
tmux-style identifier as the panes created in this function, rather than copying
snapshot.context.pane_id directly. Keep the session/window naming consistent
with the pane projection logic so the focus record and projected panes resolve
to the same identity.
In `@crates/wisp-bin/src/main.rs`:
- Around line 574-585: selected_backend_kind currently short-circuits to
BackendKind::Embers from a socket hint, which can disagree with the real runtime
fallback behavior in load_runtime_backend(). Update selected_backend_kind to use
the same Embers connect-and-fallback logic as load_runtime_backend(), or change
doctor() and the statusline precheck to rely on the resolved RuntimeBackend
instead of this config-only helper. Keep the behavior aligned for
BackendKind::Auto so stale or unreachable Embers hints still fall back to tmux
consistently.
In `@crates/wisp-bin/tests/smoke.rs`:
- Around line 105-110: The smoke tests are still influenced by user config
because `load_runtime_config()` can pick up `WISP_CONFIG`, XDG, or HOME-backed
settings, so a local `[backend]` or `[embers].socket_path` may change the
result. Update the spawned `Command` in the relevant smoke test cases to isolate
them from config by forcing `WISP_CONFIG` to a temp nonexistent path or
otherwise scrubbing the config-path environment before running `doctor`, while
keeping the existing env setup for `WISP_BACKEND` and the socket variables.
In `@crates/wisp-config/src/lib.rs`:
- Around line 635-637: The `from_environment()` handling for
`WISP_EMBERS_SOCKET` is normalizing the value too early, which prevents an empty
env override from clearing an existing file value. Update the
`WISP_EMBERS_SOCKET` branch in `from_environment()` to store the raw
`Some(PathBuf::from(value))` in the partial config instead of calling
`non_empty_socket_path`, and rely on the existing normalization during
resolve/merge for `embers.socket_path`. Make the same adjustment anywhere else
the env-to-config path for `embers.socket_path` is normalized so `merge_option`
can correctly apply the override.
In `@crates/wisp-embers/src/lib.rs`:
- Around line 187-193: In switch_session and any equivalent branch around the
no-op session switch path, avoid updating previous_session_id when the resolved
target session is already the current one. Check the current session from
current_client_record against the session returned by resolve_session_by_name,
and only assign state.previous_session_id and call switch_current_session when
the IDs differ. Keep the existing resync_all_sessions and session resolution
flow, but make the no-op branch return without changing bookkeeping.
In `@docs/config.schema.toml`:
- Around line 9-16: Update the backend config docs in config.schema.toml to
mention that "kind = \"embers\"" and the [embers] settings only work when the
binary is built with the embers feature. Keep the existing backend::kind
guidance, but add a clear note near the Embers entries that default builds may
not include Embers support and that these options require a feature-enabled
build.
🪄 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: ab9abc1b-4ac8-4cd0-867b-d97980ee4fa3
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcrates/wisp-embers/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.github/workflows/ci.yml.github/workflows/prepare-release.yml.github/workflows/release.yml.gitignoreCargo.tomlREADME.mdcrates/wisp-app/Cargo.tomlcrates/wisp-app/src/lib.rscrates/wisp-bin/Cargo.tomlcrates/wisp-bin/src/main.rscrates/wisp-bin/tests/smoke.rscrates/wisp-config/src/lib.rscrates/wisp-core/benches/projections.rscrates/wisp-core/src/domain.rscrates/wisp-core/src/reduce.rscrates/wisp-core/src/view.rscrates/wisp-embers/Cargo.tomlcrates/wisp-embers/src/lib.rscrates/wisp-embers/tests/integration.rscrates/wisp-preview/src/lib.rscrates/wisp-zoxide/tests/integration.rsdocs/config.schema.tomldocs/configuration.md
| let output = Command::new(bin()) | ||
| .arg("doctor") | ||
| .env("WISP_BACKEND", "embers") | ||
| .env_remove("WISP_EMBERS_SOCKET") | ||
| .env_remove("EMBERS_SOCKET") | ||
| .output() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Isolate these smoke tests from user config.
Removing the socket env vars is not enough here: load_runtime_config() still reads $WISP_CONFIG / XDG / HOME config, so a developer-local [backend] or [embers].socket_path can flip both outcomes. Force WISP_CONFIG to a temp nonexistent path (or scrub the config-path env) in the spawned command so these tests only exercise the env setup in the test body.
Also applies to: 147-152
🤖 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/tests/smoke.rs` around lines 105 - 110, The smoke tests are
still influenced by user config because `load_runtime_config()` can pick up
`WISP_CONFIG`, XDG, or HOME-backed settings, so a local `[backend]` or
`[embers].socket_path` may change the result. Update the spawned `Command` in
the relevant smoke test cases to isolate them from config by forcing
`WISP_CONFIG` to a temp nonexistent path or otherwise scrubbing the config-path
environment before running `doctor`, while keeping the existing env setup for
`WISP_BACKEND` and the socket variables.
Review comments
1 participant
Summary by CodeRabbit
backend.kindandWISP_BACKEND/WISP_EMBERS_SOCKET(tmux-only statusline). Runtime flows like previews and sidebar navigation are backend-aware.autoselection behavior.native_id), clarified session preview capture errors, and improved recovery from poisoned worker queues.flatcinstall), and updated.gitignore.