feat/r-script-commands-triggers - #438
Conversation
fdelbrayelle
left a comment
There was a problem hiding this comment.
Kestra Plugin Code Review
Business Requirements — met
No linked issue is referenced in the PR body. The stated goal (bring plugin-script-r trigger support to parity with the Shell/Node modules by adding ScriptTrigger/CommandsTrigger) is self-consistent and fully implemented, with tests and QA examples matching the description.
Kestra Guidelines — 4 findings
- 🟠 TODO placeholders left in production code (both new files) — forbidden by the Code Comments guideline.
- 🟠
lastMatched(AtomicBoolean) not excluded from Lombok@ToString/@EqualsAndHashCode(both files) — correctness bug, see inline comment. - 🟠 Plugin how-to doc
plugin-script-r/src/main/resources/doc/io.kestra.plugin.scripts.r.mdnot updated to mention the two new triggers — mandatory per the 'Plugin How-To Doc' section whenever a trigger is added to an existing plugin. Node's equivalent doc has a one-line mention of itsCommandsTrigger/ScriptTrigger; the R doc still only listsScript/Commands. - 🟡 Required
exitConditionis rendered with.orElse("")(both files) instead of failing loudly on an unrenderable value — see inline comment.
Security (OWASP Top 10:2025 + KPS) — 0 blocking issues
No secret fields, no new HTTP calls, no unsafe deserialization, no shell string concatenation beyond what the existing Commands/Script tasks already do (out of scope of this diff). The regex-timeout resource-leak noted under Performance has a DoS angle (A10-adjacent) but is tracked there since its primary consequence is thread-pool exhaustion, not an authorization/injection gap.
Performance — 1 finding
- 🟠 ReDoS guard in
matchesConditionabandons the backtracking regex thread on the sharedForkJoinPool.commonPool()instead of cancelling it — see inline comments on both files.
Additional non-blocking notes
- 🟡 Edge-mode tests (
edgeMode_preventsConsecutiveEmitinCommandsTriggerTest, and the threeedgeMode_*tests inScriptTriggerTest) re-implement!lastMatched.getAndSet(x) && xinline rather than callingtrigger.evaluate()twice on the same instance. They verifyAtomicBooleanarithmetic, not the trigger's actual edge-mode wiring — a regression inevaluate()'s edge logic wouldn't be caught by these tests. Given edge mode is this PR's headline feature, at least one@KestraTestshould callevaluate()twice on the same trigger instance and assert the second call returnsOptional.empty(). - 🟡 DRY:
CommandsTrigger/ScriptTriggerduplicate ~90% of their logic (evaluate,matchesCondition,extractFailure,Output). This mirrors pre-existing duplication already merged for Shell/Node/Ruby, so it's not net-new debt introduced by this PR, and the TODO (flagged above for violating the no-TODO-comment rule) at least names the follow-up. Non-blocking, but worth tracking as a real issue rather than a comment. - 🟢 Nit: trigger/task ids in tests are fixed strings (
"commands-trigger", …) rather than randomized, per the Flaky Test Prevention guideline — mirrors existing Node/Shell test precedent, so not introduced by this PR.
Verdict: REQUEST CHANGES
The R ScriptTrigger/CommandsTrigger in this branch are byte-for-byte identical to PR kestra-io#438, which is dedicated to the R module. Remove them here so kestra-io#446 is scoped to the Perl triggers and kestra-io#438 owns the R work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat/r-script-commands-triggers * feat/perl-script-commands-triggers * chore(r): drop R triggers, keep them in #438 The R ScriptTrigger/CommandsTrigger in this branch are byte-for-byte identical to PR #438, which is dedicated to the R module. Remove them here so #446 is scoped to the Perl triggers and #438 owns the R work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(perl): guard trigger exitCondition regex against ReDoS Run the user-supplied exitCondition regex with a 5s timeout and fall back to substring matching, as the Ruby/Shell/Node/Bun triggers do, so a catastrophic-backtracking pattern can no longer hang the scheduler poll thread. Document the in-memory edge-state limitation and add condition tests for pathological and invalid regexes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(perl): make ReDoS test hit the timeout and fix CommandsTrigger example (a+)+$ is memoized by the JDK 25 regex engine and fails instantly, so the test never reached the 5s guard; use (.*a){20}$ and assert the elapsed time. The CommandsTrigger example ran `perl missing.pl`, which exits 2 and never matched `exit 1`; use `perl -e 'exit 1'` instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(perl): persist trigger edge state in the namespace KV store The in-memory lastMatched flag was rebuilt with the trigger on every poll, so edge mode fired on every matching poll. Keep the previous result in the namespace KV store instead, as the Bun/.NET/PowerShell triggers do. Replaces the tautological AtomicBoolean edge tests with EdgeStateTest and an evaluate-level test that polls through a serialized copy. Refs #449 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: jymaire <jmaire@kestra.io> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: jymaire <jymaire@users.noreply.github.com>
|
Hello @Abhishek84313 👋 Any news on the code review feedback? Thanks! |
Aligns the R triggers with the pattern merged for Bun in kestra-io#433. - Persist edge state in the namespace KV store instead of an in-memory AtomicBoolean. This drops the field the review flagged for breaking Lombok equals/hashCode (AtomicBoolean has identity equality), and also fixes kestra-io#449: a rebuilt or rehydrated trigger no longer re-fires. - Replace the CompletableFuture regex timeout with a bounded LRU cache of compiled patterns. The abandoned future leaked a ForkJoinPool.commonPool() worker on timeout, since Matcher#find() is not interruptible. - Fail loudly when the required exitCondition renders empty, instead of silently coercing it to "" and never matching. - Remove the TODO comments from both trigger classes. - Document both triggers in the plugin how-to doc. - Replace the edge-mode tests that asserted AtomicBoolean arithmetic with tests that drive shouldEmit and evaluate() for real, and randomize trigger ids now that edge state persists between runs. - Revert the scheduler/worker test dependencies, which are not needed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for the thorough review @fdelbrayelle, and sorry for the delay. All findings are addressed in 8fe149d. While rebasing I noticed #433 (Bun triggers) had landed on main with a design that already solves most of what you flagged, so rather than patching each comment in isolation I aligned the R triggers to that merged precedent. The two modules now match. |
Follow-up Review — Verification of Prior Feedback (commit 8fe149d)Re-checked every item raised in review #5266634945 against the current head ( Per-item status
Threads resolved (8/8)All 8 previously unresolved inline review threads were verified against current code and resolved via GraphQL:
Item #11 (DRY) has no thread — it was a review-body-only note, left open as tracked-but-non-blocking per the author's comment. New findings from this pass
Verdict: Ready to mergeAll 8 blocking/request-changes items from the prior review are fixed and verified against current code, tests were strengthened beyond what was asked, and the lone remaining note (DRY duplication) is an accepted, tracked, non-blocking design choice consistent with merged precedent (#433). The one new observation (ReDoS timeout removal) is a non-blocking suggestion for a future follow-up, not a regression severe enough to hold this PR. Note: PR |
🧪 QA report: R
|
| # | Flow | Type | Scenario | Result |
|---|---|---|---|---|
| 1 | r_commands_trigger |
NEW, source @Example |
CommandsTrigger, exit 1, default edge |
✅ fired once, SUCCESS |
| 2 | r_script_trigger |
NEW, source @Example |
ScriptTrigger, exit 1, edge: true |
✅ fired once over ~5 min of 10s polls |
| 3 | r_script_trigger_vars |
NEW | ScriptTrigger, regex status=\w+ on vars |
✅ fired once, vars exposed |
| 4 | r_script_no_edge |
NEW | ScriptTrigger, exit 1, edge: false, PT5S |
✅ fired every poll (47 executions in ~4.5 min) |
| 5 | r_commands_no_match |
NEW (negative) | CommandsTrigger, cmd exits 0 vs exit 1 |
✅ 0 executions |
| 6 | r_script_task |
Non-regression | R Script + Commands tasks |
✅ SUCCESS |
First trigger executions appeared 7–12s after flow creation (well inside the 30s budget). No timeouts.
Flow 1: r_commands_trigger (✅ SUCCESS)
Flow YAML
id: r_commands_trigger
namespace: company.team
triggers:
- id: on_fail
type: io.kestra.plugin.scripts.r.CommandsTrigger
interval: PT5S
exitCondition: "exit 1"
commands:
- Rscript -e 'stop("boom")'
tasks:
- id: log
type: io.kestra.plugin.core.log.Log
message: "Triggered with exitCode={{ trigger.exitCode }} (condition={{ trigger.condition }})"Gantt (screenshot)
| Task | Status | Duration |
|---|---|---|
log |
SUCCESS | 0.04s |
| Total | SUCCESS | 0.07s |
Logs synthesis: Triggered with exitCode=1 (condition=exit 1). Only 1 execution over the whole run: edge defaults to true as documented.
Outputs synthesis (screenshot): trigger vars timestamp, condition: "exit 1", exitCode: 1. No vars key (script emitted none).
Flow 2: r_script_trigger (✅ SUCCESS)
Flow YAML
id: r_script_trigger
namespace: company.team
triggers:
- id: script_failure
type: io.kestra.plugin.scripts.r.ScriptTrigger
interval: PT10S
exitCondition: "exit 1"
edge: true
script: |
stop("boom")
tasks:
- id: log
type: io.kestra.plugin.core.log.Log
message: "Triggered with exitCode={{ trigger.exitCode }} (condition={{ trigger.condition }})"Gantt (screenshot)
| Task | Status | Duration |
|---|---|---|
log |
SUCCESS | 0.03s |
| Total | SUCCESS | 0.08s |
Logs synthesis: Triggered with exitCode=1 (condition=exit 1). Condition stayed true on every later poll, but no second execution: edge state (now in KV store) works.
Outputs synthesis (screenshot): condition: "exit 1", exitCode: 1, timestamp.
Flow 3: r_script_trigger_vars (✅ SUCCESS)
Flow YAML
id: r_script_trigger_vars
namespace: company.team
triggers:
- id: on_ready
type: io.kestra.plugin.scripts.r.ScriptTrigger
interval: PT10S
exitCondition: "status=\\w+"
edge: true
script: |
cat('::{"outputs":{"status":"status=ready"}}::\n')
tasks:
- id: log
type: io.kestra.plugin.core.log.Log
message: "vars={{ trigger.vars }} exitCode={{ trigger.exitCode }}"Gantt (screenshot)
| Task | Status | Duration |
|---|---|---|
log |
SUCCESS | 0.03s |
| Total | SUCCESS | 0.06s |
Logs synthesis: vars={"status":"status=ready"} exitCode=0. Regex matched against emitted vars.
Outputs synthesis (screenshot): condition: "status=\w+", exitCode: 0, vars: {status: "status=ready"}.
Flow 4: r_script_no_edge (✅ SUCCESS)
Flow YAML
id: r_script_no_edge
namespace: company.team
triggers:
- id: every_fail
type: io.kestra.plugin.scripts.r.ScriptTrigger
interval: PT5S
exitCondition: "exit 1"
edge: false
script: |
stop("boom")
tasks:
- id: log
type: io.kestra.plugin.core.log.Log
message: "Triggered at {{ trigger.timestamp }} exitCode={{ trigger.exitCode }}"Gantt (screenshot), first of 47 executions
| Task | Status | Duration |
|---|---|---|
log |
SUCCESS | 0.03s |
| Total | SUCCESS | 0.07s |
Logs synthesis: Triggered at <timestamp> exitCode=1, one execution roughly every 5–7s. All 47 SUCCESS.
Outputs synthesis (screenshot): condition: "exit 1", exitCode: 1, distinct timestamp per execution.
Flow 5: r_commands_no_match (✅ no execution, as expected)
Flow YAML
id: r_commands_no_match
namespace: company.team
triggers:
- id: never
type: io.kestra.plugin.scripts.r.CommandsTrigger
interval: PT5S
exitCondition: "exit 1"
commands:
- Rscript -e 'quit(status = 0)'
tasks:
- id: log
type: io.kestra.plugin.core.log.Log
message: "Should never run"Result: 0 executions over ~5 min (executions list). No WARN in server logs.
Flow 6: r_script_task, non-regression (✅ SUCCESS)
Flow YAML
id: r_script_task
namespace: company.team
tasks:
- id: script
type: io.kestra.plugin.scripts.r.Script
script: |
x <- sum(1:10)
cat(sprintf('::{"outputs":{"sum":%d}}::\n', x))
- id: commands
type: io.kestra.plugin.scripts.r.Commands
commands:
- Rscript -e 'cat(R.version.string, "\n")'Gantt (screenshot)
| Task | Status | Duration |
|---|---|---|
script |
SUCCESS | 1.63s |
commands |
SUCCESS | 0.86s |
| Total | SUCCESS | 2.61s |
Logs synthesis: both containers created, ran with exit code 0, and were cleaned up. commands logged R version 4.6.1 (2026-06-24).
Outputs synthesis (screenshot): script returned vars: {sum: 55}, exitCode: 0, outputFiles: {}. commands returned exitCode: 0.
Notes
- No topology checks: PR has no UI/artifact changes.
- Not covered: edge behaviour across a scheduler restart (KV persistence). Unit test
EdgeStateTestcovers it. - Timeouts: none.
feat(r): add ScriptTrigger and CommandsTrigger to plugin-script-r
What changes are being made and why?
The R module only shipped
ScriptandCommandstasks, so an R script could be run by a flow but could not start one. Teams that watch an external system with R (a CRAN job, a data-quality check, a health probe) had to wrap it in aScheduleplus a conditional task, or poll from outside Kestra.This PR adds two polling triggers to
plugin-script-r, bringing it in line with the Shell and Node modules:io.kestra.plugin.scripts.r.ScriptTrigger— polls by running an inline R script in a container and starts the flow when the result matches a condition.io.kestra.plugin.scripts.r.CommandsTrigger— same, driven by a list of R commands instead of an inline script.Behaviour shared by both triggers:
containerImageProperty<String>r-basescript/commandsProperty<String>/Property<List<String>>exitConditionProperty<String>intervalDurationPT60SedgeProperty<Boolean>truenot matching → matchingtransition.Condition matching (
matchesCondition) supports two forms:exit N— case-insensitive, compares against the process exit code. Anullexit code never matches.vars; if the pattern is invalid or takes longer than 5s to evaluate, it falls back to a plain substring check. The 5s cap exists so a user-supplied pattern cannot hang the scheduler thread through catastrophic backtracking.Edge mode (
edge: true, the default) is anti-spam: a condition that stays true across polls emits once, not on every tick. Setedge: falseto emit on every matching poll.Failure handling: a
RunnableTaskExceptionis unwrapped down the cause chain to find aTaskExceptionand recover its exit code, so a failing script still produces a usableexitCodeoutput instead of propagating. Any other exception during evaluation is logged atWARNand returnsOptional.empty()— a broken poll never blocks the scheduler.Outputs exposed to the flow (
{{ trigger.* }}):timestamp,condition(the renderedexitCondition),exitCode, andvars.Known limitation: the
edgestate (lastMatched) is an in-memoryAtomicBoolean. It resets when the trigger is rehydrated, so edge mode may re-fire once after a scheduler restart. Both classes carry aTODOto extract the duplicatedevaluate/matchesCondition/extractFailure/Outputlogic into a sharedAbstractScriptTriggerinplugin-script— the duplication across Shell, Node, Ruby and R is deliberate for now and should be consolidated in a follow-up rather than inside this PR.Files changed
plugin-script-r/src/main/java/io/kestra/plugin/scripts/r/ScriptTrigger.javaplugin-script-r/src/main/java/io/kestra/plugin/scripts/r/CommandsTrigger.javaplugin-script-r/src/test/java/io/kestra/plugin/scripts/r/ScriptTriggerTest.javaplugin-script-r/src/test/java/io/kestra/plugin/scripts/r/ScriptTriggerConditionTest.javaplugin-script-r/src/test/java/io/kestra/plugin/scripts/r/CommandsTriggerTest.java@KestraTestintegration tests.plugin-script-r/src/test/java/io/kestra/plugin/scripts/r/CommandsTriggerConditionTest.javaplugin-script-r/build.gradleschedulerandworkertest dependencies needed by@KestraTest.AGENTS.mdplugin-script-r.How the changes have been QAed?
CommandsTrigger— fires when a command exits non-zero:ScriptTrigger— inline R script, edge mode on:ScriptTrigger— match on structured outputs rather than exit code:Automated coverage
CommandsTriggerTestis a@KestraTestthat runs the trigger end to end against a realr-basecontainer and asserts on the generated execution:1againstexitCondition: "exit 1"emits, withexitCode == 1,condition == "exit 1"and a non-nulltimestamp;::{"outputs":{"listing":"toto"}}::againstexitCondition: "toto"emits, withexitCode == 0and non-nullvars;0againstexitCondition: "exit 1"does not emit.ScriptTriggerTest,ScriptTriggerConditionTestandCommandsTriggerConditionTestcovermatchesConditionand edge-mode transitions as pure unit tests, driving theOutputmodel directly. They deliberately avoid needing an R runtime, since it is not present on every CI machine — the runtime-backed coverage lives inCommandsTriggerTest.Condition cases asserted:
exit 0/exit 1/exit 42matching and mismatching, uppercaseEXIT 1(case-insensitivity), null exit code, substring match in vars, absent substring, regex match in vars, empty haystack, empty condition, and null condition.Run them with:
Setup Instructions
r-base.r-baseimage is pulled from Docker Hub on first use; no credentials needed.containerImageat an image that already includes them or install them inside the script/commands../gradlew :plugin-script-r:testneeds Docker running forCommandsTriggerTest; the three unit test classes run without it.Contributor Checklist ✅
closes #432