feat(php): add ScriptTrigger and CommandsTrigger by HarshXAI · Pull Request #457 · kestra-io/plugin-scripts

HarshXAI · 2026-10-02T11:09:01Z

What

Closes #430.

Adds polling triggers for the PHP plugin:

io.kestra.plugin.scripts.php.ScriptTrigger — runs the inline Script task on each poll and emits when exitCondition matches.
io.kestra.plugin.scripts.php.CommandsTrigger — same for the Commands task.

Both follow the dotnet/perl polling trigger pattern (namespace KV edge mode, ExitConditionRegex, blank exitCondition validation).

Issue #430 also mentions commandPattern, argumentsPattern, and Pebble exitCode != 0; those are not implemented because no sibling trigger (dotnet, perl) has them. Trigger output is timestamp, condition, exitCode, and vars.

Also updates AGENTS.md and plugin-script-php package docs.

How tested

Locally (no Docker socket):

Non-Docker PHP trigger tests (ScriptTriggerTest, CommandsTriggerConditionTest, EdgeStateTest, plus non-Docker ScriptTriggerEvaluateTest cases) — 31/31 passed.
Docker-gated tests in ScriptTriggerEvaluateTest and CommandsTriggerTest — 9 skipped (visible via JUnit assumptions), not run as passes.

Docker-backed ScriptTest, CommandsTest, and RunnerTest were not run locally.

Mirror the dotnet/perl polling trigger pattern for the PHP plugin: KV-backed edge mode, exitCondition validation, and integration tests.

fdelbrayelle

Review: request changes

No blockers, no security findings. Builds and tests were not run in this review.

Must fix

Errors swallowed (ScriptTrigger.java:484-487, CommandsTrigger.java:170-173). catch (Exception e) also wraps the exitCondition render check. A bad config, Docker outage or render error is only logged at WARN, and the trigger never fires. Edge state is not advanced on a failed poll. Fix: render and validate exitCondition before the try, and narrow the catch to execution failures.
Runner options missing (ScriptTrigger.java:530-536, CommandsTrigger.java:217-222). The inner task is built with only containerImage and the script or commands. No taskRunner, env, beforeCommands, inputFiles or namespaceFiles. Fix: expose them, or document the limit in @Schema.
Comment violations. Multi-line blocks in both triggers and in EdgeStateTest, ScriptTriggerEvaluateTest, ScriptTriggerTest and CommandsTriggerTest. Limit is one short line, only when the why is non-obvious.
Test gaps. No tests for blank exitCondition, the swallow path, edge=false via evaluate, or CommandsTrigger.matchesCondition. Docker-backed tests were not run by the author. Tests use explicit types instead of var.

Suggestions

ScriptTrigger and CommandsTrigger are near-copies (same in dotnet and perl). Consider an abstract base, maybe as a repo-wide follow-up.
safeExitCode and safeVars try/catch is unneeded. Call the getters directly.
Integer.parseInt on a huge exit code throws NumberFormatException outside the try. Use Long or catch it.
Regex runs on vars.toString(), which is fragile. Compile once.
Output.vars may hold sensitive data, so note it in the schema. condition is a redundant echo.
@Schema descriptions are inconsistent (single-line strings, no """).

Nits

AGENTS.md: missing blank line before the powershell block.
Unsorted imports in ScriptTrigger.
PR says "23/23 passed". Note the Docker tests are unverified.

Validate exitCondition before execution, narrow swallowed errors, document runner limits, trim comments, and extend trigger unit tests.

HarshXAI · 2026-10-02T13:27:23Z

Pushed 8befe84 addressing the review:

exitCondition is rendered/validated before execution; config errors propagate; only execution failures are swallowed and edge state is not advanced on a failed poll
Documented that taskRunner/env/beforeCommands/inputFiles/namespaceFiles are not exposed on the triggers (same as dotnet/perl)
Trimmed comments; added tests for blank exitCondition, execution-failure swallow path, edge=false via evaluate (when Docker is available), and CommandsTrigger.matchesCondition
AGENTS.md blank line, sorted imports, ExitConditionRegex/Long exit-code handling, vars sensitivity note

Non-Docker PHP trigger tests: 31/31 passed locally. Docker-backed tests were not run on this machine.

fdelbrayelle

Review of the PHP ScriptTrigger and CommandsTrigger at head 8befe84f. No security issue found: regex matching goes through ExitConditionRegex.find (1s deadline, substring fallback) and the exit N pattern is linear. 11 of 21 findings from the previous review are addressed. Remaining items:

Should fix before merge

Broad catch still swallows infrastructure errors (ScriptTrigger.java:157-159, CommandsTrigger.java:155-158). This was previous finding M1b and is only partly fixed. The exitCondition render now happens before the try, but Docker or runner failures are still only logged at WARN, so the trigger never fires and edge state is never reset. Narrow the catch to task execution failures and rethrow infrastructure errors, or at least log that the state was kept.
Vacuous failure test (ScriptTriggerEvaluateTest.java:62, evaluate_executionFailure_doesNotAdvanceEdgeState). It asserts only inside if (execution.isEmpty()), so it cannot fail. The generic-exception swallow path has no test. Assert unconditionally, for example that the call returns empty and the KV key is absent.
Docker-gated tests pass silently (ScriptTriggerEvaluateTest.java:77,99,114,130). They use if (!dockerAvailable()) return;, so they report green without running. CommandsTriggerTest has no guard and fails without Docker. Use Assumptions.assumeTrue(dockerAvailable()) so skips are visible, or a runner CI can execute.
Package doc not updated (plugin-script-php/src/main/resources/doc/io.kestra.plugin.scripts.php.md). Add a ## Triggers section (dotnet has one at line 48) covering exitCondition (exit N, otherwise regex with substring fallback matched against vars), edge mode and its KV state, the 60s default interval, and that a failed run has no vars. Also fix the "Common properties" text that says taskRunner is configurable, since the triggers do not expose it.

Worth considering

Edge state written before the execution exists (ScriptTrigger.java:173-190, CommandsTrigger.java:171-190). If generateExecution throws after shouldEmit wrote the KV state, the next poll is suppressed and the event is lost. Write the state after generateExecution succeeds, or document the trade-off.
Issue #430 is only partly covered. There is no commandPattern, argumentsPattern or exitCode != 0, and the output is only timestamp, condition, exitCode and vars. This matches dotnet and perl, so please state the deviation in the PR description.
Duplicated logic (shouldEmit, edgeStateKey, matchesCondition, extractFailure, Output). CommandsTrigger.java:195 calls the static ScriptTrigger.renderExitCondition, which couples the two classes. An abstract base, or a repo-wide follow-up, would fix this.
Output.condition is a redundant echo (ScriptTrigger.java:276).
Missing cases in CommandsTriggerConditionTest: EXIT 1, null condition and no-match, which ScriptTriggerTest has.

Nits

Use var for locals in main code and in tests (for example CommandsTriggerTest.java:23, ScriptTriggerEvaluateTest.java:42).
Single-line @Schema strings remain: CommandsTrigger.java:102 (commands) and the Output.exitCode descriptions in both triggers.
The comment at EdgeStateTest.java:173 restates the assertion. Remove it or shorten it.

Rethrow infrastructure errors from triggers, harden evaluate tests, document triggers in package doc, and use visible Docker skips.

HarshXAI · 2026-10-05T11:45:35Z

Pushed 464737f for the four should-fix items from the second review:

Removed the broad catch in both triggers so Docker/runner errors propagate; edge state is only updated after a successful poll output.
evaluate_executionFailure_doesNotAdvanceEdgeState now always asserts an empty result and no KV entry (non-matching poll with Docker); added evaluate_infrastructureFailure_propagatesWithoutAdvancingEdgeState when Docker is unavailable.
Docker-gated evaluate/Commands tests use Assumptions.assumeTrue so skips are reported (9 skipped locally without Docker).
Added a ## Triggers section to io.kestra.plugin.scripts.php.md and clarified that triggers do not expose taskRunner.

PR description updated for the #430 scope note vs dotnet/perl.

fdelbrayelle

Kestra Plugin Code Review

Business Requirements — partial

Issue #430 asks for PHP polling triggers. ScriptTrigger and CommandsTrigger are delivered and mirror dotnet/perl. commandPattern, argumentsPattern and exitCode != 0 are not implemented; the PR description now states this deviation.

Previous Review Follow-up — 5 of 9 addressed

Baseline: 8befe84f (review by a kestra-io member). Head: 464737fd. The earlier review had no inline threads, so none were resolved.

✅ Should-fix 1, broad catch: removed in both triggers; only RunnableTaskException is caught (ScriptTrigger.java:209, CommandsTrigger.java:201), infrastructure errors propagate.
✅ Should-fix 2, vacuous failure test: evaluate_executionFailure_doesNotAdvanceEdgeState now asserts unconditionally (ScriptTriggerEvaluateTest.java:78-79); infra path covered at line 83.
✅ Should-fix 3, silent Docker skips: assumeTrue used (ScriptTriggerEvaluateTest.java:61,97,118,135,147).
✅ Should-fix 4, package doc: ## Triggers added and taskRunner wording fixed.
✅ #6, #430 deviation: stated in the PR description.
❌ #5, edge state written before the execution exists: ScriptTrigger.java:155-161, CommandsTrigger.java:153-159.
❌ #7, duplicated logic and static cross-class call: CommandsTrigger.java:186.
❌ #8, redundant Output.condition: ScriptTrigger.java:271.
❌ #9, missing CommandsTriggerConditionTest cases: CommandsTriggerConditionTest.java:24-33.
❌ Nits, var and single-line @Schema: still present (see below).

Kestra Guidelines — 5 findings

🟡 ScriptTrigger.java:161 [Guidelines] Edge state written before execution is generated
🟡 CommandsTrigger.java:186 [Guidelines] Duplicated trigger logic and cross-class static call
🟡 ScriptTrigger.java:271 [Guidelines] Redundant condition output
🟡 ScriptTrigger.java:149 [Guidelines] Explicit local types instead of var
🟢 CommandsTrigger.java:102 [Guidelines] Single-line @Schema descriptions

Test gap: CommandsTriggerConditionTest.java:31 [Tests] is covered as an inline 🟡 comment.

Security (OWASP Top 10:2025 + KPS) — 0 findings

✅ No issues found. Regex matching goes through ExitConditionRegex.find (bounded), the exit N pattern is linear, and Output.vars carries a sensitivity note in the schema.

Performance — 0 findings

✅ No issues found. EXIT_CONDITION_PATTERN is a static final constant; one container run per poll is inherent to the design.

Verdict: APPROVE

fdelbrayelle · 2026-10-05T13:20:07Z

+            return Optional.empty();
+        }
+
+        return Optional.of(TriggerService.generateExecution(this, conditionContext, context, out));


🟡 [Guidelines] Edge state written before execution is generated
Problem: shouldEmit persists the new KV state at line 155, then generateExecution runs here; if it throws, the next poll sees previouslyMatched=true and the event is lost (same in CommandsTrigger.java:153-159).
Fix: Compute emit without writing, call generateExecution, then write the KV state; or document the trade-off in the class description.

fdelbrayelle · 2026-10-05T13:20:07Z

+    @Override
+    public Optional<Execution> evaluate(ConditionContext conditionContext, TriggerContext context) throws Exception {
+        RunContext runContext = conditionContext.getRunContext();
+        boolean renderedEdge = runContext.render(this.edge).as(Boolean.class).orElse(true);


🟡 [Guidelines] Explicit local types instead of var
Problem: The guidelines require var for all locals; main code still uses explicit types (RunContext, boolean, Output, String, KVStore, Script, Matcher at lines 148-155, 169-170, 172, 198, 216, 218), and the same in CommandsTrigger and the tests (CommandsTriggerTest.java:32, ScriptTriggerEvaluateTest.java:46,63,73).
Fix: Replace the explicit local types with var in both triggers and the new tests.

fdelbrayelle · 2026-10-05T13:20:07Z

+            title = "Rendered condition",
+            description = "Rendered value of the exitCondition property for this poll."
+        )
+        private String condition;


🟡 [Guidelines] Redundant condition output
Problem: Output.condition only echoes the exitCondition property the user already wrote, which the Outputs Contract says to avoid unless needed downstream (same in CommandsTrigger.java:263).
Fix: Drop condition from both Output classes and from the example message, or keep it and justify it in the PR.

fdelbrayelle · 2026-10-05T13:20:07Z

+    }
+
+    private String renderExitCondition(RunContext runContext) throws IllegalVariableEvaluationException {
+        return ScriptTrigger.renderExitCondition(runContext, this.exitCondition);


🟡 [Guidelines] Duplicated trigger logic and cross-class static call
Problem: shouldEmit, edgeStateKey, matchesCondition, extractFailure and Output are copied between both triggers, and CommandsTrigger depends on the static ScriptTrigger.renderExitCondition, so changing one class silently changes the other. This follows dotnet/perl, so it is not blocking.
Fix: Extract an AbstractPhpTrigger (or a shared helper) holding the edge, condition and render logic, or open a repo-wide follow-up for all language triggers.

fdelbrayelle · 2026-10-05T13:20:07Z

+
+    @Schema(
+        title = "PHP commands to execute",
+        description = "Commands executed on each poll (same semantics as the PHP Commands task)."


🟢 [Guidelines] Single-line @Schema descriptions
Problem: The commands, interval (line 126) and both Output.exitCode descriptions (CommandsTrigger.java:267, ScriptTrigger.java:275) use single-line strings instead of """ blocks.
Fix: Convert them to text blocks like the other descriptions in the class.

fdelbrayelle · 2026-10-05T13:20:07Z

+        "exit 1, 1, true",
+        "exit 1, 0, false",
+    })
+    void exitCodeCondition(String condition, int exitCode, boolean expected) {


🟡 [Tests] Missing condition cases for CommandsTrigger.matchesCondition
Problem: Only exit 0 and exit 1 are covered; case-insensitive EXIT 1, null condition, null exit code, and a no-match regex are not, while ScriptTriggerTest has them.
Fix: Add those cases to exitCodeCondition and add null-condition and no-match tests.

HarshXAI · 2026-10-09T06:57:18Z

The four should-fix items from the 5 Oct review are in 464737f. CI on that commit is green. Nothing new has been pushed since.

fdelbrayelle

Automated review by the kestra-plugins:code-reviewer agent, run on behalf of @fdelbrayelle. Static read of the diff, the build and tests were not run.

VERDICT: APPROVE
No new commits since your last review. The head is still 464737fd, so I only re-checked the unresolved findings against it.
Blocking: none. All four earlier should-fix items are addressed, and I confirmed the narrowed catch in ScriptTrigger.java:209. I did not re-check the other three. Your last review's own follow-up says they are fixed, and I did not run the code or tests.
Non-blocking notes (all still open):

ScriptTrigger.java:155-161 and CommandsTrigger.java:153-159: shouldEmit writes the edge KV state before generateExecution. If generateExecution throws, the event is lost. Write the state after it succeeds, or document the trade-off.
CommandsTrigger.java:186 and ScriptTrigger.java:271: the two triggers duplicate the edge, condition and render logic, and CommandsTrigger calls the static ScriptTrigger.renderExitCondition. Output.condition is also a redundant echo of exitCondition. This matches the dotnet and perl triggers.
Style and tests: CommandsTriggerConditionTest lacks the EXIT 1, null-condition and no-match cases. Main code and tests still use explicit local types instead of var. Several @Schema descriptions are still single-line strings instead of """ blocks.

fdelbrayelle

Approved. See my earlier review comment for non-blocking suggestions.

…nds-triggers # Conflicts: # AGENTS.md

fdelbrayelle · 2026-10-12T14:37:21Z

Edition: OSS | Docker tag: develop (next: 2.1.0-SNAPSHOT)
Instance: http://76f4v.kestra.docker.localhost:1355 (left running for manual review, docker rm -f kestra-76f4v to stop)
Screenshots: https://claude.ai/artifact/1mUpf3JoKCKU7Ty8kEsyYV

Tested merged state b3a65016 (PR branch + main). Trigger interval set to PT10S (example uses PT60S) to fit the 30s timeout.

Summary

#	Flow	Result
1	php_script_trigger (ScriptTrigger, example flow)	✅ SUCCESS
2	php_commands_trigger (CommandsTrigger, example flow)	✅ SUCCESS
3	Edge mode: no re-fire while condition stays true	✅ 1 execution after ~70s (7 polls)
4	Negative: `exit(0)` with `exitCondition: "exit 1"`	✅ no execution
5	Blank `exitCondition`	⚠️ saves fine, fails at runtime
6	php_regression (Script + Commands tasks)	✅ SUCCESS

Flow 1: php_script_trigger (✅ SUCCESS)

Flow YAML

id: php_script_trigger
namespace: qa.php
triggers:
  - id: script_failure
    type: io.kestra.plugin.scripts.php.ScriptTrigger
    interval: PT10S
    exitCondition: "exit 1"
    edge: true
    containerImage: php
    script: |
      <?php
      exit(1);
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	<1s
Total	SUCCESS	0.07s

Logs synthesis (screenshot): fired on the first poll. Triggered with exitCode=1 (condition=exit 1).

Outputs synthesis: trigger vars exitCode=1, condition=exit 1 available to the flow.

Flow 2: php_commands_trigger (✅ SUCCESS)

Flow YAML

id: php_commands_trigger
namespace: qa.php
triggers:
  - id: commands_failure
    type: io.kestra.plugin.scripts.php.CommandsTrigger
    interval: PT10S
    exitCondition: "exit 1"
    edge: true
    containerImage: php
    commands:
      - php -r 'exit(1);'
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	<1s
Total	SUCCESS	0.13s

Logs synthesis (screenshot): same result as flow 1, Triggered with exitCode=1 (condition=exit 1).

Flow 3 and 4: edge and negative

Same YAML as flow 1 (edge) and a variant with script: <?php exit(0);, exitCondition: "exit 1" (negative). Edge flows stayed at one execution each after ~70s. The negative flow polled and never fired.

Flow 5: php_trigger_blank_condition (⚠️)

triggers:
  - id: blank
    type: io.kestra.plugin.scripts.php.ScriptTrigger
    interval: PT10S
    exitCondition: ""
    containerImage: php
    script: |
      <?php
      exit(1);

Flow save and /flows/validate return no error. At each poll the trigger logs ERROR exitCondition must render to a non-empty value and no execution is created. Expected given the PR description (validation at render time). Only a note: a typo here is not caught on save.

Flow 6: php_regression (✅ SUCCESS)

Flow YAML

id: php_regression
namespace: qa.php
tasks:
  - id: script
    type: io.kestra.plugin.scripts.php.Script
    containerImage: php
    script: |
      <?php
      echo "hello script\n";
  - id: commands
    type: io.kestra.plugin.scripts.php.Commands
    containerImage: php
    commands:
      - php -r 'echo "hello commands\n";'

Gantt (screenshot)

Task	Status	Duration
`script`	SUCCESS	16.04s (image pull)
`commands`	SUCCESS	0.85s
Total	SUCCESS	17.00s

Logs synthesis (screenshot): hello script and hello commands, both exit code 0.

Outputs synthesis: no outputs.

Timeouts: none. Topology: skipped (develop is not a 2.0 tag, no UI or artifact change in this PR).

feat(php): add ScriptTrigger and CommandsTrigger

19d7702

Mirror the dotnet/perl polling trigger pattern for the PHP plugin: KV-backed edge mode, exitCondition validation, and integration tests.

HarshXAI mentioned this pull request Oct 2, 2026

Introduce ScriptTrigger & CommandsTrigger for the PHP plugin #430

Closed

kestrabot Bot added this to Pull Requests Oct 2, 2026

github-project-automation Bot moved this to To review in Pull Requests Oct 2, 2026

fdelbrayelle requested review from a team and fdelbrayelle October 2, 2026 12:42

fdelbrayelle assigned HarshXAI Oct 2, 2026

fdelbrayelle added kind/external Pull requests raised by community contributors area/plugin Plugin-related issue or feature request labels Oct 2, 2026

fdelbrayelle requested changes Oct 2, 2026

View reviewed changes

fdelbrayelle and others added 2 commits October 2, 2026 14:59

Merge branch 'main' into feat/php-script-commands-triggers

3bd8d4a

fix(php): address ScriptTrigger review on PR kestra-io#457

8befe84

Validate exitCondition before execution, narrow swallowed errors, document runner limits, trim comments, and extend trigger unit tests.

fdelbrayelle reviewed Oct 2, 2026

View reviewed changes

fdelbrayelle and others added 2 commits October 4, 2026 17:28

Merge branch 'main' into feat/php-script-commands-triggers

d0af1ae

fix(php): address second review on PR kestra-io#457

464737f

Rethrow infrastructure errors from triggers, harden evaluate tests, document triggers in package doc, and use visible Docker skips.

fdelbrayelle reviewed Oct 5, 2026

View reviewed changes

fdelbrayelle reviewed Oct 9, 2026

View reviewed changes

fdelbrayelle approved these changes Oct 9, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into feat/php-script-comma…

b3a6501

…nds-triggers # Conflicts: # AGENTS.md

Merge branch 'main' into feat/php-script-commands-triggers

8cbc917

fdelbrayelle merged commit d614d5a into kestra-io:main Oct 12, 2026
1 check passed

Conversation

HarshXAI commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How tested

Uh oh!

fdelbrayelle left a comment

Choose a reason for hiding this comment

Review: request changes

Must fix

Suggestions

Nits

Uh oh!

HarshXAI commented Oct 2, 2026

Uh oh!

fdelbrayelle left a comment

Choose a reason for hiding this comment

Should fix before merge

Worth considering

Nits

Uh oh!

HarshXAI commented Oct 5, 2026

Uh oh!

fdelbrayelle left a comment

Choose a reason for hiding this comment

Kestra Plugin Code Review

Business Requirements — partial

Previous Review Follow-up — 5 of 9 addressed

Kestra Guidelines — 5 findings

Security (OWASP Top 10:2025 + KPS) — 0 findings

Performance — 0 findings

Uh oh!

fdelbrayelle Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

fdelbrayelle Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

fdelbrayelle Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

fdelbrayelle Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

fdelbrayelle Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

fdelbrayelle Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

HarshXAI commented Oct 9, 2026

Uh oh!

fdelbrayelle left a comment

Choose a reason for hiding this comment

Uh oh!

fdelbrayelle left a comment

Choose a reason for hiding this comment

Uh oh!

fdelbrayelle commented Oct 12, 2026

Summary

Flow 1: php_script_trigger (✅ SUCCESS)

Flow 2: php_commands_trigger (✅ SUCCESS)

Flow 3 and 4: edge and negative

Flow 5: php_trigger_blank_condition (⚠️)

Flow 6: php_regression (✅ SUCCESS)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

HarshXAI commented Oct 2, 2026 •

edited

Loading