Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a statically-registered filter infrastructure to brush, enabling interception and modification of shell operations through zero-cost abstractions. The implementation uses Rust's trait system with monomorphization to achieve zero runtime overhead when using default no-op filters.
Changes:
- Adds comprehensive filter infrastructure with
CmdExecFilterandSourceFiltertraits for intercepting command execution and script sourcing operations - Introduces
with_filter!macro to reduce boilerplate at hook sites - Implements
ExternalCommandbuilder for introspectable external process configuration - Integrates filters into shell's command execution and script sourcing paths while maintaining backward compatibility
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/reference/filter-architecture.md | Comprehensive documentation of filter architecture, design philosophy, and usage examples |
| brush-core/src/filter.rs | Core filter infrastructure including traits, parameter types, macro, and extensive unit tests (1371 lines) |
| brush-core/src/extensions.rs | Extends ShellExtensions trait to include CmdExecFilter and SourceFilter types with backward-compatible default parameters |
| brush-core/src/shell.rs | Adds filter storage fields and accessor methods to Shell struct |
| brush-core/src/shell/builder.rs | Adds filter configuration to shell builder options |
| brush-core/src/shell/execution.rs | Integrates SourceFilter hooks into script sourcing operations |
| brush-core/src/commands.rs | Integrates CmdExecFilter hooks into command execution pipeline for both simple and external commands |
| brush-core/src/lib.rs | Exports filter module in public API |
| Cargo.lock | Incidental dependency updates (windows-sys 0.60.2 → 0.61.2) |
Comment on lines
+42
to
+51
|
|
||
| /// Returns a reference to the command execution filter. | ||
| pub const fn cmd_exec_filter(&self) -> &CF { | ||
| &self.cmd_exec_filter | ||
| } | ||
|
|
||
| /// Returns a reference to the source filter. | ||
| pub const fn source_filter(&self) -> &SF { | ||
| &self.source_filter | ||
| } |
There was a problem hiding this comment.
These accessor methods on ShellExtensionsImpl return references to the struct's fields, but these fields are never actually populated with meaningful instances - they're just default instances created by the #[derive(Default)]. The actual filter instances are stored and accessed directly on the Shell struct itself via Shell::cmd_exec_filter() and Shell::source_filter(). These accessor methods appear to be unused dead code. Consider removing them unless there's a specific use case for accessing the extension struct's fields directly.
Suggested change
| /// Returns a reference to the command execution filter. | |
| pub const fn cmd_exec_filter(&self) -> &CF { | |
| &self.cmd_exec_filter | |
| } | |
| /// Returns a reference to the source filter. | |
| pub const fn source_filter(&self) -> &SF { | |
| &self.source_filter | |
| } |
Comment on lines
+554
to
604
| async fn execute_via_external(self, path: &Path) -> Result<ExecutionSpawnResult, error::Error> { | ||
| // Build an ExternalCommand for the filter to inspect/modify. | ||
| let mut ext_cmd = ExternalCommand::new(path); | ||
| for arg in &self.args[1..] { | ||
| if let CommandArg::String(s) = arg { | ||
| ext_cmd.arg(s); | ||
| } | ||
| } | ||
|
|
||
| let resolved_path = path.to_string_lossy(); | ||
| let result = execute_external_command( | ||
| cmd_context, | ||
| resolved_path.as_ref(), | ||
| self.process_group_id, | ||
| &self.args[1..], | ||
| ); | ||
| // Extract fields from self before creating params (which borrows shell). | ||
| let mut shell = self.shell; | ||
| let command_name = self.command_name; | ||
| let params = self.params; | ||
| let process_group_id = self.process_group_id; | ||
| let post_execute = self.post_execute; | ||
|
|
||
| if let Some(post_execute) = self.post_execute { | ||
| let _ = post_execute(&mut shell); | ||
| } | ||
| // Create filter params. | ||
| let filter_params = ExternalCmdParams::new(&shell, ext_cmd); | ||
|
|
||
| result | ||
| crate::with_filter!( | ||
| shell, | ||
| cmd_exec_filter, | ||
| pre_external_cmd, | ||
| post_external_cmd, | ||
| filter_params, | ||
| p => { | ||
| // Extract the (possibly modified) command info. | ||
| let ext_cmd = p.command; | ||
| let new_path = PathBuf::from(ext_cmd.program()); | ||
| let new_args: Vec<CommandArg> = ext_cmd | ||
| .args() | ||
| .iter() | ||
| .map(|a| CommandArg::String(a.to_string_lossy().to_string())) | ||
| .collect(); | ||
|
|
||
| let cmd_context = ExecutionContext { | ||
| shell: &mut shell, | ||
| command_name, | ||
| params, | ||
| }; | ||
|
|
||
| let resolved_path = new_path.to_string_lossy(); | ||
| execute_external_command(cmd_context, resolved_path.as_ref(), process_group_id, &new_args) | ||
| }, | ||
| finally { | ||
| if let Some(post_execute) = post_execute { | ||
| let _ = post_execute(&mut shell); | ||
| } | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
The ExternalCommand provides env, clear_env, and set_current_dir methods, but the current integration in execute_via_external only extracts the program() and args() from the command. The environment variables and current directory configured on ExternalCommand are not actually applied to the spawned process. Either these fields should be used when spawning the process, or the Known Limitations section should explicitly document that environment and current directory modification are not yet supported. The into_std_command() method does convert these fields, but it's never called in the current code.
| pre_source_script, | ||
| post_source_script, | ||
| SourceScriptParams::new(&shell, path, &args), | ||
| |p| run_script(p.path(), p.args()).await |
There was a problem hiding this comment.
The syntax shown here uses closure syntax |p| but the actual with_filter! macro expects arrow syntax p => (without the pipes). This should be changed to match the actual macro implementation.
Suggested change
| |p| run_script(p.path(), p.args()).await | |
| p => run_script(p.path(), p.args()).await |
Test Results 3 files 32 suites 13m 19s ⏱️ Results for commit 17bdb69. ♻️ This comment has been updated with latest results. |
Public API changes for crate: brush-coreRemoved itemsAdded itemsChanged itemsPerformance Benchmark Report
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is Test Summary: bash-completion test suite
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.