Conversation
The existing `run_cmd_exec_funcs` support called whatever `precmd_functions` and `preexec_functions` happened to contain, with no setup and no status handling. Make it a real implementation of that contract, in a `brush-interactive::zsh_hooks` module of its own. `init_zsh_style_hooks` seeds the two registries and claims the `bash_preexec_imported`/`__bp_imported` inclusion guards before profile and rc files load, so a copy of bash-preexec sourced later finds itself "already loaded" and leaves the hooks to us. The guards aren't exported, so a child bash still gets the real implementation. Calling it, and calling it before config files load, is a documented requirement of the feature. Dispatch honors the rest: `precmd` runs ahead of `PROMPT_COMMAND`; every hook sees the last command's `$?`, `PIPESTATUS`, and `$_`, restored afterwards so neither `PROMPT_COMMAND` nor the next command sees a hook's leavings; a failing hook is reported and the rest still run; a hook that calls `exit` exits the shell. `preexec` fires only for a line that runs a command, matching the `DEBUG` trap it stands in for, and receives the line as typed. Terminal integration now emits its OSC 633 command markers as a pair from the one path that runs the command. A line a `preexec` hook exited out of never runs and gets neither marker; a `bind -x` command does run and now gets both, where it previously emitted a lone `D`. The integration utility takes on the writing itself and is no longer optional: a shell without integration holds one that reports no capabilities, so every event reported to it does nothing. The atuin e2e adapter turns the feature on, since `atuin init bash` inlines a copy of bash-preexec. Contract documented in docs/reference/zsh-hooks.md. BREAKING CHANGE: `InteractiveOptions::run_cmd_exec_funcs` is renamed to `zsh_style_hooks`, and embedders must now call `init_zsh_style_hooks` before loading profile and rc files. Only shell functions are dispatched, matching zsh. Because a sourced bash-preexec now stands down rather than installing itself, the `BP_PIPESTATUS` copy it used to provide is gone; hooks read the real `PIPESTATUS`, which brush restores for them. Assisted-By: Claude Opus 5
There was a problem hiding this comment.
🟡 Changes recommended
Direct hook invocation misses errexit handling for hooks that explicitly return a nonzero status.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Implements native zsh-style hooks and integrates them with interactive execution, terminal markers, documentation, and atuin testing.
Changes:
- Initializes and dispatches
precmd/preexechooks with preserved command state. - Pairs terminal command markers across typed and bound commands.
- Documents and comprehensively tests the hook contract and breaking API rename.
File summaries
| File | Description |
|---|---|
e2e/README.md |
Documents config-based feature activation. |
e2e/atuin/entrypoint.sh |
Enables native hooks for atuin tests. |
docs/reference/zsh-hooks.md |
Defines the hook contract. |
docs/reference/README.md |
Links the new reference. |
docs/reference/experimental.md |
Expands experimental-feature guidance. |
brush-shell/tests/reedline_interactive_tests.rs |
Tests bound-command hooks and markers. |
brush-shell/tests/interactive_tests.rs |
Tests initialization and marker behavior. |
brush-shell/tests/cases/brush/zsh_hooks.yaml |
Adds compatibility coverage. |
brush-shell/src/entry.rs |
Initializes hooks before startup files. |
brush-interactive/src/zsh_hooks.rs |
Implements hook initialization and dispatch. |
brush-interactive/src/term_integration.rs |
Encapsulates terminal event output. |
brush-interactive/src/options.rs |
Maps the renamed option. |
brush-interactive/src/lib.rs |
Exports hook initialization. |
brush-interactive/src/interactive_shell.rs |
Integrates hooks and paired markers. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| match shell | ||
| .invoke_function(&hook_name, arg, shell.default_exec_params()) | ||
| .await | ||
| { | ||
| // Returned without restoring: the shell is exiting, and `$?` belongs to the hook | ||
| // that exited it. | ||
| Ok(result) if result.is_exit() => return Ok(ControlFlow::Break(result)), | ||
| Ok(_) => {} | ||
| Err(e) => { | ||
| let mut stderr = shell.stderr(); | ||
| let _ = shell.display_error(&mut stderr, &e); | ||
| } | ||
| } |
Test Results 5 files 49 suites 23m 47s ⏱️ For more details on these failures, see this check. Results for commit 59306f2. |
Public API changes for crate: brush-interactiveRemoved itemsAdded itemsPerformance Benchmark Report
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is Test Summary: bash-completion test suite
|
The existing
run_cmd_exec_funcssupport called whateverprecmd_functionsandpreexec_functionshappened to contain, with no setup and no status handling. Make it a real implementation of that contract, in abrush-interactive::zsh_hooksmodule of its own.init_zsh_style_hooksseeds the two registries and claims thebash_preexec_imported/__bp_importedinclusion guards before profile and rc files load, so a copy of bash-preexec sourced later finds itself "already loaded" and leaves the hooks to us. The guards aren't exported, so a child bash still gets the real implementation. Calling it, and calling it before config files load, is a documented requirement of the feature.Dispatch honors the rest:
precmdruns ahead ofPROMPT_COMMAND; every hook sees the last command's$?,PIPESTATUS, and$_, restored afterwards so neitherPROMPT_COMMANDnor the next command sees a hook's leavings; a failing hook is reported and the rest still run; a hook that callsexitexits the shell.preexecfires only for a line that runs a command, matching theDEBUGtrap it stands in for, and receives the line as typed.Terminal integration now emits its OSC 633 command markers as a pair from the one path that runs the command. A line a
preexechook exited out of never runs and gets neither marker; abind -xcommand does run and now gets both, where it previously emitted a loneD. The integration utility takes on the writing itself and is no longer optional: a shell without integration holds one that reports no capabilities, so every event reported to it does nothing.The atuin e2e adapter turns the feature on, since
atuin init bashinlines a copy of bash-preexec. Contract documented in docs/reference/zsh-hooks.md.BREAKING CHANGE:
InteractiveOptions::run_cmd_exec_funcsis renamed tozsh_style_hooks, and embedders must now callinit_zsh_style_hooksbefore loading profile and rc files. Only shell functions are dispatched, matching zsh. Because a sourced bash-preexec now stands down rather than installing itself, theBP_PIPESTATUScopy it used to provide is gone; hooks read the realPIPESTATUS, which brush restores for them.Assisted-By: Claude Opus 5