Repository navigation
feat: add shell command prompt (default ':') to execute arbitrary commands safely - #35
Conversation
Preserve tree navigation, commit filters, image previews, and branch checkout fixes while preparing the shell prompt integration.
Run prompt and configured commands off the GUI thread with detached noninteractive processes, five-minute deadlines, process-group cancellation/reaping, and bounded stdout/stderr capture. Replace temporary scripts with safe native shell arguments and bash/zsh rc wrappers while preserving sh semantics for configured commands. Honor remapped and disabled prompt bindings in list, diff, and compare views without stealing search or popup input. Preserve literal pasted commands, report errors before stdout, expose scrollable/copyable results, bound command-log previews, and refresh repository/comparison data without losing filters or navigation. Add runner, UI, configuration, rendering, and output regressions; document limitations.
Bring PR Blankeos#35 up to date with Blankeos#41 without reintroducing the removed HEAD-toggle shortcut, footer hints, or configuration. Retain shell prompt binding and legacy-key regression coverage.
|
Updated this branch with a normal fast-forward push to
Final local validation: 360 tests passed, 0 failed, 1 ignored; formatting, diff checks, build, and real-terminal PTY workflow all passed. The PR description now reflects the implementation and its noninteractive/Unix-only limitations. No merge or force-push was performed. |
|
Thanks a lot for the thorough review and great improvements, @Blankeos! 🙌 I just pulled the branch locally and verified everything on Linux:
Everything is running solidly and looks ready to merge whenever you'd like! |
|
Tested interactively in my daily workflow as well and confirmed everything works as expected. All good to merge from my side! 🚀 |
PR review:
|
| Check | Result |
|---|---|
cargo fmt followed by cargo fmt --check |
Passed. |
git diff --check |
Passed. |
macOS cargo test --locked --offline with Bash/zsh/Fish available |
370 passed, 0 failed, 1 ignored. |
macOS cargo build --locked --offline |
Passed; 50 warnings remain. |
Linux ARM64 cargo test --locked with Bash 5.2, zsh 5.9, Fish 4.0.2 installed |
370 passed, 0 failed, 1 ignored. |
Linux ARM64 cargo build --locked |
Passed; 50 warnings remain. |
| macOS Fish 4.9.3 focused execution suite | 25 passed; Fish execution actually ran, not an early-return skip. Temporary verified Homebrew bottle outside the project; no system installation. |
| Real-terminal PTY smoke, isolated repos/config/state | Passed: prompt, cwd, stdout/stderr, exit 7, cancellation, clean quit; actual Ctrl-G → arm Escape → open/close shell prompt leaves AI alive and restores draft; rebase abort output remains until dismissal before notification; commit selection beyond entry 300 and drilled files survive completion refresh. |
| Independent final static review | No remaining actionable defect found. |
| Live main/PR check | Main unchanged at 6271245; remote PR head remains fe49a49, OPEN/MERGEABLE, approval required. |
Linux ran in an automatically removed disposable Docker container with the project mounted read-only, using Rust 1.99.0 and an isolated Cargo target directory. macOS also used Rust 1.99.0. The ignored test is the existing manual checkout-visibility timing benchmark. Test fixtures and helper dependencies stayed in temporary directories; no user Git config or installed shell was changed. Existing unrelated compiler warnings were not blindly auto-fixed.
Logs/harness:
/tmp/lazygitrs-pr35-fixes-tests.log/tmp/lazygitrs-pr35-fixes-build.log/tmp/lazygitrs-pr35-fixes-fish.log/tmp/lazygitrs-pr35-fixes-linux-final.log/tmp/lazygitrs-pr35-fixes-pty.pyand/tmp/lazygitrs-pr35-fixes-pty.log
Remaining merge checklist and confidence bumps
- All five findings fixed and regression-tested; dependent error/modal edge cases checked.
- Revalidate full tests/build/formatting on macOS and Linux with installed shell execution coverage.
- Fresh real-terminal smoke of rebase, comparison pagination, AI dismissal, and ordinary command behavior.
- Inspect representative user-specific custom commands for persistent background children, execution longer than five minutes, output beyond the cap, unsupported platforms, and
:collisions. Documented intentional runner constraints remain; repository tests cannot audit private user config. - Review and publish the local diff to the contributor branch when authorized; rerun checks on the exact published commit. No commit/push performed in this fix pass.
- Obtain the required approving review and recheck main/head/mergeability before merging. PR deliberately left open and unmerged.
Confidence bump achieved: fixing R1–R5 plus fresh macOS/Linux/shell/PTY evidence raises the reviewed local implementation from 3/5 → 4.8/5. Compatibility confirmation for representative real configurations plus independent approval on the published SHA would support approximately 4.9/5. Approval/publication are procedural gates, not substitutes for correctness evidence; no score guarantees absence of bugs. Windows remains intentionally unsupported by the runner; Linux x86-64 was not separately executed.
Migrations remain unnecessary: no dependency, schema, persisted-state, or mandatory config migration. README documents shell startup behavior and the existing noninteractive/timeout/output/background-process limits.
Original pre-fix review (historical)
The sections below describe the fe49a49 version before the local fixes above; unresolved language and unchecked items are retained as review history, not current local findings.
Local comparison: fix/pr35-shell-prompt → origin/main (same commits as the PR head and live main).
- PR: #35 — feat: add shell command prompt (default ':') to execute arbitrary commands safely
- Reviewed head:
fe49a4943235365bd2133289b765bc528c74b7f9 - Base:
6271245a712a3bca061fa7788a3163da3f46f193 - Review timestamp:
2026-10-07 16:22:08 UTC/2026-10-08 00:22:08 +08:00 - Scope:
origin/main...HEAD; 10 files, 2,329 additions and 30 deletions. Independent worker and GUI reviews plus local checks.
Short GitHub-comment body
Adds a configurable shell prompt and an asynchronous, bounded, cancellable command runner. Local tests pass (360 passed, 1 ignored), as do formatting, whitespace, and build checks. However, refresh notifications can still overwrite shell popups, comparison refresh loses selections beyond the first commit page, and armed AI cancellation can intercept shell Escape. Bash/zsh also leak wrapper arguments into commands, while Fish startup can move execution outside the promised repository directory. Recommend addressing these edge cases with regression tests before merging. No schema/data migration is required; existing custom-command behavior needs a compatibility audit. Merge confidence: 3/5.
Findings
All five findings are P2 / medium: concrete, conditional correctness problems, not a claim that the ordinary execution path is broken. No P0/P1 issue was identified in this review. Passing tests do not cover the scenarios below.
R1 — Background refresh can overwrite the shell-owned popup
Locations: src/gui/mod.rs:2762–2765, 8781–8796; new prompt dispatch at 3643–3653; ownership predicate at 1720–1725.
AI and remote-result receivers respect shell ownership, but other asynchronous popup producers do not. A files-refresh error unconditionally calls show_error, replacing typed shell input or command output. Likewise, after_model_refresh replaces the popup with “Rebase complete.” The prompt is now accessible before rebase-mode dispatch, making this reachable from the rebase-progress view.
Scenario: Open the prompt during an in-progress rebase and execute git rebase --abort or a successful noninteractive git rebase --continue. Shell completion displays output and requests a refresh; the subsequent refresh replaces that output with the rebase notification. A competing refresh can also discard a partially typed command. The abort path additionally exposes the pre-existing misleading “Successfully rebased” notification; this PR does not introduce that wording.
Recommendation: Defer competing notifications while shell input, a job, or output owns the modal. Apply model updates separately from popup presentation. Test both files-refresh failure and rebase completion while typing, running, and viewing output.
Verification: Confirmed by tracing popup assignments and main-loop refresh ordering; this exact UI scenario was not executed locally.
R2 — Comparison refresh loses a selected commit beyond the first 300 entries
Locations: src/gui/controller/custom_commands.rs:174–201; reload at src/gui/controller/diff_mode.rs:817–832; reset at src/gui/modes/diff_mode.rs:155–164.
Completion saves the selected commit hash, but reload_diff_files clears all loaded commits and reloads only DEFAULT_COMMIT_LIMIT (300). Restoration searches that first page only. If the previously selected commit was on a subsequently loaded page, selection remains at index zero. Commit exploration then displays a different commit; drilled-down commit files can remain associated with the old hash while the comparison commit selection points elsewhere.
Scenario: Compare histories containing more than 300 commits, load another page, select a commit there, then run a no-op command such as true. The selected hash cannot be restored from the newly truncated list.
Recommendation: Reload at least the previous loaded count, or page until the selected hash is found. Use an explicit fallback only when the commit genuinely disappears. Test both full-commit and commit-file exploration beyond the first page.
Verification: Confirmed from the reload/reset/restore code. Existing shell-comparison preservation coverage uses a short history and does not exercise this case.
R3 — Shell Escape can cancel an unrelated AI job instead of dismissing the popup
Locations: src/gui/mod.rs:3618–3628, 3033–3057; shell dismissal at 4611–4624.
Running shell jobs receive explicit input priority, but shell input/output popups do not. The AI Escape handler runs before popup dispatch and consumes Escape when its cancellation is already armed.
Scenario: With AI generation active, press Escape once to arm cancellation, open : within the 900 ms window, then press Escape to close it. AI generation is cancelled, and the shell prompt remains open. Shell output has the same ordering problem if cancellation was armed earlier.
Recommendation: Route shell-popup keys before AI cancellation handling, or explicitly exclude shell-owned modals from the AI Escape handler. Assert both popup dismissal and preservation of the unrelated AI job.
Verification: Confirmed from input dispatch and the cancellation state machine; this timing-dependent UI sequence was not executed locally.
R4 — Bash/zsh expose internal wrapper arguments to the user's command
Locations: src/os/shell.rs:135–156, 179–187.
The wrappers save the repository path and command from $1/$2, but never clear positional parameters before eval. Commands therefore see the implementation arguments through $#, $1, and "$@"; an rc file can also leave its own parameters behind. This differs from native shell -c command without additional arguments and can silently change argument-forwarding snippets.
Verification: Executed the actual wrapper strings read from the source using isolated HOME/ZDOTDIR values and a print-only probe:
/bin/bash wrapper: argc=2 arg1=/Users/carlo/Desktop/Projects/lazygitrs
/bin/bash native: argc=0 arg1=unset
/bin/zsh wrapper: argc=2 arg1=/Users/carlo/Desktop/Projects/lazygitrs
/bin/zsh native: argc=0 arg1=unset
Recommendation: Clear positional parameters after rc loading and before evaluating the saved command. Add tests for $#, $1, and "$@", including rc files that run set -- ....
R5 — Fish configuration can move execution outside the repository root
Location: src/os/shell.rs:189–193; repository-root promise in README.md:75–76.
Bash/zsh restore the repository directory after configuration loading. Other shells only receive an initial current_dir plus native -c. Fish executes configuration for noninteractive shells as well; an unconditional cd in config.fish changes the directory before the command runs. Relative-path commands may therefore affect a different directory or repository.
Scenario: With $SHELL=fish, configure an unconditional startup cd to another directory; execute pwd or a relative-path Git command in the prompt.
Recommendation: Restore cwd after startup using a Fish-native, safely parameterized wrapper, or explicitly narrow the documented guarantee. Test an isolated Fish config that changes directory.
Verification: Code inspection plus Fish's configuration-file documentation, which states configuration executes on every shell startup. Not locally reproduced: Fish is not installed. The existing optional Fish execution test returns early when Fish is unavailable, despite appearing as a passing test.
Regressions and compatibility
Yes — conditional regressions/new-feature correctness gaps are identified above. The highest-impact existing-feature interactions are modal replacement, comparison navigation, and AI cancellation. The wrapper-argument and Fish-cwd issues concern new prompt execution semantics.
Positive evidence:
mainis already an ancestor of HEAD; no current integration conflict.- Tests cover tree navigation/footer behavior and restored Ctrl-G generation; no regression was identified in those covered paths. The removed HEAD toggle is not reintroduced.
- The runner has strong coverage for cancellation, deadlines, output bounds, process-group cleanup, descendant-pipe draining, and stdin EOF.
- Old configuration remains readable:
UniversalKeybindinguses#[serde(default)], and omitted/remapped/disabled prompt-binding tests pass.
Intentional compatibility changes requiring awareness:
- Existing configured
customCommandsstill usesh -c, but now have a five-minute deadline, bounded output, cancellation, and termination of same-group background children even on successful completion. - Configured commands intended to launch persistent background services may stop working as intended. Commands expecting complete output larger than 1 MiB per stream will be truncated.
- The shared runner rejects non-Unix platforms, including configured commands. Confirm the supported-platform policy if anyone relies on those commands outside macOS/Linux.
- Process-group/session escape is documented; the runner is not a sandbox. No sandbox guarantee should be inferred from the PR title.
Checks performed for this review
| Check | Result |
|---|---|
gh pr view --json number,title,headRefName |
No PR found for local fix/pr35-shell-prompt; explicit gh pr view 35 --json ... resolved the contributor PR. |
| Live PR head vs local HEAD | Exact match at fe49a49. |
git ls-remote origin refs/heads/main |
Live main matches local main/origin/main at 6271245. No fetch was needed. |
git merge-base --is-ancestor origin/main HEAD |
Passed. |
git diff --check origin/main...HEAD |
Passed. |
cargo fmt --check |
Passed; formatting was checked, not rewritten. |
cargo test --locked --offline |
360 passed, 0 failed, 1 ignored. |
cargo build --locked --offline |
Passed; reports 50 warnings. |
| Bash/zsh wrapper-vs-native argument probe | Reproduced R4 on both installed shells. |
| GitHub checks for exact reviewed SHA | Release plan succeeded; artifact build and publication stages skipped. Not Linux test/build evidence. |
| PR mergeability/protection | OPEN, MERGEABLE, but BLOCKED / REVIEW_REQUIRED. One approving review required; no required status-check list configured. |
| Tracked worktree and index | Unchanged after checks. Existing untracked PR_REVIEW_20261007_160335Z.md preserved. |
Environment: macOS, Rust/Cargo 1.99.0. Tests/builds may update ignored Cargo artifacts and isolated temporary test fixtures; no application source, configuration, real repository history, branch refs, or PR state was changed. Only this requested review document was added to the worktree.
Logs: /tmp/lazygitrs-pr35-review-tests-20261008.log and /tmp/lazygitrs-pr35-review-build-20261008.log.
Not performed: Linux execution/build, Fish execution, new regression-test implementation, or a fresh PTY UI smoke run. The PR description reports a prior macOS PTY smoke run, but it is not counted as newly verified here. No cross-platform success is inferred from macOS tests or release planning.
Migrations
No schema, database, persisted-state, dependency, or mandatory configuration migration is required. Cargo.toml and Cargo.lock are unchanged. The new binding defaults to : when omitted and can be remapped or disabled with "".
A manual compatibility audit, not a data migration, is advisable for existing custom commands that rely on long execution, full unbounded output, persistent background children, or non-Unix execution. Also inspect any existing : shortcut for a collision: the universal prompt has priority over panel shortcuts, but not active text entry/popups.
Before-merge checklist
Completed during this review:
- Confirm PR identity and exact head/base refs against GitHub.
- Confirm latest main is integrated and the PR has no merge conflict.
- Run formatting, whitespace, locked tests, and locked build checks.
- Check configuration defaults and dependency/state migration scope.
- Independently inspect worker lifecycle and GUI integration.
Still needed:
- R1: Protect shell modal ownership from files-refresh errors and rebase-completion notifications; add regressions for each shell UI phase.
- R2: Preserve paginated comparison selection and commit-file consistency; test a selected commit beyond entry 300.
- R3: Prevent shell Escape from cancelling an armed AI job; test actual key-dispatch ordering.
- R4: Clear Bash/zsh positional arguments before command evaluation; test both ordinary and rc-mutated parameters.
- R5: Guarantee or explicitly qualify Fish post-startup cwd; test with Fish installed and a startup
cd. - Rerun the full tests/build/formatting checks after fixes on the resulting head.
- Run Linux tests/build and shell-cleanup checks with Bash, zsh, and Fish actually installed; avoid silently skipped shell coverage.
- Run a fresh isolated PTY smoke covering prompt/output, cancellation, rebase notifications, comparison pagination, and concurrent AI cancellation.
- Audit representative existing custom commands against the new timeout/output/background-process/platform constraints; check binding collisions.
- Obtain the required approving review and recheck live main/head/mergeability immediately before merge.
Confidence bumps
These are conditional, cumulative estimates, not scores already earned. A green checklist alone does not prove the absence of bugs.
| Accomplishment | Why confidence improves | New estimated score |
|---|---|---|
| Fix R1–R3 with focused regression tests | Closes the identified existing-feature/modal/navigation regressions. | 3.8/5 |
| Also fix R4–R5 and rerun all local checks | Aligns shell arguments and cwd with expected execution behavior, closing all five findings. | 4.3/5 |
| Also validate Linux, installed Fish/zsh/Bash, and a fresh PTY smoke on the fixed head | Adds missing platform/shell and real input-loop evidence beyond unit tests. | 4.8/5 |
Custom-command compatibility review removes rollout uncertainty; the approving review removes the current procedural merge block. Neither is a substitute for the correctness fixes. Recommendation remains request changes, with the PR left open and unmerged.
Overview
Adds a configurable shell command prompt (default
:), inspired by lazygit, so commands can run from the repository root without suspending or exiting lazygitrs. The prompt is available in normal panels, diff focus, and comparison mode.Behavior
keybinding.universal.customCommandPromptdefaults to":", supports remapping, and can be disabled with"". Text entry and existing popups retain priority; footer hints and the command palette reflect the configured binding.Enterexecutes;Escdismisses without execution.EscorCtrl-Ccancels; a five-minute timeout bounds execution. Cleanup terminates the job group and reaps the shell, including when the app drops a running job.$SHELL(fallbacksh). Bash explicitly loads~/.bash_aliasesand~/.bashrcwith alias expansion; zsh loads${ZDOTDIR:-$HOME}/.zshrc. Shell configuration cannot accidentally leave execution in a different working directory. Fish and other shells use their native-csyntax. No temporary execution scripts are needed.customCommandsretainsh -cexecution semantics, but use the same asynchronous cancellation, timeout, output, and cleanup path.Integration
Merged current
maininto the contributor branch without rewriting history. Preserves #33 tree navigation and #41's restored Ctrl-G commit generation; does not reintroduce the removed HEAD toggle.Validation
Validated locally on macOS at
fe49a49:cargo test --locked: 360 passed, 0 failed, 1 ignored.cargo fmt --check,git diff --check, andcargo build --locked: passed. Existing build warnings remain.Limitations
This is noninteractive execution, not an embedded terminal: editors and password prompts are unsupported. Aliases/functions available only in an already-running interactive shell are not inherited; rc-file noninteractive guards may skip definitions. Background jobs in the command's process group are terminated on completion, so this is not for persistent services. Processes that explicitly create a separate process group/session can escape cleanup; this is not a sandbox. The runner currently requires Unix process-group support (macOS/Linux); non-Unix platforms receive an explicit unsupported error. Linux/cross-platform builds were not run locally.