fix(engine): use type-appropriate zero values for optional input defaults#123
Conversation
…ults
Optional workflow inputs without an explicit `default:` previously
defaulted to Python None, which renders as "None" in templates and
isn't caught by Jinja's `| default()` filter without the boolean flag.
Now uses type-appropriate zero values: "" for string, 0 for number,
false for boolean, [] for array, {} for object. This ensures templates
render cleanly without requiring `| default()` guards or `if X else Y`
workarounds.
Explicit `default:` values in the schema are still honored.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
=======================================
Coverage ? 84.94%
=======================================
Files ? 53
Lines ? 7424
Branches ? 0
=======================================
Hits ? 6306
Misses ? 1118
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Replace shared [] and {} instances in the class-level _TYPE_ZERO_VALUES
dict with a _zero_value_for_type() method that returns fresh copies for
mutable types (array, object). Prevents potential shared-state bugs if
a caller ever mutates the returned default.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jrob5756
left a comment
There was a problem hiding this comment.
Same comment as before about portability. Other than that, looks good.
| optional numbers as 0 (not 'None'), etc. | ||
| """ | ||
| config = WorkflowConfig( | ||
| workflow=WorkflowDef( |
There was a problem hiding this comment.
Same as a few other PRs, replace pwsh with sys.executable.
Every other script-type test in this repo uses command=sys.executable with args=["-c", "..."] (see tests/test_engine/test_script_workflow.py:56,87). PowerShell isn't installed on most local dev environments — this test won't even run on a stock macOS/Linux machine without an extra install. The single-quote stdout assertions are also pwsh-specific.
Suggested rewrite (also requires import sys at the top of the file):
command=sys.executable,
args=[
"-c",
(
"print(f'msg={{ workflow.input.optional_msg }}"
" count={{ workflow.input.optional_count }}"
" def={{ workflow.input.with_default }}')"
),
],Even better: convert this to a direct unit test of _apply_input_defaults / _zero_value_for_type parametrized over all five types — it would drop the subprocess dependency entirely and exercise the boolean, array, and object branches that the current end-to-end test never touches.
There was a problem hiding this comment.
Both calls (pwsh -> direct unit test, all-five-types coverage) folded into one rewrite in ac928e3. Replaced the e2e test with TestApplyInputDefaults — parametrized over string / number / boolean / array / object directly against _apply_input_defaults, no subprocess, no pwsh. Same class also covers explicit-default-honored, provided-value-passthrough, and required-untouched. Thanks for the nudge to drop the subprocess — much cleaner.
|
|
||
| stdout = engine.context.agent_outputs["echo"]["stdout"].strip() | ||
| assert "msg=" in stdout | ||
| assert "None" not in stdout # Should never render Python's None |
There was a problem hiding this comment.
Coverage gap. This test covers string and number (and the explicit-default path), but not boolean, array, or object. The central claim of the fix is "all five InputDef.type values get a sensible zero," so consider adding assertions for the other three — easiest as a parametrized unit test on _apply_input_defaults directly.
While you're there, a small invariant test on _zero_value_for_type would be worthwhile:
assert engine._zero_value_for_type("array") is not engine._zero_value_for_type("array")
assert engine._zero_value_for_type("object") is not engine._zero_value_for_type("object")This guards against a future "optimization" that caches the mutable instances and reintroduces the classic shared-mutable-default bug.
There was a problem hiding this comment.
Added test_zero_value_for_mutable_type_returns_fresh_instance parametrized over array and object in ac928e3 — exactly the is not invariant you suggested. Locks in the fix from the second commit on this branch so a future cache "optimization" can't quietly reintroduce the shared-mutable-default bug. The boolean/array/object coverage gap is closed by the parametrized _apply_input_defaults test in the same commit.
…apply_input_defaults Addresses review feedback on PR microsoft#123: - Drops subprocess + pwsh dependency (other script tests use sys.executable) - Covers all 5 InputDef.type values (string, number, boolean, array, object), not just string and number - Adds invariant test on _zero_value_for_type that mutable types (array, object) return fresh instances — guards against a future 'optimization' caching the instances and reintroducing the shared-mutable-default bug - Adds explicit-default-honored, provided-value-passthrough, and required-input-untouched cases Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jrob5756
left a comment
There was a problem hiding this comment.
All review feedback addressed — tests are notably stronger than originally suggested. CI green. LGTM 🚀
…, #121-#123, #125, #129, #130, #131, #139, #141-#144, #146) CHANGELOG: add 6 newer PRs (#119, #121, #122, #123, #125, #113, #130, #131, #141, #146) to [Unreleased] alongside the previously documented batch. docs/workflow-syntax.md: - Add metadata + instructions fields to the workflow configuration block. - Add input_mapping and max_depth to Sub-Workflow Steps; correct stale claims that circular references are rejected and that workflow steps cannot be used in for_each groups. - Add 'Sub-workflows in for_each groups' subsection with example. - Add JSON stdout auto-parsing note + example to Script Steps output section. - Add type-appropriate zero values table to Workflow Inputs. - Add 'Workflow Metadata Variables' subsection covering workflow.dir, workflow.file, workflow.name. - Update on_start hook context list to include the new workflow.dir/file vars. docs/cli-reference.md: - Document --metadata/-m, --workspace-instructions, and --instructions flags on conductor run. - Add 'Metadata and Instructions' examples block. - Update conductor validate to describe the new template-reference error/warning checks added in #125. docs/providers/claude.md, docs/providers/comparison.md: - Replace stale 'All models support a 200K token context window' / '200K (all models)' claims with notes that the dashboard now sources context_window_max from each provider's SDK at runtime (#144). README.md: - Refresh the Features list to mention sub-workflow composition, dialog mode, workspace instructions, breadcrumb navigation, and the enhanced validate behavior. - Add --metadata, --workspace-instructions, --instructions to the conductor run options table. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* docs: changelog + doc updates for unreleased PRs (#100, #109-#111, #119, #121-#123, #125, #129, #130, #131, #139, #141-#144, #146) CHANGELOG: add 6 newer PRs (#119, #121, #122, #123, #125, #113, #130, #131, #141, #146) to [Unreleased] alongside the previously documented batch. docs/workflow-syntax.md: - Add metadata + instructions fields to the workflow configuration block. - Add input_mapping and max_depth to Sub-Workflow Steps; correct stale claims that circular references are rejected and that workflow steps cannot be used in for_each groups. - Add 'Sub-workflows in for_each groups' subsection with example. - Add JSON stdout auto-parsing note + example to Script Steps output section. - Add type-appropriate zero values table to Workflow Inputs. - Add 'Workflow Metadata Variables' subsection covering workflow.dir, workflow.file, workflow.name. - Update on_start hook context list to include the new workflow.dir/file vars. docs/cli-reference.md: - Document --metadata/-m, --workspace-instructions, and --instructions flags on conductor run. - Add 'Metadata and Instructions' examples block. - Update conductor validate to describe the new template-reference error/warning checks added in #125. docs/providers/claude.md, docs/providers/comparison.md: - Replace stale 'All models support a 200K token context window' / '200K (all models)' claims with notes that the dashboard now sources context_window_max from each provider's SDK at runtime (#144). README.md: - Refresh the Features list to mention sub-workflow composition, dialog mode, workspace instructions, breadcrumb navigation, and the enhanced validate behavior. - Add --metadata, --workspace-instructions, --instructions to the conductor run options table. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore: bump version to 0.1.11 and changelog #148 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Problem
Optional workflow inputs without an explicit
default:value previously defaulted to PythonNone. This causes two issues:{{ workflow.input.optional_msg }}renders as the literal string"None"instead of empty string| default()doesn't catch None —{{ workflow.input.optional_msg | default("fallback") }}still renders"None"because Jinja'sdefaultfilter only catches undefined, not None (unlessboolean=trueis passed)Reproduction
conductor run test.yaml --input required_id=42Expected: Script outputs
fallback(or empty string)Actual: Script outputs
NoneFix
_apply_input_defaults()now uses type-appropriate zero values instead ofNonefor optional inputs with no declared default:""0false[]{}Explicit
default:values in the schema are still honored. This is a backward-compatible change — templates that checkedif workflow.input.Xwill still work since zero values are falsy.Changes
src/conductor/engine/workflow.py: Add_TYPE_ZERO_VALUESclass constant, update_apply_input_defaults()to use ittests/test_engine/test_workflow.py: Test verifying optional inputs render cleanly (no "None")Testing