feat: Add support for script execution (trigger & filter) runtime flags.#445
feat: Add support for script execution (trigger & filter) runtime flags.#445TEMHITHORPHE wants to merge 5 commits intoOpenZeppelin:mainfrom
Conversation
- Introduced `runtime_flags` field in the script notification configuration to allow passing runtime-specific flags. - Updated the script execution logic to handle and log runtime flags for Python, JavaScript, and Bash scripts. - Modified the `TriggerConditions` and `TriggerTypeConfig` structs to include `runtime_flags`. - Enhanced the `MonitorBuilder` and related test utilities to support the new runtime flags feature. - Updated various tests to validate the correct handling of runtime flags in script execution.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds support for configurable runtime flags in script execution. A new optional Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant Monitor as Monitor/Trigger
participant Executor as Script Executor
participant Process as OS Process
Config->>Monitor: Load trigger with runtime_flags
Monitor->>Monitor: Store runtime_flags in TriggerConditions
Monitor->>Executor: execute(input, timeout, args, runtime_flags)
Executor->>Executor: Combine input with runtime_flags
Executor->>Process: spawn(script_path, args=[...runtime_flags, ...script_args])
Process-->>Executor: stdout/stderr/exit_code
Executor-->>Monitor: execution_result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
All contributors have signed the CLA ✍️ ✅ |
|
I confirm that I have read and hereby agree to the OpenZeppelin Contributor License Agreement |
|
Hey @shahnami , sorry this took a minute, so I made it a draft cause I wanted to get your thoughts on the PR before I go ahead with updating the docs where necessary. |
There was a problem hiding this comment.
Hello @TEMHITHORPHE thanks for open this PR! The content looks good. Btw you can move the PR to ready for review..
Just posting some comments...
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/tests/builders/trigger.rs (1)
504-520:⚠️ Potential issue | 🟡 MinorMissing assertion for
runtime_flagsin this test.
script_runtime_flagsis called on Line 510 but the match block (Lines 514–517) only asserts onarguments. Theruntime_flagsvalue is never verified, so this test wouldn't catch a brokenscript_runtime_flagsbuilder.Proposed fix
match trigger.config { - TriggerTypeConfig::Script { arguments, .. } => { + TriggerTypeConfig::Script { arguments, runtime_flags, .. } => { assert_eq!(arguments, Some(vec!["arg1".to_string()])); + assert_eq!(runtime_flags, Some(vec!["-v".to_string()])); } _ => panic!("Expected script config"), }
🤖 Fix all issues with AI agents
In `@examples/config/triggers/script_notifications.json`:
- Around line 6-9: The example is misleading because the runtime flag in the
"runtime_flags" array is a Node/V8 flag but the entry uses "language": "Bash"
with "script_path": "./config/triggers/scripts/custom_notification.sh"; update
the example so the runtime flag matches the language: either change "language"
to "Node" and point "script_path" to a .js file (and keep the V8
"--max-old-space-size=2048" flag), or keep "language": "Bash" and replace the
runtime flag with a Bash-relevant example (e.g., a shell option or environment
variable) so "runtime_flags" correctly reflects the chosen language.
In `@src/services/trigger/script/executor.rs`:
- Around line 56-60: The JSON payload currently serializes runtime_flags
inconsistently (null in PythonScriptExecutor::execute and
BashScriptExecutor::execute, but [] in the JavaScript executor), so update the
combined_input construction in PythonScriptExecutor::execute and
BashScriptExecutor::execute to use runtime_flags.unwrap_or(&[]) (or the
Rust-equivalent used in the JS executor) instead of plain runtime_flags so
runtime_flags is always an array in the JSON sent to scripts; keep the
JavaScript executor unchanged.
In `@tests/properties/triggers/script.rs`:
- Line 138: Update the incorrect inline comment next to the status construction
where ExitStatusExt::from_raw(exit_code as u32) is used: change the range
description from "1 >= exit_code <= 255" to the correct range "0..2 (i.e., 0 or
1)" or "0..=1" and note that there is no underflow because exit_code is
non-negative; update the comment adjacent to the status field (the line
referencing std::os::windows::process::ExitStatusExt::from_raw and exit_code)
accordingly.
- Around line 78-82: The Unix call to ExitStatus::from_raw is using the raw wait
status, so using ExitStatus::from_raw(exit_code) treats small integers as signal
deaths rather than exit codes; update all places that construct
std::process::Output on Unix (where ExitStatus::from_raw is invoked with
exit_code, e.g., in the test creating std::process::Output) to pass the encoded
exit status for an exit code by shifting the code left 8 bits (use exit_code <<
8) before calling ExitStatus::from_raw, so status.code() yields Some(exit_code)
and .success() remains correct; apply the same fix for every Unix from_raw call
with non-zero exit codes in this file (including the occurrence around the
second call noted).
🧹 Nitpick comments (8)
src/utils/metrics/server.rs (1)
339-339: Consider splitting unrelated test stability fixes into a separate PR.The timeout increase from 1 to 2 seconds is reasonable for test stability, but this change appears unrelated to the runtime_flags feature that is the focus of this PR. While not problematic, keeping PRs focused on a single concern improves reviewability and makes it easier to track changes.
tests/properties/strategies.rs (2)
314-321: Consider generating non-Noneruntime_flags in property tests.The strategy always sets
runtime_flags: None, so property tests never exercise the runtime-flags code path. Consider occasionally generatingSome(vec![...])to increase coverage.♻️ Example strategy producing optional runtime flags
+let runtime_flags_strategy = option::of( + proptest::collection::vec( + prop_oneof![ + Just("--max-old-space-size=2048".to_string()), + Just("--cpu-prof".to_string()), + Just("--env-file-if-exists=test.env".to_string()), + ], + 1..3, + ), +);Then thread it through the
prop_mapand assign toruntime_flags.
364-367: Pre-existing:ExitStatusExt::from_raw(1)on Unix represents signal termination, not exit code 1.On Unix, the raw wait status encodes exit codes in bits 8–15.
from_raw(1)means "killed by signal 1 (SIGHUP)", not "exited with code 1". This happens to satisfy!status.success()so tests still pass, but if any test inspects the exit code directly it will be surprising. Not introduced by this PR, but worth knowing.src/services/trigger/script/executor.rs (3)
64-67: Runtime flags are injected as interpreter arguments without validation — document the trust assumption.Flags like
--require,--inspect(Node), or-c(Python/sh) placed inruntime_flagscould alter script behavior or open debug ports. Since config files are operator-authored (same trust level asscript_path), this is acceptable, but consider adding a doc comment on theruntime_flagsfield or the trait method noting that values are passed directly to the interpreter and must be trusted.Also applies to: 106-109, 148-151
502-502: Remove debugprintln!left in test.Line 502:
println!("Result: {:?}", result);appears to be a leftover debug statement.🧹 Proposed fix
- println!("Result: {:?}", result);
446-505: Good coverage for JS runtime flags — consider adding equivalent tests for Python and Bash.There's a dedicated
test_javascript_script_executor_runtime_flags_successtest, but no corresponding tests forPythonScriptExecutororBashScriptExecutorverifying that runtime flags are actually received by the interpreter. For Python, you could checksys.flagsorsys.argv; for Bash, you could verify shell options withset -o.src/repositories/monitor.rs (1)
157-198: Consider validatingruntime_flagsandargumentsduring monitor reference validation.The
validate_monitor_referencesfunction validatesscript_path, file extension, andtimeout_msfor trigger conditions, but does not validateruntime_flagsorarguments. These values are passed directly to child process invocations (python3, node, sh) viaCommand::args(). While Rust's command execution prevents shell injection by design, adding validation for these fields would provide defense-in-depth and prevent potentially malformed or unexpected flag values from reaching the runtime.src/utils/tests/builders/solana/monitor.rs (1)
278-308: Consider adding a test case exercising non-Noneruntime_flags.All four chain-specific builders only test
runtime_flags: None. A single test withSome(vec!["--max-old-space-size=2048".to_string()])and a corresponding assertion would strengthen coverage of the new field.
There was a problem hiding this comment.
Thanks @TEMHITHORPHE, looks good, just a few questions. Can you also have a look at the CodeRabbit comments?
…related configurations
|
Hi @TEMHITHORPHE -- looks like this test is still failing: |
|
|
||
| # Ignore logs dir | ||
| logs/ | ||
| *.cpuprofile |
There was a problem hiding this comment.
hey @TEMHITHORPHE, do you remember why do we need this line?
There was a problem hiding this comment.
I added it because the runtime arguments tests of node produced that file artifact (which is like proof of it working), but I'll have it removed 👍🏾
edit: On second thoughts, i think it's best to leave it, to prevent accidental commits to the repo.
4 participants
Summary
Feat: Add runtime flags support for scripts (triggers & filters)
Previously, script notifications could accept arguments but lacked a way to pass flags directly to the script runtime (e.g., Node, Python, Bash).
With this change, users can now configure runtime flags (like
--max‑old‑space‑size=2048) to customize how scripts are launched.This PR introduces support for runtime flags for script execution in triggers and filters, fixes #356
✅ Key Changes
New Functionality
runtime_flags: Option<Vec<String>>in trigger/filter configs to allow passing runtime flags to scripts.Code & API Updates
TriggerConditionsandTriggerTypeConfigstructs to include the newruntime_flagsfield.runtime_flagsacross builder utilities and script execution flows.Examples Updated
runtime_flags, e.g.:Testing Process
Checklist
Note
If you are using Monitor in your stack, consider adding your team or organization to our list of Monitor Users in the Wild!
Summary by CodeRabbit
Release Notes
New Features
Improvements