fix!: drop values when skipped arguments are being substituted#16223
Open
isubasinghe wants to merge 1 commit into
Open
fix!: drop values when skipped arguments are being substituted#16223isubasinghe wants to merge 1 commit into
isubasinghe wants to merge 1 commit into
Conversation
Joibel
added
cherry-pick/3.7
Joibel
reviewed
Jun 5, 2026
Joibel
left a comment
Joibel
left a comment
Member
There was a problem hiding this comment.
docs/variables.md needs updating too for this behaviour (steps L#157 and dag L#176)
docs/enahanced-depends-logic.md should should mention new behaviour too
This is breaking so should get ! in PR title and upgrading.md note
| // input default for that parameter. Removing the argument lets that input default apply instead of the | ||
| // empty skipped placeholder. Arguments whose input has no default are left intact (empty value) so the | ||
| // reference still resolves and the node stays live (preserving the #15932 requeue-loop fix). | ||
| func (woc *wfOperationCtx) dropSkippedDefaultedArgs(ctx context.Context, tmplCtx *templateresolution.TemplateContext, holder wfv1.TemplateReferenceHolder, origArgs wfv1.Arguments, resolved *wfv1.Arguments, scope *wfScope) { |
Member
There was a problem hiding this comment.
This really feels like it should return err and let callers deal with those failures we're swallowing here otherwise those failures will result in the old bad behaviour
| // addSkippedOutputParamToScope registers an output parameter of a skipped/omitted node. If the | ||
| // producing template declared a valueFrom.default for the parameter, that default is used as the | ||
| // scope value so every consumer (task inputs and template-output aggregation alike) sees it. | ||
| // Otherwise it falls back to addSkippedParamToScope's empty-value-plus-marker behavior. |
Member
There was a problem hiding this comment.
empty-value-plus-marker is no longer what happens here (just comment is wrong)
Joibel
reviewed
Jun 5, 2026
isubasinghe
changed the title
References to output parameters of steps and tasks that were Skipped
(when was false) or Omitted (depends unsatisfied) now resolve instead
of requeuing forever or erroring.
A producer's valueFrom.default applies to every reference. Otherwise
the output is absent: simple tags substitute an empty string, an
argument that is purely such a reference is dropped so the consuming
template's input default applies, and expression tags see nil so
expr-lang `??` fallbacks work. An expression that does not handle the
nil fails the node with a terminal error rather than leaving the
workflow stuck. A legitimately empty output ("") is not absent.
These semantics apply uniformly to step and task arguments, when
clauses, lifecycle hook and exit handler arguments and expressions,
nested template tags, and template output aggregation (where a
valueFrom.default on the aggregating parameter applies when the
reference is absent or the expression fails).
BREAKING CHANGE: expressions referencing a skipped/omitted output that
declared no valueFrom.default see nil instead of "": comparisons such
as `== ''` no longer match, and an expression that does not handle the
nil (e.g. a bare `{{= tasks.x.outputs.parameters.y}}` without `??`)
fails the node instead of resolving.
Documented in docs/variables.md (Outputs of Skipped and Omitted Nodes)
and docs/upgrading.md, with runnable examples under
examples/skipped-output-defaults/.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: isubasinghe <isitha@pipekit.io>
isubasinghe
force-pushed
the
expression-default-handling
branch
from
926def7 to
c89954f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Skipped/omitted node outputs resolved to
"", so: producervalueFrom.defaultwas ignored, consumer input defaults were clobbered, and??never fired (value waspresent-empty, not absent).
Modifications
nilin scope (absent optional); simple{{...}}refs still flatten to""(preserves fix: populate scope with empty values for outputs of skipped/omitted … #15932 no-requeue behavior).valueFrom.defaultapplies; else consumer input default applies (dropSkippedDefaultedArgs); else"".ReplaceStrictAnypreserves nil into expression envs, so??works in inline{{=...}}args andwhenclauses, not justvalueFrom.expression/fromExpression.""instead of staying unresolved.missingVarsInEnv; removed lexer-tokenhasVariableInExpression/searchTokens/filterEOF.skippedRef + '-suffix'now see nil (error) instead of""; use??.Verification
New unit tests (
TestReplaceStrictAnyNilValues,Test_missingVarsInEnv) and controller tests (TestDAGSkippedOutput*,TestDAGSkippedInlineExpressionFallback,TestStepsSkippedOutputDefault,examples_skipped_defaults_test.go). Fullworkflow/controller,workflow/validate,workflow/common,util/templatesuites pass.Documentation
New examples in
examples/skipped-output-defaults/.AI
Claude Code was used for code, tests, and this description, under human review.