feat: extend lifecycle hooks with declarative bundles - #176
Conversation
Summary of ChangesHello @serrrfirat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the extensibility and control within the system by introducing a comprehensive, declarative lifecycle hook mechanism. It allows users and plugins to define custom behaviors at various points in the application's lifecycle through configuration, rather than requiring code changes. This change not only broadens the system's capabilities for customization and integration but also incorporates strong security measures for external communications, ensuring that new functionalities are both powerful and safe. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a declarative lifecycle hook system, a significant feature enhancement. It adds support for bundled, plugin-based, and workspace-based hooks, including rule-based transformations and outbound webhooks. The implementation includes robust security measures for webhooks, such as HTTPS-only enforcement, SSRF protection via hostname and IP validation, and payload redaction. The changes are well-structured across new modules for bootstrapping and bundled hooks, and the core ExtensionManager and HookRegistry are updated to support the new dynamic hook lifecycle. My review includes a couple of suggestions to improve the efficiency of webhook delivery and hook unregistration. Overall, this is a high-quality contribution with strong security considerations.
| async fn unregister_hook_prefix(&self, prefix: &str) -> usize { | ||
| let Some(ref hooks) = self.hooks else { | ||
| return 0; | ||
| }; | ||
|
|
||
| let names = hooks.list().await; | ||
| let mut removed = 0; | ||
| for hook_name in names { | ||
| if hook_name.starts_with(prefix) && hooks.unregister(&hook_name).await { | ||
| removed += 1; | ||
| } | ||
| } | ||
| removed | ||
| } |
There was a problem hiding this comment.
The current implementation of unregister_hook_prefix iterates over all hook names and calls hooks.unregister for each match. Since unregister acquires a write lock on the hooks list, this results in repeated locking and unlocking within the loop, which is inefficient, especially for plugins with many hooks.
To improve performance, consider enhancing HookRegistry with a method that can unregister multiple hooks by prefix in a single operation, for example unregister_by_prefix. This would allow acquiring the write lock only once to remove all matching hooks. ExtensionManager could then call this more efficient method.
| async fn validate_webhook_target_runtime(url: &str) -> Result<(), String> { | ||
| let parsed = reqwest::Url::parse(url).map_err(|e| format!("Invalid URL: {e}"))?; | ||
| let host = parsed | ||
| .host_str() | ||
| .ok_or_else(|| "Webhook URL has no host".to_string())?; | ||
|
|
||
| if let Ok(ip) = host.parse::<IpAddr>() { | ||
| if is_forbidden_ip(ip) { | ||
| return Err(format!("Webhook target resolves to blocked IP {ip}")); | ||
| } | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let port = parsed | ||
| .port_or_known_default() | ||
| .ok_or_else(|| "Webhook URL has no valid port".to_string())?; | ||
|
|
||
| let addrs = tokio::net::lookup_host((host, port)) | ||
| .await | ||
| .map_err(|e| format!("DNS resolution failed: {e}"))?; | ||
|
|
||
| for addr in addrs { | ||
| if is_forbidden_ip(addr.ip()) { | ||
| return Err(format!("Webhook target resolves to blocked IP {}", addr.ip())); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The validate_webhook_target_runtime function performs a DNS lookup for every outbound webhook delivery. For high-frequency webhooks targeting the same host, this can lead to redundant network calls and add latency.
To optimize this, consider introducing a cache for the validation results. A time-based cache (e.g., using the moka crate) could store the validation status of a host for a short period (e.g., 1-5 minutes), avoiding repeated DNS lookups for the same target within that window. This would improve the efficiency of the webhook delivery system.
After merging main (which extracted AppBuilder from main.rs in #198), the ExtensionManager::new() call in app.rs was missing the `hooks` parameter that PR #176 added. This moves HookRegistry creation before init_extensions() and threads it through, matching the existing pattern in main.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat: add bundled and declarative hook bundle loading * fix: load plugin hooks only for active extensions * fix: avoid duplicate plugin hook registration * security: harden outbound webhook hooks * fix: pin webhook DNS resolutions for outbound hooks * fix: block IPv4-mapped local webhook targets * style: format webhook hardening changes for CI * fix: pass HookRegistry to ExtensionManager in AppBuilder After merging main (which extracted AppBuilder from main.rs in nearai#198), the ExtensionManager::new() call in app.rs was missing the `hooks` parameter that PR nearai#176 added. This moves HookRegistry creation before init_extensions() and threads it through, matching the existing pattern in main.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* feat: add bundled and declarative hook bundle loading * fix: load plugin hooks only for active extensions * fix: avoid duplicate plugin hook registration * security: harden outbound webhook hooks * fix: pin webhook DNS resolutions for outbound hooks * fix: block IPv4-mapped local webhook targets * style: format webhook hardening changes for CI * fix: pass HookRegistry to ExtensionManager in AppBuilder After merging main (which extracted AppBuilder from main.rs in nearai#198), the ExtensionManager::new() call in app.rs was missing the `hooks` parameter that PR nearai#176 added. This moves HookRegistry creation before init_extensions() and threads it through, matching the existing pattern in main.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
reject/regex replace/prepend/append), and outbound webhook hookshooks/hooks.json,hooks/*.hook.json) and wire runtime activate/remove flows to register/unregister plugin hooksFEATURE_PARITY.mdstatuses for bundled/plugin/workspace/outbound hooks (withtranscribeAudiointentionally still pending)Testing
cargo test hooks::Notes
transcribeAudiofor a follow-up, matching current scope.