From 00137a943c97ec1c194040cbde0a40a337d73224 Mon Sep 17 00:00:00 2001 From: ausias-armesto Date: Sun, 6 Sep 2026 12:48:54 +0200 Subject: [PATCH] fix: skip alias rendering when a step's if condition is false (#6744) --- keep/step/step.py | 25 ++++++++++++++++++------- tests/test_steps.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/keep/step/step.py b/keep/step/step.py index 20b508d722..8ea9c367bb 100644 --- a/keep/step/step.py +++ b/keep/step/step.py @@ -163,14 +163,13 @@ def _run_single(self, dont_render=False): # Initialize all conditions conditions = [] - aliases = self.config.get("alias", {}) - - # if aliases are defined, set them in the context - for alias_key, alias_val in aliases.items(): - aliases[alias_key] = self.io_handler.render(alias_val) - + # set the step's vars in the context so they're available for condition/if evaluation + # note: aliases are intentionally NOT rendered/set here - they are only computed + # once we know the step is actually going to run (see below), since rendering them + # can fail/throw if the step (and its "if") is meant to be skipped (e.g. because they + # reference results of another step that was itself skipped) self.context_manager.set_step_vars( - self.step_id, _vars=self.vars, _aliases=aliases + self.step_id, _vars=self.vars, _aliases={} ) for condition in self.conditions: @@ -301,6 +300,18 @@ def _run_single(self, dont_render=False): }, ) + # Now that we know the step is actually going to run, render and set its aliases. + # This must happen after the "if" check above (and not before, and not + # unconditionally) since aliases are only meaningful/safe to compute when the + # step runs - e.g. they may reference results of another step that was skipped, + # in which case rendering them here (before the "if" gate) could throw. + aliases = self.config.get("alias", {}) + for alias_key, alias_val in aliases.items(): + aliases[alias_key] = self.io_handler.render(alias_val) + self.context_manager.set_step_vars( + self.step_id, _vars=self.vars, _aliases=aliases + ) + # Third, check throttling # Now check if throttling is enabled self.logger.info( diff --git a/tests/test_steps.py b/tests/test_steps.py index 1975a7b138..dcdf4480b3 100644 --- a/tests/test_steps.py +++ b/tests/test_steps.py @@ -88,6 +88,37 @@ def test_run_single_and_trigger_keep_function(sample_step, mocked_context_manage assert mock_len.call_count == 1 +def test_run_single_if_false_skips_alias_evaluation(sample_step, mocked_context_manager): + # Regression test: when a step's "if" condition evaluates to false, its "alias" + # block must NOT be evaluated at all - just like the provider call is skipped. + # + # Here the alias expression references another step's result + # ("steps.get-incident.results.body") that is missing from the context (e.g. + # because that other step was itself skipped), which renders as an empty string + # and turns "keep.dictget({{ ... }}, ...)" into the syntactically invalid + # "keep.dictget(, ...)". If the alias were evaluated despite "if" being false, + # this would raise a SyntaxError instead of the step being cleanly skipped. + sample_step.config["if"] = "'{{ steps.get-incident.results.body }}' != ''" + sample_step.config["alias"] = { + "incident_name": ( + "keep.dictget({{ steps.get-incident.results.body }}, " + "'user_generated_name', 'Untitled incident')" + ) + } + + sample_step.io_handler.context_manager = mocked_context_manager + sample_step.provider.query = Mock(return_value="result") + sample_step.provider.notify = Mock(return_value="result") + + # Should not raise (the alias must never be rendered), and the step should be + # reported as not having run. + result = sample_step._run_single() + + assert result is None + sample_step.provider.query.assert_not_called() + sample_step.provider.notify.assert_not_called() + + def test_run_single_exception(sample_step): # Simulate an exception sample_step.provider.query = Mock(side_effect=Exception("Test exception"))