From 2a4c4dfcb9f011e3e13c529147f6a38095f5f8b2 Mon Sep 17 00:00:00 2001 From: spinje Date: Thu, 8 Jan 2026 12:00:00 +0000 Subject: [PATCH 1/2] fix: resolve optional inputs without defaults to None Optional inputs declared with `required: false` but no `default` value now resolve to `None` instead of failing with "Unresolved variables". Changes: - workflow_validator.py: Add None to context for optional inputs without defaults - node_wrapper.py: Filter error messages to only show actually unresolved variables This allows templates like ${optional_param} to work when the optional input is not provided, resolving to null in the output. Fixes #47 --- src/pflow/runtime/node_wrapper.py | 7 +- src/pflow/runtime/workflow_validator.py | 6 +- .../test_node_wrapper_template_validation.py | 74 ++++++++++++++++ tests/test_runtime/test_null_defaults.py | 86 +++++++++++++++++-- 4 files changed, 164 insertions(+), 9 deletions(-) diff --git a/src/pflow/runtime/node_wrapper.py b/src/pflow/runtime/node_wrapper.py index fdcdfbeda..6f07b191b 100644 --- a/src/pflow/runtime/node_wrapper.py +++ b/src/pflow/runtime/node_wrapper.py @@ -744,7 +744,11 @@ def _build_enhanced_template_error(self, param_key: str, template: str, context: Formatted error message with context and suggestions """ # Extract variable names from template - variables = TemplateResolver.extract_variables(str(template)) + all_variables = TemplateResolver.extract_variables(str(template)) + + # Filter to only actually unresolved variables (not in context) + # This prevents misleading errors like "${provided}, ${missing}" when only ${missing} failed + variables = {v for v in all_variables if not TemplateResolver.variable_exists(v, context)} # Build available keys section available_keys = [k for k in context if not k.startswith("__")] @@ -758,6 +762,7 @@ def _build_enhanced_template_error(self, param_key: str, template: str, context: available_display = available_keys # Simplified single-line error message (removes redundancy) + # Only report actually unresolved variables error_parts = [f"Unresolved variables in parameter '{param_key}': {', '.join(f'${{{v}}}' for v in variables)}"] # Add available keys section (only if there are keys to show) diff --git a/src/pflow/runtime/workflow_validator.py b/src/pflow/runtime/workflow_validator.py index e7a3c2f1f..39b5ed6a7 100644 --- a/src/pflow/runtime/workflow_validator.py +++ b/src/pflow/runtime/workflow_validator.py @@ -173,11 +173,13 @@ def prepare_inputs( ) defaults[input_name] = default_value else: - # Optional with no default key means it can be omitted entirely + # Optional with no default key - use None so templates can resolve + # This allows ${optional_param} to resolve to None instead of failing logger.debug( - f"Optional input '{input_name}' not provided and has no default", + f"Optional input '{input_name}' not provided, using None as default", extra={"phase": "input_validation", "input": input_name}, ) + defaults[input_name] = None else: # Input is provided logger.debug(f"Input '{input_name}' provided", extra={"phase": "input_validation", "input": input_name}) diff --git a/tests/test_runtime/test_node_wrapper_template_validation.py b/tests/test_runtime/test_node_wrapper_template_validation.py index 5b0454dca..66da603eb 100644 --- a/tests/test_runtime/test_node_wrapper_template_validation.py +++ b/tests/test_runtime/test_node_wrapper_template_validation.py @@ -517,3 +517,77 @@ def test_no_template_params_executes_immediately(self): assert result == "default" assert node.params_at_execution == {"message": "static text", "count": 42} + + +class TestErrorMessageAccuracy: + """Test that error messages only report actually unresolved variables. + + Bug fix: Previously, when a parameter had multiple template variables + and some resolved but others didn't, the error message would list ALL + variables as unresolved - even ones that successfully resolved. + """ + + def test_error_only_shows_actually_unresolved_variables(self): + """Error message should only list variables that are actually missing. + + Regression test for misleading error messages: + - Parameter: {"a": "${provided}", "b": "${missing}"} + - Old error: "Unresolved variables: ${provided}, ${missing}" (wrong!) + - Fixed error: "Unresolved variables: ${missing}" (correct) + """ + node = DummyNode() + wrapper = TemplateAwareNodeWrapper(node, "test-node", initial_params={}) + + # Parameter with two variables - one exists, one doesn't + wrapper.set_params({ + "data": { + "has_value": "${provided}", + "no_value": "${missing}", + } + }) + + with pytest.raises(ValueError) as exc_info: + wrapper._run(shared={"provided": "hello"}) + + error_msg = str(exc_info.value) + + # Error should mention the missing variable + assert "${missing}" in error_msg + + # Error should NOT mention the resolved variable + assert "${provided}" not in error_msg + + def test_error_shows_all_missing_when_multiple_missing(self): + """When multiple variables are missing, error should list all of them.""" + node = DummyNode() + wrapper = TemplateAwareNodeWrapper(node, "test-node", initial_params={}) + + wrapper.set_params({"message": "Hello ${name}, you have ${count} items with status ${status}"}) + + with pytest.raises(ValueError) as exc_info: + # Only provide name, missing count and status + wrapper._run(shared={"name": "Alice"}) + + error_msg = str(exc_info.value) + + # Should list both missing variables + assert "${count}" in error_msg + assert "${status}" in error_msg + + # Should NOT list the resolved variable + assert "${name}" not in error_msg + + def test_error_available_keys_shows_provided_context(self): + """Error message should show what keys ARE available for debugging.""" + node = DummyNode() + wrapper = TemplateAwareNodeWrapper(node, "test-node", initial_params={}) + + wrapper.set_params({"field": "${missing}"}) + + with pytest.raises(ValueError) as exc_info: + wrapper._run(shared={"available_key": "value", "another_key": 42}) + + error_msg = str(exc_info.value) + + # Should show available keys to help user debug + assert "available_key" in error_msg or "Available context keys" in error_msg diff --git a/tests/test_runtime/test_null_defaults.py b/tests/test_runtime/test_null_defaults.py index d59079dc6..6058d7158 100644 --- a/tests/test_runtime/test_null_defaults.py +++ b/tests/test_runtime/test_null_defaults.py @@ -279,18 +279,92 @@ def test_multiple_null_defaults(self): "inputs": { "input1": {"required": False, "default": None}, "input2": {"required": False, "default": ""}, - "input3": {"required": False}, # No default at all + "input3": {"required": False}, # No default at all - should resolve to None }, } registry = Registry() - # Note: input3 without default and not provided will fail validation - # So we need to provide it - flow = compile_ir_to_flow(workflow_ir, registry, initial_params={"input3": "value3"}) + # Optional inputs without explicit defaults should resolve to None + # This allows templates like ${input3} to work without requiring a value + flow = compile_ir_to_flow(workflow_ir, registry, initial_params={}) node = flow.start_node assert hasattr(node, "initial_params") - assert node.initial_params["input1"] is None # null default + assert node.initial_params["input1"] is None # explicit null default assert node.initial_params["input2"] == "" # empty string default - assert node.initial_params["input3"] == "value3" # provided value + assert node.initial_params["input3"] is None # implicit None (no default specified) + + def test_optional_input_without_default_resolves_to_none(self): + """Test that optional inputs without defaults resolve to None in templates. + + This is a regression test for the bug where optional inputs without defaults + failed template resolution with "Unresolved variables" error. + + Bug report: Optional inputs declared with required=false but no default value + should resolve to None when not provided, not fail validation. + """ + workflow_ir = { + "ir_version": "0.1.0", + "nodes": [ + { + "id": "test", + "type": "shell", + "params": { + "stdin": "${optional_param}", + "command": "cat", + }, + } + ], + "edges": [], + "inputs": { + "optional_param": { + "type": "string", + "required": False, + # No default specified - should resolve to None + "description": "Optional parameter with no default", + }, + }, + } + + registry = Registry() + + # Should compile successfully - optional input without default resolves to None + flow = compile_ir_to_flow(workflow_ir, registry, initial_params={}) + + node = flow.start_node + assert hasattr(node, "initial_params") + assert node.initial_params["optional_param"] is None + + def test_optional_input_without_default_can_be_overridden(self): + """Test that optional inputs without defaults can still be provided.""" + workflow_ir = { + "ir_version": "0.1.0", + "nodes": [ + { + "id": "test", + "type": "shell", + "params": { + "stdin": "${optional_param}", + "command": "cat", + }, + } + ], + "edges": [], + "inputs": { + "optional_param": { + "type": "string", + "required": False, + # No default specified + }, + }, + } + + registry = Registry() + + # Provide a value for the optional input + flow = compile_ir_to_flow(workflow_ir, registry, initial_params={"optional_param": "user_provided"}) + + node = flow.start_node + assert hasattr(node, "initial_params") + assert node.initial_params["optional_param"] == "user_provided" From 983aa2852cbd7a082fdb5e2c95762f722e89984a Mon Sep 17 00:00:00 2001 From: spinje Date: Thu, 8 Jan 2026 12:00:00 +0000 Subject: [PATCH 2/2] test: add edge case tests per code review feedback [skip review] Addresses code review feedback on PR #49: - Add test for nested path access on None-valued optional input (verifies ${optional_config.field} fails gracefully when config is None) - Add test for nested path access when optional input IS provided (verifies normal case still works) - Enhance comment in workflow_validator.py explaining the design decision and noting that nested access on None will still fail (intentionally) This documents the expected behavior: optional inputs resolve to None, but attempting to traverse into None (e.g., ${config.api_key}) correctly fails with a clear error message. --- .../optional-input-resolution/README.md | 190 ++++++++++++++++++ .../test-cases/01-optional-no-default.json | 23 +++ .../test-cases/02-optional-with-default.json | 24 +++ .../test-cases/03-mixed-templates.json | 29 +++ .../test-cases/04-real-world-example.json | 56 ++++++ src/pflow/runtime/workflow_validator.py | 8 +- tests/test_runtime/test_null_defaults.py | 80 ++++++++ 7 files changed, 408 insertions(+), 2 deletions(-) create mode 100644 scratchpads/optional-input-resolution/README.md create mode 100644 scratchpads/optional-input-resolution/test-cases/01-optional-no-default.json create mode 100644 scratchpads/optional-input-resolution/test-cases/02-optional-with-default.json create mode 100644 scratchpads/optional-input-resolution/test-cases/03-mixed-templates.json create mode 100644 scratchpads/optional-input-resolution/test-cases/04-real-world-example.json diff --git a/scratchpads/optional-input-resolution/README.md b/scratchpads/optional-input-resolution/README.md new file mode 100644 index 000000000..aea74dea4 --- /dev/null +++ b/scratchpads/optional-input-resolution/README.md @@ -0,0 +1,190 @@ +# Bug Report: Optional Inputs Without Defaults Fail Template Resolution + +## Summary + +When a workflow input is declared with `required: false` but no `default` value, and the user doesn't provide a value, any template referencing that input fails with "Unresolved variables". + +Expected behavior: Optional inputs should resolve to `null` or empty string when not provided. + +## Problem Statement + +Users declare optional inputs to allow flexible workflow usage. However, if they reference an optional input in a template and the user doesn't provide a value, the workflow fails - defeating the purpose of making it optional. + +### Current Behavior + +```json +{ + "inputs": { + "optional_param": {"type": "string", "required": false} + }, + "nodes": [{ + "id": "test", + "type": "shell", + "params": { + "stdin": "${optional_param}", + "command": "cat" + } + }] +} +``` + +Running without the optional param: +``` +$ pflow workflow.json + +❌ Workflow execution failed +Error: Unresolved variables in parameter 'stdin': ${optional_param} +``` + +### Expected Behavior + +The workflow should succeed with `${optional_param}` resolving to `null` or empty string: + +``` +$ pflow workflow.json +✓ Workflow completed +(empty output) +``` + +## Steps to Reproduce + +### Test 1: Optional input without default (FAILS) + +```json +{ + "inputs": { + "optional_name": {"type": "string", "required": false} + }, + "nodes": [{ + "id": "greet", + "type": "shell", + "params": {"stdin": "${optional_name}", "command": "cat"} + }], + "edges": [] +} +``` + +```bash +pflow test.json # No value provided +# Result: FAILS with "Unresolved variables" +``` + +### Test 2: Optional input WITH default (WORKS) + +```json +{ + "inputs": { + "optional_name": {"type": "string", "required": false, "default": "World"} + }, + "nodes": [{ + "id": "greet", + "type": "shell", + "params": {"stdin": "${optional_name}", "command": "cat"} + }], + "edges": [] +} +``` + +```bash +pflow test.json # No value provided +# Result: WORKS, outputs "World" +``` + +### Test 3: Mixed templates - misleading error (FAILS) + +```json +{ + "inputs": { + "provided": {"type": "string", "required": true}, + "missing": {"type": "string", "required": false} + }, + "nodes": [{ + "id": "test", + "type": "shell", + "params": { + "stdin": {"a": "${provided}", "b": "${missing}"}, + "command": "jq ." + } + }], + "edges": [] +} +``` + +```bash +pflow test.json provided="hello" +``` + +**Actual error:** +``` +Error: Unresolved variables in parameter 'stdin': ${provided}, ${missing} + +Available context keys: + • provided (str): hello +``` + +**Issue:** Error says `${provided}` is unresolved, but it's clearly in context. The error should only list `${missing}`. + +## Root Cause Analysis + +1. Optional inputs without defaults are **not added to the context** at all +2. Template resolution **fails on first unresolvable variable** +3. Error reporting **lists all templates** in the parameter as unresolved, not just the problematic ones + +## Impact + +- **Workaround tax:** Users must always provide `default` for optional inputs, even when empty string isn't semantically correct +- **Confusing errors:** Error message suggests ALL templates failed, even ones with valid values +- **Design limitation:** Can't distinguish between "user provided empty string" vs "user didn't provide value" + +## Proposed Solutions + +### Option A: Resolve missing optional inputs to null (Recommended) + +When an optional input has no default and no value is provided: +- Add it to context with value `null` +- Templates like `${optional_param}` resolve to `null` (or empty string in string contexts) + +**Pros:** Most intuitive behavior, matches how optional parameters work in most languages +**Cons:** Might break workflows that rely on current "fail if missing" behavior + +### Option B: Resolve to empty string + +Similar to Option A but use empty string `""` instead of `null`. + +**Pros:** Simpler, no null handling needed +**Cons:** Can't distinguish between "not provided" and "provided empty" + +### Option C: Add explicit null syntax + +Allow workflows to handle missing values explicitly: +```json +"stdin": "${optional_param ?? 'default_value'}" +``` + +**Pros:** Maximum flexibility +**Cons:** More complex, changes template syntax + +## Secondary Issue: Misleading Error Message + +When multiple templates exist and ONE is unresolvable, the error lists ALL as unresolved: + +``` +Error: Unresolved variables in parameter 'stdin': ${provided}, ${missing} +``` + +Should only list actually unresolvable ones: +``` +Error: Unresolved variables in parameter 'stdin': ${missing} +``` + +This is lower priority but would help debugging. + +## Test Cases + +See `test-cases/` directory for reproduction workflows. + +## Environment + +- pflow version: (current) +- OS: macOS (Darwin 24.6.0) +- Date discovered: 2026-01-08 diff --git a/scratchpads/optional-input-resolution/test-cases/01-optional-no-default.json b/scratchpads/optional-input-resolution/test-cases/01-optional-no-default.json new file mode 100644 index 000000000..bef495405 --- /dev/null +++ b/scratchpads/optional-input-resolution/test-cases/01-optional-no-default.json @@ -0,0 +1,23 @@ +{ + "_test": "Optional input without default - currently FAILS, should PASS", + "_run": "pflow 01-optional-no-default.json", + "_expected": "Should succeed with empty/null output", + "inputs": { + "optional_name": { + "type": "string", + "required": false, + "description": "Optional name with no default" + } + }, + "nodes": [ + { + "id": "greet", + "type": "shell", + "params": { + "stdin": "${optional_name}", + "command": "echo \"Got: $(cat)\"" + } + } + ], + "edges": [] +} diff --git a/scratchpads/optional-input-resolution/test-cases/02-optional-with-default.json b/scratchpads/optional-input-resolution/test-cases/02-optional-with-default.json new file mode 100644 index 000000000..089e284b0 --- /dev/null +++ b/scratchpads/optional-input-resolution/test-cases/02-optional-with-default.json @@ -0,0 +1,24 @@ +{ + "_test": "Optional input with default - currently WORKS (baseline)", + "_run": "pflow 02-optional-with-default.json", + "_expected": "Should succeed with 'World' output", + "inputs": { + "optional_name": { + "type": "string", + "required": false, + "default": "World", + "description": "Optional name with default" + } + }, + "nodes": [ + { + "id": "greet", + "type": "shell", + "params": { + "stdin": "${optional_name}", + "command": "echo \"Got: $(cat)\"" + } + } + ], + "edges": [] +} diff --git a/scratchpads/optional-input-resolution/test-cases/03-mixed-templates.json b/scratchpads/optional-input-resolution/test-cases/03-mixed-templates.json new file mode 100644 index 000000000..910acb94f --- /dev/null +++ b/scratchpads/optional-input-resolution/test-cases/03-mixed-templates.json @@ -0,0 +1,29 @@ +{ + "_test": "Mixed resolvable and unresolvable templates - shows misleading error", + "_run": "pflow 03-mixed-templates.json provided=hello", + "_expected": "Should succeed OR error should only mention ${missing}, not ${provided}", + "inputs": { + "provided": { + "type": "string", + "required": true + }, + "missing": { + "type": "string", + "required": false + } + }, + "nodes": [ + { + "id": "test", + "type": "shell", + "params": { + "stdin": { + "has_value": "${provided}", + "no_value": "${missing}" + }, + "command": "jq ." + } + } + ], + "edges": [] +} diff --git a/scratchpads/optional-input-resolution/test-cases/04-real-world-example.json b/scratchpads/optional-input-resolution/test-cases/04-real-world-example.json new file mode 100644 index 000000000..22036a039 --- /dev/null +++ b/scratchpads/optional-input-resolution/test-cases/04-real-world-example.json @@ -0,0 +1,56 @@ +{ + "_test": "Real-world example: optional output path", + "_run": "pflow 04-real-world-example.json url=https://example.com", + "_expected": "Should work - use auto-generated filename when output_path not provided", + "_notes": "This pattern is common: allow user to override output location, but have sensible default", + "inputs": { + "url": { + "type": "string", + "required": true, + "description": "URL to fetch" + }, + "output_path": { + "type": "string", + "required": false, + "description": "Optional custom output path" + } + }, + "nodes": [ + { + "id": "generate-default-path", + "type": "shell", + "params": { + "command": "echo './output.txt'" + } + }, + { + "id": "determine-path", + "type": "shell", + "purpose": "Use custom path if provided, otherwise use default", + "params": { + "stdin": { + "custom": "${output_path}", + "default": "${generate-default-path.stdout}" + }, + "command": "jq -r 'if .custom != \"\" and .custom != null then .custom else .default end'" + } + }, + { + "id": "save-result", + "type": "shell", + "params": { + "command": "echo 'Would save to: ${determine-path.stdout}'" + } + } + ], + "edges": [ + { + "from": "generate-default-path", + "to": "determine-path" + }, + { + "from": "determine-path", + "to": "save-result" + } + ] +} diff --git a/src/pflow/runtime/workflow_validator.py b/src/pflow/runtime/workflow_validator.py index 39b5ed6a7..1927f7b2c 100644 --- a/src/pflow/runtime/workflow_validator.py +++ b/src/pflow/runtime/workflow_validator.py @@ -173,8 +173,12 @@ def prepare_inputs( ) defaults[input_name] = default_value else: - # Optional with no default key - use None so templates can resolve - # This allows ${optional_param} to resolve to None instead of failing + # Optional inputs without explicit default resolve to None. + # Rationale: "required: false" means "can be omitted", and omitted + # values should still be available in context (as None) so templates + # like ${optional_param} can resolve rather than fail validation. + # Note: Nested access like ${optional_param.field} will still fail + # at runtime since you can't traverse into None - this is intentional. logger.debug( f"Optional input '{input_name}' not provided, using None as default", extra={"phase": "input_validation", "input": input_name}, diff --git a/tests/test_runtime/test_null_defaults.py b/tests/test_runtime/test_null_defaults.py index 6058d7158..11d37602f 100644 --- a/tests/test_runtime/test_null_defaults.py +++ b/tests/test_runtime/test_null_defaults.py @@ -368,3 +368,83 @@ def test_optional_input_without_default_can_be_overridden(self): node = flow.start_node assert hasattr(node, "initial_params") assert node.initial_params["optional_param"] == "user_provided" + + def test_nested_path_on_none_optional_input_fails_gracefully(self): + """Test that nested path access on None-valued optional input fails with clear error. + + When an optional input without default resolves to None, attempting to access + nested paths like ${optional_param.field} should fail with an informative error, + not silently pass or crash. + + This is the expected behavior: you can't access .field on None. + """ + workflow_ir = { + "ir_version": "0.1.0", + "nodes": [ + { + "id": "test", + "type": "shell", + "params": { + "stdin": "${optional_config.api_key}", + "command": "cat", + }, + } + ], + "edges": [], + "inputs": { + "optional_config": { + "type": "object", + "required": False, + # No default - will resolve to None + }, + }, + } + + registry = Registry() + + # Compilation should succeed - the input is optional + flow = compile_ir_to_flow(workflow_ir, registry, initial_params={}) + + # But execution should fail because we can't access .api_key on None + # This is correct behavior - nested path on None is an error + with pytest.raises(ValueError, match="optional_config"): + flow.run({}) + + def test_nested_path_on_provided_optional_input_succeeds(self): + """Test that nested path access works when optional input is provided.""" + workflow_ir = { + "ir_version": "0.1.0", + "nodes": [ + { + "id": "test", + "type": "shell", + "params": { + "stdin": "${optional_config.api_key}", + "command": "cat", + }, + } + ], + "edges": [], + "inputs": { + "optional_config": { + "type": "object", + "required": False, + }, + }, + } + + registry = Registry() + + # Provide the optional input with nested structure + flow = compile_ir_to_flow( + workflow_ir, + registry, + initial_params={"optional_config": {"api_key": "secret123"}}, + ) + + # Execution should succeed + shared = {} + flow.run(shared) + + # Verify the nested value was resolved + assert shared.get("test", {}).get("stdout", "").strip() == "secret123"